Replace the GUI Builder with a Maven-first Codename One app - #5523
Replace the GUI Builder with a Maven-first Codename One app#5523shai-almog wants to merge 25 commits into
Conversation
The Settings-era GUI Builder is a Swing tool tied to the old project layout. This replaces it with a standalone Codename One desktop app under scripts/guibuilder, launched by `mvn cn1:guibuilder`, in the same shape as `cn1:settings` and the Game Builder: its own Maven build, its own executable JAR, and its own Maven Central coordinates. The editor edits `.gui` XML in `src/main/guibuilder` and round-trips the generated Java next to it, so the design surface and the source stay in step. Guided Layout builds on LayeredLayout with builder-owned, name-based relationships (match width/height, reference targets, anchors), which is why the model enforces unique component names and cascades renames, deletes, and pastes across every relationship that points at them. Core changes are the minimum the editor needs: - CodeEditor gains protected-region markers and caret positioning, so the generated regions of a form's Java cannot be edited by hand. - LayeredLayout UNIT_BASELINE now only uses a component's reported baseline when the component also describes its baseline resize behavior. The default `Component#getBaseline` returns the bottom content edge rather than a text baseline, so without this the documented font-ascent fallback was unreachable and containers and text areas aligned on the wrong line. - SplitPane and Tabs no longer assume `getComponentForm()` is non-null; both can be deinitialized by the same gesture that triggers the callback, which the builder hits routinely when it rebuilds the inspector. `cn1:guibuilder` now forwards every `guibuilder.*` property, passes the desktop identity and `--add-exports` arguments the JavaSE runtime needs, and fails with a clear message when Maven is running on a JDK older than 17. scripts/** is excluded from PR CI, so .github/workflows/guibuilder.yml is added as the only job that compiles the editor against a freshly built core. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR replaces the legacy Swing “Settings-era” GUI Builder with a Maven-first, standalone Codename One desktop app under scripts/guibuilder, integrated with the Maven plugin via mvn cn1:guibuilder. It also adds the minimal core framework changes required to support the new editor (protected regions in the code editor, corrected baseline alignment in LayeredLayout, and null-safety fixes).
Changes:
- Introduces the new standalone GUI Builder app (common + JavaSE modules), demo project assets, and interaction/unit tests.
- Extends core editor/layout APIs to support protected generated regions and correct baseline alignment behavior.
- Updates Maven plugin + release/CI workflows to build, test, and publish the new GUI Builder artifact (
com.codenameone:codenameone-guibuilder).
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/guibuilder/tools/guibuilder-mcp-client.mjs | Adds a local Node MCP client for driving/inspecting the GUI Builder over MCP. |
| scripts/guibuilder/pom.xml | Adds standalone GUI Builder Maven reactor parent (Java 17) with publishing profile. |
| scripts/guibuilder/javase/src/test/java/com/codename1/guibuilder/GeneratedSourceTest.java | Verifies generated sources compile together (form + model strategies + guided constraints). |
| scripts/guibuilder/javase/src/test/java/com/codename1/guibuilder/CodeEditorInteractionTest.java | Tests protected region behavior and caret positioning in the pure editor. |
| scripts/guibuilder/javase/src/desktop/java/com/codename1/guibuilder/CodenameOneGUIBuilderStub.java | Desktop stub/launcher wiring + self-tests for editor/guided layout/interaction. |
| scripts/guibuilder/javase/src/desktop/java/com/codename1/guibuilder/CodenameOneGUIBuilderLauncher.java | Small main-class wrapper for the executable jar. |
| scripts/guibuilder/javase/pom.xml | Defines the published com.codenameone:codenameone-guibuilder JavaSE module and executable-jar profile. |
| scripts/guibuilder/demo-project/src/main/guibuilder/com/example/TableLayoutForm.gui | Demo GUI fixture for TableLayout behaviors. |
| scripts/guibuilder/demo-project/src/main/guibuilder/com/example/NestedLayoutsForm.gui | Demo GUI fixture for nested layout hierarchy behaviors. |
| scripts/guibuilder/demo-project/src/main/guibuilder/com/example/LoginForm.gui | Demo GUI fixture for a basic form. |
| scripts/guibuilder/demo-project/src/main/guibuilder/com/example/GuidedLayoutForm.gui | Demo GUI fixture for Guided Layout constraints and baseline snapping. |
| scripts/guibuilder/demo-project/src/main/guibuilder/com/example/GridLayoutForm.gui | Demo GUI fixture for GridLayout reorder/cell behaviors. |
| scripts/guibuilder/demo-project/src/main/guibuilder/com/example/BoxXLayoutForm.gui | Demo GUI fixture for horizontal BoxLayout scrolling/reorder. |
| scripts/guibuilder/demo-project/src/main/guibuilder/com/example/BorderDropForm.gui | Demo GUI fixture for BorderLayout drop/constraint behaviors. |
| scripts/guibuilder/demo-project/src/main/css/theme.css | Demo project theme for previewing styling + dark mode. |
| scripts/guibuilder/common/src/test/java/com/codename1/guibuilder/project/ProjectBindingTest.java | Unit test for parsing the modern binding format. |
| scripts/guibuilder/common/src/test/java/com/codename1/guibuilder/model/GuiDocumentTest.java | Unit tests for document editing, undo/redo, naming, relationships, drag/drop logic. |
| scripts/guibuilder/common/src/main/java/com/codename1/guibuilder/ui/GuidedLayoutSupport.java | Applies name-based Guided Layout constraints into LayeredLayout at preview/runtime. |
| scripts/guibuilder/common/src/main/java/com/codename1/guibuilder/ui/DragGuideOverlay.java | Overlay painting for drag/drop guides, selection, and simulated layout previews. |
| scripts/guibuilder/common/src/main/java/com/codename1/guibuilder/ui/ComponentPreviewFactory.java | Renders live preview components from .gui XML with designer interaction hooks. |
| scripts/guibuilder/common/src/main/java/com/codename1/guibuilder/project/ProjectIO.java | Loads binding + reads/writes GUI/CSS/source content via FileSystemStorage. |
| scripts/guibuilder/common/src/main/java/com/codename1/guibuilder/project/ProjectBinding.java | Binding model for guibuilder.input key/value format. |
| scripts/guibuilder/common/src/main/java/com/codename1/guibuilder/model/GuiDocument.java | Core .gui XML document model with transactions, undo/redo, and relationship management. |
| scripts/guibuilder/common/src/main/java/com/codename1/guibuilder/GuiBuilderMcpController.java | MCP tool registration and event/state streaming for automation/interaction tests. |
| scripts/guibuilder/common/src/main/css/theme.css | Editor UI theme (builder chrome styling + dark mode). |
| scripts/guibuilder/common/pom.xml | Common module config incl. cn1 plugin integration and test artifact attachment. |
| scripts/guibuilder/common/codenameone_settings.properties | GUI Builder CN1 settings (Java 17, desktop defaults, theme flags). |
| scripts/guibuilder/.gitignore | Ignores build output + generated binding input file for demo project. |
| maven/update-version.sh | Extends version bump script to include the new GUI Builder reactor. |
| maven/core-unittests/src/test/java/com/codename1/ui/layouts/LayeredLayoutTest.java | Adds regression test for true-baseline alignment with padding/margins. |
| maven/core-unittests/src/test/java/com/codename1/ui/CodeEditorTest.java | Adds regression tests for protected markers and caret movement. |
| maven/codenameone-maven-plugin/src/test/java/com/codename1/maven/OpenGuiBuilderMojoTest.java | Tests binding output, property forwarding, desktop identity args, and project dir detection. |
| maven/codenameone-maven-plugin/src/main/java/com/codename1/maven/OpenGuiBuilderMojo.java | Replaces legacy jar-based GUI Builder launch with Maven-resolved Java 17 editor launch. |
| CodenameOne/src/com/codename1/ui/Tabs.java | Adds null-safety around getComponentForm() during gesture handling. |
| CodenameOne/src/com/codename1/ui/layouts/LayeredLayout.java | Updates baseline unit behavior to use declared baselines only when resize behavior is declared. |
| CodenameOne/src/com/codename1/ui/editor/PureEditor.java | Adds a setCursor command for pure editor caret positioning. |
| CodenameOne/src/com/codename1/ui/editor/CodeView.java | Adds protected region markers that block edits to generated ranges. |
| CodenameOne/src/com/codename1/ui/editor/CodePureEditor.java | Wires setProtectedMarkers command into CodeView protected-region support. |
| CodenameOne/src/com/codename1/ui/CodeEditor.java | Public API for protected region markers and caret positioning. |
| CodenameOne/src/com/codename1/components/SplitPane.java | Adds null-safety around getComponentForm() during init. |
| .github/workflows/release-on-maven-central.yml | Extends release workflow to deploy/confirm/publish GUI Builder alongside other editors. |
| .github/workflows/guibuilder.yml | Adds dedicated CI workflow to build core + compile/test/package the standalone GUI Builder. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ((start == end && start >= protectedStart && start <= protectedEnd) | ||
| || (start < protectedEnd && end > protectedStart)) { | ||
| return true; | ||
| } |
| if (!line.trim()) continue; | ||
| const message = JSON.parse(line); | ||
| const handler = pending.get(message.id); | ||
| if (handler) { | ||
| pending.delete(message.id); | ||
| handler(message); |
| private static void ensureParent(String path) { | ||
| int slash = path.lastIndexOf('/'); | ||
| if (slash <= "file://".length()) return; | ||
| String parent = path.substring(0, slash); |
| if (model == null) { | ||
| result = compiler.run(null, null, null, "-classpath", System.getProperty("java.class.path"), formFile.toString()); | ||
| } else { | ||
| Path modelFile = pkg.resolve("LoginFormModel.java"); | ||
| Files.write(modelFile, model.getBytes(StandardCharsets.UTF_8)); | ||
| result = compiler.run(null, null, null, "-classpath", System.getProperty("java.class.path"), | ||
| formFile.toString(), modelFile.toString()); | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b3fb02eb7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!(value instanceof Element child) || !"component".equals(child.getTagName())) continue; | ||
| String name = javaName(child); | ||
| String type = value(child, "type", "Container"); | ||
| out.append(indent).append(name).append(" = ").append(componentSource(child)).append(";\n") |
There was a problem hiding this comment.
When a form uses the companion Java source path, properties edited in the inspector such as enabled, visible, selected, editable, slider progress, rows/columns, alignment, and similar attributes are honored by the preview via ComponentPreviewFactory.applyAttributes, but this generator only emits the constructor/name/UIID/action wiring. Saving or opening the generated source for a form with those attributes therefore produces a runtime UI that silently reverts to defaults instead of matching the .gui document.
Useful? React with 👍 / 👎.
| out.append(indent).append(parentName).append(".add(((TableLayout) ").append(parentName) | ||
| .append(".getLayout()).createConstraint(").append(value(child, "tableRow", "0")) | ||
| .append(", ").append(value(child, "tableColumn", "0")).append(")") | ||
| .append(".horizontalSpan(").append(value(child, "tableHorizontalSpan", "1")).append(")") | ||
| .append(".verticalSpan(").append(value(child, "tableVerticalSpan", "1")).append("), ") |
There was a problem hiding this comment.
For TableLayout children, the inspector exposes tableWidth and tableHeight and the preview applies them with widthPercentage() / heightPercentage(), but the generated companion source emits only row, column, and span. Any form that sets a column width or row height percentage in the builder will preview correctly but lose those table constraints in the generated Java UI.
Useful? React with 👍 / 👎.
| if (cleaned.length() == 0) return "component"; | ||
| char first = cleaned.charAt(0); | ||
| boolean validStart = first == '_' || first == '$' || first >= 'A' && first <= 'Z' || first >= 'a' && first <= 'z'; | ||
| return validStart ? cleaned : "_" + cleaned; |
There was a problem hiding this comment.
This sanitization still returns Java keywords and can collapse distinct GUI names to the same Java name, e.g. components named class or foo-bar plus foo_bar. Because the generated source uses these values for fields and handler method names, those valid .gui names can produce uncompilable companion Java even though the builder accepts them; reserve-word handling and collision avoidance need to happen here or during rename.
Useful? React with 👍 / 👎.
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Cloudflare Preview
|
|
Compared 181 screenshots: 181 matched. |
Two demos did not survive real use. **TableLayout moved components nobody dragged.** Every drop ran the table through normalizeTableCells, which reassigned row/column from sibling order. That is what implicit, code-first TableLayout does, and it is wrong for a designer: dropping one component renumbered every other cell, so the table bounced into a layout the user never asked for. A table drop is now a placement into one addressed cell. TablePlacementAdapter picks the cell from the pointer over the parent's own geometry, an occupied cell swaps with its occupant instead of pushing the sequence along, and normalizeTableCells only assigns cells to children that have none or that collide. Moving a component earlier or later in a table now swaps the two cells rather than renumbering the table, so the reorder means what it says. XML order is left alone: in a table it carries no layout meaning, and churning it churns the generated source for nothing. **Nested containers looked broken because the canvas stopped following the model.** The drop spacer shown during a drag called animateLayout on a preview container. A layout animation captures the component tree and re-applies that captured state when it finishes, and the drop commits and rebuilds the canvas well inside that window -- so the animation restored the pre-drop preview over the new one and left the spacer behind. The model was always right; the canvas showed the component in its old parent, with the old parent's geometry. Hit testing then worked off phantom rectangles, which is why the next drag missed and the whole editor felt unstable. Nested layouts hit it hardest because each level animated. The spacer now revalidates instead. Undo and redo restore by reparsing, so every Element identity changes even though the form is unchanged. The multi-selection was dropped rather than re-resolved, silently deselecting after every undo; it is now rebound by name. Attributes were serialized in Hashtable order, so the same form produced different text on different runs. That made the transaction's did-anything- change check unreliable and churned unrelated lines on every save. GuiDocument now writes them in a stable order -- type, name, layout, then alphabetical. LiveWorkspaceDragTest is the regression harness this needed: it drives the assembled workspace rather than the document model, because the model-level tests passed throughout for gestures that visibly failed in the editor. It asserts one preview per component and that each preview renders inside the parent the model claims. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 45 out of 46 changed files in this pull request and generated no new comments.
Suppressed comments (4)
CodenameOne/src/com/codename1/ui/editor/CodeView.java:163
- Edits at the exact end of a protected block (i.e., immediately after the end marker) are currently treated as "inside" the protected region because the caret check uses
<= protectedEnd. This prevents inserting text right after the generated block, which should be allowed.
if ((start == end && start >= protectedStart && start <= protectedEnd)
|| (start < protectedEnd && end > protectedStart)) {
return true;
scripts/guibuilder/tools/guibuilder-mcp-client.mjs:23
- Incoming messages are parsed with
JSON.parse(line)without error handling. If the server emits a malformed line (or the stream is corrupted), the client will crash rather than reporting a parse error and continuing.
scripts/guibuilder/common/src/main/java/com/codename1/guibuilder/project/ProjectIO.java:87 fsUrl()currently just prependsfile://without normalizing Windows paths. On Windows this will produce invalid file URLs (e.g.file://C:\Users\...), and consumers that stripfile://can end up with drive-relative paths (the same pitfall covered by scripts/settings ProjectIOTest). This can break project file reads/writes on Windows.
public static String fsUrl(String path) {
if (path == null || path.startsWith("file://") || path.indexOf("://") > 0) return path;
return "file://" + path;
}
scripts/guibuilder/tools/guibuilder-mcp-client.mjs:12
portis parsed withNumber(...)and used directly innet.createConnection(). If the env var/arg is non-numeric, this becomesNaNand the client fails with a low-signal runtime error. Validating the port early provides a clearer failure mode.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0099dabf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <dependency><groupId>com.codenameone.guibuilder</groupId><artifactId>${cn1app.name}-common</artifactId><version>${project.version}</version></dependency> | ||
| <dependency><groupId>com.codenameone.guibuilder</groupId><artifactId>${cn1app.name}-common</artifactId><version>${project.version}</version><classifier>tests</classifier><scope>test</scope></dependency> | ||
| <dependency><groupId>com.codenameone</groupId><artifactId>codenameone-core</artifactId><scope>test</scope></dependency> | ||
| <dependency><groupId>com.codenameone</groupId><artifactId>codenameone-javase</artifactId><scope>test</scope></dependency> |
There was a problem hiding this comment.
When cn1:guibuilder runs from a user project, OpenGuiBuilderMojo resolves com.codenameone:codenameone-guibuilder transitively and launches exactly that resolved classpath. With codenameone-javase only test-scoped here (and compile-scoped only inside the non-activated executable-jar build profile), a normal consumer resolution of the published POM omits com.codename1.impl.javase.JavaSEPort, so the launcher fails with NoClassDefFoundError before the GUI appears. Make the JavaSE runtime a normal runtime dependency or add it explicitly to the mojo-resolved classpath.
Useful? React with 👍 / 👎.
| else if ("Slider".equals(type)) source = "new Slider()"; | ||
| else if ("TextField".equals(type)) source = "new TextField(" + text + ", \"" + javaEscape(value(element, "hint", "")) + "\")"; | ||
| else if ("TextArea".equals(type)) source = "new TextArea(" + text + ")"; | ||
| else source = "new " + type + "(" + text + ")"; |
There was a problem hiding this comment.
For a legacy .gui containing type="Accordion", the document model treats Accordion as a child-accepting component, but this fallback emits new Accordion("...") and the generated class imports only SpanLabel from com.codename1.components. Accordion has no String constructor and is not in com.codename1.ui.*, so opening/saving companion source for that supported component produces uncompilable Java; add an explicit Accordion branch/import or reject it as unsupported.
Useful? React with 👍 / 👎.
Nested containers are the weak spot: a move is a remove from one parent and an add to another, and anything going wrong between the two loses the component from the document entirely. The existing tests checked the component that moved, which is exactly the assertion that passes when a component ends up in neither container. NestedHierarchyTest re-checks the entire document after every single gesture: no component gained or lost, no duplicate names, every parent link agreeing with the child list it claims, the tree surviving a save/load round trip, and every component rendering exactly once at a non-zero size. It covers draining a container child by child, refilling an emptied one, moving populated containers, four-level nesting in both directions, refused cyclic drops, every layout as both source and destination, and undo/redo replaying a drain step by step. LiveWorkspaceDragTest gains the same accounting against the real canvas. Two defects it found: **Components added to a table had no cell.** The preview fell back to sibling order and the generated source fell back to cell (0, 0), so a table that looked correct in the designer compiled to every component stacked in one corner. GuiDocument now owns the rule -- effectiveTableRow/Column -- and both consumers use it, so they cannot drift apart again. New children are given an explicit free cell as they are added, growing the row count when they need it. **A drop aimed at a container it did not fit in landed elsewhere.** The MCP drag path derived the release point from the dragged component's own box and then added the grab offset, so dropping a full width button "into" a narrow column put the pointer past the column's right edge and the component went into the next one. Drops resolve from the pointer, so the pointer is what gets aimed: at the target's centre, or just outside the relevant edge for above/below/leftOf/rightOf, or inside the leading or trailing half for before/after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a745163cb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Set<String> taken = new LinkedHashSet<>(); | ||
| for (Element sibling : componentsIn(parent)) { | ||
| if (sibling == child) continue; | ||
| taken.add(effectiveTableRow(parent, sibling) + ":" + effectiveTableColumn(parent, sibling)); |
There was a problem hiding this comment.
When adding a component to a TableLayout that already contains a child with tableHorizontalSpan or tableVerticalSpan greater than 1, this occupancy set marks only the sibling's origin cell as taken. The next component can therefore be assigned to a cell covered by that span, so the designer preview and generated TableLayout.Constraint place two children in overlapping table space; mark every cell covered by each sibling's row/column span before picking the first free slot.
Useful? React with 👍 / 👎.
| if (document != null && document.isModified()) save(); | ||
| openForm(path); |
There was a problem hiding this comment.
If the user chooses Save in this unsaved-changes prompt but ProjectIO.write() fails, save() only shows an error toast and returns; this code still calls openForm(path), replacing the dirty in-memory document and discarding the edits in read-only/permission/disk-error scenarios. Have save() report success and keep the current form open when the write fails.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 46 out of 47 changed files in this pull request and generated no new comments.
Suppressed comments (4)
CodenameOne/src/com/codename1/ui/editor/CodeView.java:163
- The protected-region check treats the end marker as inclusive for insertions (
start <= protectedEnd), which blocks edits immediately after the closing marker (i.e., at the first character following the protected block). This makes it hard to place user code right after a generated section.
if ((start == end && start >= protectedStart && start <= protectedEnd)
|| (start < protectedEnd && end > protectedStart)) {
return true;
scripts/guibuilder/tools/guibuilder-mcp-client.mjs:26
JSON.parse(line)in the socket data handler is unguarded. Any malformed/partial line from the server will throw and crash the client process, leaving pending requests unresolved.
scripts/guibuilder/tools/guibuilder-mcp-client.mjs:43request()resolves even when the JSON-RPC response contains anerrorobject, so callers proceed as if initialization/tool calls succeeded. This should reject the promise on JSON-RPC errors.
maven/codenameone-maven-plugin/src/main/java/com/codename1/maven/OpenGuiBuilderMojo.java:68- The binding file name includes a random UUID, so every
mvn cn1:guibuilderrun leaves another staleguibuilder-*.inputin~/.codenameoneGUIBuilder. Over time this can accumulate unnecessarily.
File runtimeDir = new File(System.getProperty("user.home"), ".codenameoneGUIBuilder");
runtimeDir.mkdirs();
File input = new File(runtimeDir, "guibuilder-" + UUID.randomUUID() + ".input");
writeBinding(input, projectDir, guiDir, sourceDir, cssFile);
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
The demo project only exercised one nested form, and it was the one that failed. These cover the shapes the recent defects came from: four levels of containers, every layout nested inside every other, a column meant to be drained and refilled, an already-empty container, a single-child grid, tables inside tables, and a LayeredLayout container nested inside ordinary ones with ordinary containers nested back inside it. DemoFormsTest keeps them honest. A form that parses but renders nothing is a broken first impression for anyone evaluating the editor, so every form is checked for unique names, a coherent tree, every component rendering at a non-zero size, and a stable save/load round trip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 53 changed files in this pull request and generated no new comments.
Suppressed comments (4)
CodenameOne/src/com/codename1/ui/editor/CodeView.java:163
- Protected-region edit detection treats an insertion exactly at the end marker boundary as protected ("<= protectedEnd"), which prevents typing immediately after a generated block. This makes it impossible to add code right after the protected end marker without first moving further away.
if ((start == end && start >= protectedStart && start <= protectedEnd)
|| (start < protectedEnd && end > protectedStart)) {
return true;
CodenameOne/src/com/codename1/ui/layouts/LayeredLayout.java:3688
- UNIT_BASELINE absolute pixel calculation mixes the reference component's current size with the target component's preferred size when calling getBaseline(). This can yield incorrect baseline offsets for components that are resized by constraints. Use the component's current size when available, with a preferred-size fallback.
int componentBaseline = declaredBaseline(cmp, cmp.getPreferredW(), cmp.getPreferredH());
scripts/guibuilder/tools/guibuilder-mcp-client.mjs:22
- The TCP client assumes every non-empty line is valid JSON and calls JSON.parse() without a try/catch. Any non-JSON line (e.g., server logging, partial/corrupt output) will throw and crash the client, leaving pending requests unresolved. Handle parse errors explicitly and continue reading.
CodenameOne/src/com/codename1/ui/layouts/LayeredLayout.java:3142 - UNIT_BASELINE alignment uses the reference component's current size (ref.getWidth()/getHeight()) but computes the moved component's baseline using its preferred size. If the component is laid out at a non-preferred size, this can misalign baselines. Use the component's current size when available (with a preferred-size fallback if size is still 0 at this point).
This issue also appears on line 3688 of the same file.
int componentBaseline = declaredBaseline(cmp, cmp.getPreferredW(), cmp.getPreferredH());
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83742fbb96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Element previous = document.selected(); | ||
| Element added = null; | ||
| try { | ||
| added = document.addComponent(type); |
There was a problem hiding this comment.
When a palette item is released outside the form or onto an invalid/occupied target, this first inserts the new component under the current selection and only later deletes it on a rejected plan. If the current selection is a TableLayout, addComponent() assigns a free cell and may grow tableLayoutRows; the cleanup removes only the child, leaving the table changed and the document dirty even though the drop was cancelled or committed elsewhere. Create a detached candidate or roll back the whole pre-validation mutation before reporting an invalid drop.
Useful? React with 👍 / 👎.
| private void refreshProject() { | ||
| guiFiles = ProjectIO.findGuiFiles(binding.guiDir()); | ||
| refreshForms(); | ||
| if (document != null) openForm(document.path()); |
There was a problem hiding this comment.
When the current form has unsaved inspector/drag edits and the user presses Refresh (or invokes the MCP refresh command), this reloads the same .gui file from disk via openForm(document.path()) without saving or prompting. openForm() replaces the in-memory document, so those unsaved changes are discarded; refresh should either prompt like form switching or avoid reopening a modified form.
Useful? React with 👍 / 👎.
Draining a column in NestedLayoutsForm made every component on the form disappear in phone portrait. The empty-container marker was an ordinary label, so it asked for whatever width "Drop components here" needed -- wider than any real empty container -- and in a horizontal box that left no room for the next column, which was laid out past the right edge of the device and off the visible canvas. The components were never lost; there was nowhere on screen left to draw them. The marker exists to show a drop target, so it must never be the thing that decides a layout. It now has a small fixed preferred size and ellipsises its text. This is why the earlier accounting did not catch it: those tests ran on the desktop canvas, which is wide enough to absorb the overflow. The new test renders at the 720px phone-portrait width -- the narrowest canvas the editor offers, and the one the user hits first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Yellow titles and blue labels were experiment values, not intended demo styling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The desktop port maps the left soft key to VK_F1. That is 112, and 112 is also the character code of a lowercase p. Key codes and character codes share one value space, so MenuBar.handlesKeycode could not tell them apart: Form consumed every 112 as a soft key press and returned before the focused component saw it. Typing "pink" produced "ink", in any text component on the desktop, not only in the GUI Builder. VK_F2 is 113, so a two-soft-key configuration loses q the same way. A component that has called setHandlesInput(true) has declared it owns the keyboard, so it is now served ahead of soft key mapping. Components that have not still let the menu bar win, which is what soft keys are for. SoftKeyCollisionTest pins the real value rather than whatever the test implementation reports for a soft key -- it reports none, which is why nothing caught this. Reverting the guard fails it. This took an absurd number of rounds because each layer looked innocent on its own: the port delivered the key, Display queued and dispatched it, the editor was focused, editable and enabled. Only tracing all four layers at once in the running application showed Form dropping it between the last two. The tracing is removed now that the answer is known. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb7a361639
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ignore && !(e.isAltDown() || e.isControlDown() || e.isMetaDown() || e.isAltGraphDown())) { | ||
| ignore = false; | ||
| } |
There was a problem hiding this comment.
When a user releases the modifier before the shortcut key (for example, Ctrl+P followed by releasing Ctrl and then P), the P press was recorded in ignorePressedKeys, but its release no longer reports isControlDown(), so this branch clears ignore and dispatches an unmatched keyReleased('p'). Since Codename One text components enter characters on key release, using a shortcut can insert its final letter into the focused field; stale entries should be distinguished without relying on the modifiers still being present on the matching release.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 64 changed files in this pull request and generated no new comments.
Suppressed comments (6)
CodenameOne/src/com/codename1/ui/editor/EditorView.java:1362
focusGained()updates the global Display multi-key mode, but there is no flag to indicate that this instance actually changed it. Adding a capture flag allowsfocusLost()/deinitialize()to restore the global mode only when it was set by this EditorView.
// last one, so releases were being discarded and characters silently vanished -- always
// the same ones, because it depends on which pairs overlap. Multi key mode delivers every
// release. It is restored on focus loss so the rest of the application is unaffected.
multiKeyModeRestore = Display.getInstance().isMultiKeyMode();
Display.getInstance().setMultiKeyMode(true);
CodenameOne/src/com/codename1/ui/editor/EditorView.java:1395
deinitialize()restores multi-key mode unconditionally. If this EditorView was never focused, it may restore an uninitialized default and change global keyboard behavior. Guard the restore behind the capture flag (set infocusGained()), and clear the flag after restoring.
// caret blink when focus returns. Multi key mode is restored here for the same reason:
// focusLost never runs on this path and the setting is global.
Display.getInstance().setMultiKeyMode(multiKeyModeRestore);
stopInput();
CodenameOne/src/com/codename1/ui/editor/EditorView.java:1377
focusLost()always restores the global Display multi-key mode, even if this EditorView never captured/overrode it. This can incorrectly flip the global keyboard setting for the whole app. Restore only whenfocusGained()actually set multi-key mode, and clear the capture flag after restoring.
protected void focusLost() {
super.focusLost();
Display.getInstance().setMultiKeyMode(multiKeyModeRestore);
stopInput();
if (animRegistered && getComponentForm() != null) {
scripts/guibuilder/tools/guibuilder-mcp-client.mjs:22
- Incoming socket data is assumed to be valid JSON, so any malformed line (or partial line if framing ever breaks) will throw and crash the client. Wrap
JSON.parse()in a try/catch and continue on parse errors so the tool remains usable and can surface the bad line.
CodenameOne/src/com/codename1/ui/editor/EditorView.java:57 multiKeyModeRestoredefaults tofalse, butdeinitialize()always restores multi-key mode using this field. If an EditorView is never focused (sofocusGained()never captures the previous value) but is later deinitialized, this will forcibly set the global Display multi-key mode tofalse, potentially changing app-wide keyboard behavior.
This issue also appears in the following locations of the same file:
- line 1358
- line 1373
- line 1392
private int caret;
private int anchor = -1;
private boolean multiKeyModeRestore;
private int composingStart = -1;
scripts/guibuilder/tools/guibuilder-mcp-client.mjs:43
request()resolves successfully even when the server returns a JSON-RPCerrorobject, which can cause the CLI to print misleading output and makes it hard to script against failures. Treat JSON-RPC errors as promise rejections.
LiveTypingTest now selects the stylesheet, types a colour over it and asserts the canvas restyles. The earlier CSS test called the recompile directly, so it covered everything except the path that was broken: keystroke to change event to compile to repainted canvas. That path passes now that the keyboard reaches the editor. refreshProjectThemeOnPreview also repaints the form, not just the canvas host. A component repaints itself only when it is already painting its own region, and the canvas sits inside a split pane whose parent decides the frame, so restyling without asking the form for a frame could leave old pixels up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 63 out of 64 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
scripts/guibuilder/tools/guibuilder-mcp-client.mjs:22
- The data handler checks
line.trim()for emptiness but parses the untrimmedline. If the server uses CRLF (\r\n) framing,linewill end with\randJSON.parse(line)will throw, crashing the client. Parse the trimmed line (or strip\r) and treat parse failures as non-fatal so the client can keep running.
| public static String fsUrl(String path) { | ||
| if (path == null || path.startsWith("file://") || path.indexOf("://") > 0) return path; | ||
| return "file://" + path; | ||
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d5bc375d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| timer.start(); | ||
| } | ||
|
|
||
| @Override public void windowClosing(WindowEvent e) { Display.getInstance().exitApplication(); } |
There was a problem hiding this comment.
When the user closes the native window while the active .gui document or an embedded source/CSS editor has unsaved changes, this immediately calls exitApplication(), whose JavaSE implementation terminates the JVM without invoking a save or lifecycle callback. The Cmd/Ctrl+W menu item follows the same direct-exit path, so ordinary application closure permanently discards edits; route both paths through a shared save/discard/cancel confirmation.
Useful? React with 👍 / 👎.
| public void keyPressed(int keyCode) { | ||
| int game = Display.getInstance().getGameAction(keyCode); | ||
| if (menuBar.handlesKeycode(keyCode)) { | ||
| if (menuBar.handlesKeycode(keyCode) && !focusedHandlesInput()) { |
There was a problem hiding this comment.
On devices with actual soft keys or a back key, focusing any component whose interaction mode sets handlesInput()—including List, editable Slider, and MapComponent—now bypasses MenuBar handling. These components generally consume navigation/fire keys rather than the soft-key code, so the corresponding toolbar or back command is silently lost; the collision workaround should be limited to components that truly need printable raw text rather than every existing user of handlesInput().
Useful? React with 👍 / 👎.
| String canonical = name.toLowerCase(); | ||
| element.removeAttribute(canonical); | ||
| if (!canonical.equals(name)) element.removeAttribute(name); | ||
| if (value != null && value.trim().length() > 0) element.setAttribute(canonical, value.trim()); |
There was a problem hiding this comment.
When an inspector or inline edit updates text, hint, or title, this shared normalizer trims the value before storing it, so intentional leading/trailing whitespace in labels and multiline text is lost immediately. It also removes an explicitly empty textual attribute altogether, causing preview fallbacks such as "Label" or "Text field" to reappear when the user tries to clear the content; only structural attributes should be normalized this way.
Useful? React with 👍 / 👎.
| if (plan.occupied != null) { | ||
| int destinationIndex = document.componentIndex(plan.parent, plan.occupied); | ||
| document.select(plan.occupied); | ||
| if (!document.moveSelectedToParent(oldParent, oldIndex)) return false; |
There was a problem hiding this comment.
When a component is dragged from a TableLayout into an occupied BorderLayout region, the displaced border child is moved back into the original table here, but the subsequent oldLayout handling has no table branch and assigns neither tableRow nor tableColumn. If the table's XML order does not match its explicit cell coordinates, the fallback cell derived from sibling order can collide with an existing child, making the displaced component overlap another component in both preview and generated source; place it in the dragged component's vacated cell or normalize the table after the swap.
Useful? React with 👍 / 👎.
Asked for directly, and the gap was real: LiveCssTest writes its own one-rule stylesheet, which proves a rule can reach the preview but not that the project's rules do. If the canvas were styled by something unrelated to the file being edited, every existing test would still pass. ProjectCssStylesPreviewTest copies the demo project, writes known colours into its theme.css and asserts the components on the canvas resolve to exactly those colours -- a Label and a Button, by name, through the real workspace. A second test edits the stylesheet afterwards and asserts the canvas follows, using a named colour since that is what a person types. Both pass, so the project stylesheet does style the canvas and editing it does restyle the canvas, at the level of the resolved styles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fb15691de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| out.append(indent).append(name).append(" = ").append(componentSource(child)).append(";\n") | ||
| .append(indent).append(name).append(".setName(\"").append(javaEscape(value(child, "name", name))).append("\");\n"); | ||
| if (child.getAttribute("uiid") != null) out.append(indent).append(name).append(".setUIID(\"") | ||
| .append(javaEscape(child.getAttribute("uiid"))).append("\");\n"); |
There was a problem hiding this comment.
When a user changes properties such as Enabled, Visible, RTL, label alignment/gap, text-input constraints, slider progress, tab placement, or container scrolling, ComponentPreviewFactory.applyAttributes() reflects them in the designer, but this generated-source path emits only the name and optional UIID before adding the component. The saved .gui therefore appears correct in the builder while the generated runtime form silently uses defaults for these properties; emit the corresponding setters in the companion source.
Useful? React with 👍 / 👎.
| Map<String, Object> out = new LinkedHashMap<String, Object>(); | ||
| out.put("latestSequence", Long.valueOf(sequence)); | ||
| out.put("events", outEvents); |
There was a problem hiding this comment.
When more events are pending than the requested limit (for example, 500 queued events with the default limit of 100), this response returns only the first page but sets latestSequence to the global sequence. The tool contract tells clients to reuse latestSequence as afterSequence, so the next request skips every undelivered event from that page; return the sequence of the last event actually included, or expose a separate pagination cursor.
Useful? React with 👍 / 👎.
| .append(GuiDocument.effectiveTableRow(parent, child)) | ||
| .append(", ").append(GuiDocument.effectiveTableColumn(parent, child)).append(")") | ||
| .append(".horizontalSpan(").append(value(child, "tableHorizontalSpan", "1")).append(")") | ||
| .append(".verticalSpan(").append(value(child, "tableVerticalSpan", "1")).append("), ") | ||
| .append(name).append(");\n"); |
There was a problem hiding this comment.
When a user sets the TableLayout inspector's tableWidth or tableHeight percentage, the preview applies widthPercentage()/heightPercentage(), but the generated constraint chain emits only row, column, and spans. The compiled form consequently uses TableLayout's default sizing instead of the proportions shown in the designer; append the configured percentage setters when their values are not -1.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 64 out of 65 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
CodenameOne/src/com/codename1/ui/editor/CodeView.java:167
- CodeView's protected-region check treats an insertion at exactly protectedEnd (i.e. immediately after the end marker) as "inside" the protected region due to the
<= protectedEndcomparison. This can make it impossible to type at the boundary right after a generated block.
int endMarker = source.indexOf(protectedEndMarker,
protectedStart + protectedStartMarker.length());
int protectedEnd = endMarker < 0
? source.length() : endMarker + protectedEndMarker.length();
if ((start == end && start >= protectedStart && start <= protectedEnd)
|| (start < protectedEnd && end > protectedStart)) {
return true;
scripts/guibuilder/tools/guibuilder-mcp-client.mjs:43
- The MCP client stores only
resolvehandlers inpending, never rejects on JSON-RPC error responses, and doesn't clean up pending requests on socket close. This can causeawait request(...)to hang forever and also crashes the process on any malformed/non-JSON line due to an unguarded JSON.parse.
| @Override | ||
| protected void deinitialize() { | ||
| super.deinitialize(); | ||
| // Removing a focused component clears the form's focus without firing focusLost | ||
| // (Container.removeComponentImpl uses setFocusedInternal), so release the platform | ||
| // input session here or the stale handle would block startInput on the next focus | ||
| // gain, leaving the editor deaf until an explicit focus round-trip. The form also | ||
| // drops animation registrations on deinit, so reset the flag to re-register the | ||
| // caret blink when focus returns. | ||
| // caret blink when focus returns. Multi key mode is restored here for the same reason: | ||
| // focusLost never runs on this path and the setting is global. | ||
| Display.getInstance().setMultiKeyMode(multiKeyModeRestore); | ||
| stopInput(); |
…esheet
MCP reported geometry but never what the canvas was drawing with, so nothing
outside the process could tell whether a CSS edit had reached the preview or
stopped on the way. It now reports each component's UIID, resolved foreground
and background, and which UIManager resolved them, plus what the project theme
compiled to and what each manager resolves for a plain Label.
Pointing that at the running editor answers the question the tests could not.
For the demo project, whose theme.css sets Label colour #26324a:
projectThemeLabelFg 26324a <- the compiled theme is correct
previewLabelFg f5f8ff <- what the canvas resolves
globalLabelFg 6d6d72
themeApplyCount 2 <- the theme was installed, twice
Three different values. The canvas resolves from its own UIManager, that
manager is created fresh and handed the compiled project theme, and it still
answers with the builder's own colour. So UIManager.createInstance() followed
by setThemeProps() does not change what getComponentStyle() returns, which is
why every style level test passes -- under test no application theme is
installed to lose to -- while the running editor shows styling unrelated to
the stylesheet being edited.
Also removes the demo project's target directory from the copy the tests make,
and hardens build-and-run.sh: mvn clean was leaving a jar mixing fresh and
stale classes, so the core output directories are removed outright and three
more symbols are verified in the installed artifact.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed92f38545
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!ProjectIO.exists(modelPath) && !"none".equals(value(document.root(), "bindingStrategy", "properties"))) { | ||
| ProjectIO.write(modelPath, generatedModelSource()); | ||
| setStatus("Saved form source and created its binding model"); | ||
| } else { | ||
| setStatus("Saved form source • existing model left unchanged"); |
There was a problem hiding this comment.
When a form with an existing PropertyBusinessObject model is switched to @Bindable POJO, saving the regenerated companion source leaves that model unchanged here. The companion still compiles because Binders.bind() is generic, but constructing the form at runtime throws IllegalStateException because no binder is generated for the non-@Bindable model; switching in the opposite direction can make UiBinding.bind() fail compilation. Migrate or regenerate the existing model, or prevent the strategy change until the incompatibility is resolved.
Useful? React with 👍 / 👎.
| fields.add(actions); | ||
| fields.add(propertyField("Advanced insets (top right bottom left)", "layeredInsets", "auto auto auto auto")); | ||
| } else if ("TableLayout".equals(document.parentLayout(element))) { | ||
| fields.add(numericPropertyField("Table row", "tableRow", "0", 0, 99)); |
There was a problem hiding this comment.
When a user enters a row substantially beyond the parent table's declared row count—for example row 4 in a two-row table—this field accepts it and immediately rebuilds the preview. TableLayout.addLayoutComponent() grows the table by only one row when a constraint is out of range, then indexes the still-too-small positions array, so the refresh throws and leaves the document with a value it cannot render. Limit the field to the current table bounds or grow the table to include the requested row before refreshing.
Useful? React with 👍 / 👎.
| LOCAL_REPO="${CN1_LOCAL_REPO:-/tmp/cn1-local-repo}" | ||
| JDK8="${JAVA8_HOME:-$(/usr/libexec/java_home -v 1.8)}" | ||
| JDK21="${JAVA21_HOME:-$(/usr/libexec/java_home -v 21)}" | ||
| CORE_JAR="$LOCAL_REPO/com/codenameone/codenameone-core/8.0-SNAPSHOT/codenameone-core-8.0-SNAPSHOT.jar" |
There was a problem hiding this comment.
When maven/update-version.sh advances the project away from 8.0-SNAPSHOT, the core install writes a differently versioned artifact but this helper still verifies the hard-coded snapshot path; unzip then fails under set -e, and the later package, launch, and process checks are likewise tied to the old jar name. Resolve the current project version from Maven and use it consistently so the advertised build helper survives the normal release/version-update workflow.
Useful? React with 👍 / 👎.
An earlier commit here concluded that the canvas ignores the project stylesheet, from a resolved Label colour of f5f8ff where theme.css declares 26324a. That was wrong. The demo project's own stylesheet sets f5f8ff for a Label inside its prefers-color-scheme: dark block, and the builder runs in dark mode, so the canvas was showing the project's CSS correctly all along. The comparison was against the light-mode declaration while the dark one applied. Documents how to read canvas styling so the same mistake is not repeated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d3fe7c737
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <dependency><groupId>com.codenameone.guibuilder</groupId><artifactId>${cn1app.name}-common</artifactId><version>${project.version}</version></dependency> | ||
| <dependency><groupId>com.codenameone.guibuilder</groupId><artifactId>${cn1app.name}-common</artifactId><version>${project.version}</version><classifier>tests</classifier><scope>test</scope></dependency> | ||
| <dependency><groupId>com.codenameone</groupId><artifactId>codenameone-core</artifactId><scope>test</scope></dependency> | ||
| <dependency><groupId>com.codenameone</groupId><artifactId>codenameone-javase</artifactId><scope>test</scope></dependency> |
There was a problem hiding this comment.
When a user runs the normal mvn cn1:guibuilder goal, OpenGuiBuilderMojo.getGuiBuilderClasspath() resolves only the published artifact's transitive dependencies; it does not activate the producer's executable-jar profile. Since codenameone-javase is test-scoped outside that profile, the resolved classpath lacks JavaSEPort, which CodenameOneGUIBuilderStub directly references, so the distributed launcher fails with a missing-class error. Move the JavaSE runtime dependency out of the packaging-only profile or explicitly resolve it in the mojo.
Useful? React with 👍 / 👎.
| String cleaned = value == null ? "component" : value.replaceAll("[^A-Za-z0-9_$]", "_"); | ||
| if (cleaned.length() == 0) return "component"; | ||
| char first = cleaned.charAt(0); | ||
| boolean validStart = first == '_' || first == '$' || first >= 'A' && first <= 'Z' || first >= 'a' && first <= 'z'; | ||
| return validStart ? cleaned : "_" + cleaned; |
There was a problem hiding this comment.
When a component is renamed to a Java keyword such as class, this returns the keyword unchanged and the generated field declaration cannot compile. Distinct accepted GUI names can also collapse to the same identifier—for example a-b and a_b both become a_b—because GuiDocument.renameSelected() enforces uniqueness before this normalization. Reject or escape keywords and ensure uniqueness using the final Java identifier so user-entered names cannot break the companion source.
Useful? React with 👍 / 👎.
check-copyright-headers failed on the PR: 27 added files carried no licence header. All of them now use the complete Codename One GPLv2 + Classpath Exception header, and the check passes locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4238759736
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (document != null && document.isModified()) save(); | ||
| openForm(path); |
There was a problem hiding this comment.
When the user chooses Save in the unsaved-changes prompt but ProjectIO.write() fails (for example on a read-only or full filesystem), save() catches the exception and returns here without reporting failure, after which openForm(path) replaces the modified document. The in-memory edits are therefore discarded immediately after a failed save; make the switch conditional on a successful save.
Useful? React with 👍 / 👎.
| private void refreshProject() { | ||
| guiFiles = ProjectIO.findGuiFiles(binding.guiDir()); | ||
| refreshForms(); | ||
| if (document != null) openForm(document.path()); |
There was a problem hiding this comment.
When the active form has unsaved designer edits, clicking the toolbar's Refresh command—or invoking the MCP refresh command—reopens the same path from disk here without the save/discard prompt used by switchForm(). This replaces the modified GuiDocument and permanently loses those edits, so refresh should refuse or confirm before reloading.
Useful? React with 👍 / 👎.
…stall mvn install reported BUILD SUCCESS repeatedly while leaving the previous jar in the local repository. The freshly built maven/core/target jar contained the change; the installed copy did not. Everything downstream then compiled and tested against stale classes, which is why the same defects were fixed several times over without the running editor ever changing, and why unrelated tests failed at random. The freshly built jars are now copied over the installed ones explicitly, and a fifth symbol is verified in the artifact afterwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4c07f9bc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (Element sibling : componentsIn(parent)) { | ||
| if (sibling == child) continue; | ||
| taken.add(effectiveTableRow(parent, sibling) + ":" + effectiveTableColumn(parent, sibling)); | ||
| } |
There was a problem hiding this comment.
When an existing table child has tableHorizontalSpan or tableVerticalSpan greater than one, this records only its anchor cell as occupied. Adding a palette component can therefore assign it a cell covered by that span; TableLayout then reflows or overlaps constraints, so the preview and generated form no longer represent a genuinely free placement. Mark every cell covered by each sibling's spans before selecting the new child's coordinates.
Useful? React with 👍 / 👎.
| int mask = menuShortcut ? Toolkit.getDefaultToolkit().getMenuShortcutKeyMaskEx() : 0; | ||
| item.setAccelerator(KeyStroke.getKeyStroke(key, mask)); | ||
| } | ||
| item.addActionListener(e -> action.run()); |
There was a problem hiding this comment.
Swing invokes this listener on the AWT event-dispatch thread, but most supplied actions directly call GUI Builder methods that mutate Codename One state and components, including undo/redo, form switching, refresh, editor opening, and selection deletion. The pointer bridge and test timers correctly use Display.callSerially, whereas these menu commands bypass it; with EDT warnings disabled, ordinary menu use can race painting or input and leave the preview hierarchy inconsistent. Queue the action onto the Codename One EDT before running it.
Useful? React with 👍 / 👎.
The editor is launched by resolving its published POM, so codenameone-javase has to be a real dependency of it rather than a test one; a consumer running cn1:guibuilder got a NoClassDefFoundError before the window opened. Generated companion source now carries what the inspector shows. Enabled, visible, RTL, gap, alignment, ticker, toggle, selected, columns, rows, maximum length, editable, grow by content, input constraint, slider bounds and progress, tab placement and selection, and container scrolling were applied to the preview and dropped from the generated form, so the running app used defaults for everything the user had configured. Table cell percentages are emitted for the same reason. A Container or Dialog root generates that type instead of always extending Form, Accordion generates code that compiles, and component names are made unique and keyword safe before they become fields and methods. Companion sources scaffolded by older versions of cn1:create-gui-form carry the legacy "DON'T EDIT" markers. The editor did not recognise them, so designing a form in such a project saved the .gui and produced an empty screen at runtime. Those files are now migrated, keeping the developer's own methods, and the scaffolder writes the current format. Saving writes a temporary file and replaces the target only once the content is safely on disk; a failed write used to leave an empty file where the form was. save() reports failure, so switching forms no longer discards the edits the user just asked to keep, and refreshing the project asks before reloading over unsaved work. MCP clients are told when a save or an open fails. Core: a protected region no longer blocks the first character after it, input method composition is checked against protected regions rather than only replaceRange, multi key mode is restored only by the editor that changed it, and the soft key bypass is limited to components that consume raw text instead of everything that sets handlesInput. On the desktop port a blocked shortcut's release is dropped even when the modifier came up first, and a record left by a shortcut the platform swallowed is cleared by the next plain press. The Maven plugin's artifact resolution now honours offline mode. It reached the network during mvn -o and installed a remote snapshot over the artifact the same build had just produced, which is how the editor kept running without changes that had demonstrably compiled. Also: IDE shortcuts for the new editor in the generated project (IntelliJ, VS Code, Eclipse; NetBeans already had one), Windows path handling in ProjectIO, table spans honoured when allocating a free cell, line breaks and deliberate whitespace preserved through a save/reload round trip, a JSON guard in the MCP client, and the GUI Builder workflow triggers on the core it actually consumes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9287cf7592
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Element parent = acceptsChildren(selected) ? selected : findParent(root, selected); | ||
| if (parent == null) parent = root; | ||
| uniquifyPastedNames(pasted); | ||
| parent.addChild(pasted); |
There was a problem hiding this comment.
When a component in a TableLayout is copied and pasted, the clone retains the original tableRow and tableColumn, and this append does not call the free-cell logic used by addComponent(). The subsequent preview rebuild passes both components' identical constraints to TableLayout.addLayoutComponent(), which throws because the cell is already occupied, while the document remains modified with the conflicting component. Reassign the pasted root to an unoccupied cell before adding it.
Useful? React with 👍 / 👎.
| addArtifact(files, artifact); | ||
| if (result != null && result.getArtifacts() != null) { | ||
| for (Artifact resolved : result.getArtifacts()) addArtifact(files, resolved); | ||
| } | ||
| if (files.isEmpty()) { |
There was a problem hiding this comment.
When the main codenameone-guibuilder JAR is cached but a transitive runtime dependency is missing or fails to download, ArtifactResolutionResult reports the failure, yet addArtifact(files, artifact) makes this list nonempty and the goal proceeds. In detached mode Maven therefore reports a successful launch while the process immediately fails in guibuilder.log with a missing-class error. Check the resolution result for missing artifacts or exceptions before constructing the launch classpath.
Useful? React with 👍 / 👎.
| ArtifactResolutionResult result = repositorySystem.resolve(new ArtifactResolutionRequest() | ||
| .setLocalRepository(localRepository) | ||
| .setRemoteRepositories(new ArrayList<ArtifactRepository>(remoteRepositories)) | ||
| .setResolveTransitively(true) | ||
| .setArtifact(artifact)); |
There was a problem hiding this comment.
When users run mvn -o cn1:guibuilder, Maven documents -o as “Work offline,” but this newly constructed legacy resolution request omits .setOffline(offline). Unlike the requests updated in AbstractCN1Mojo, it may still contact the configured remote repositories or refresh a snapshot instead of using only the local repository.
Useful? React with 👍 / 👎.
| if ("Form".equals(type)) { | ||
| fields.add(propertyField("Title", "title")); | ||
| fields.add(bindingStrategyPicker()); | ||
| } |
There was a problem hiding this comment.
When a form is created with the explicitly supported cn1:create-gui-form -DguiType=Dialog, selecting its root does not satisfy this Form-only check, so the inspector exposes neither its title nor binding strategy; the later identical check also hides toolbar-command editing. The generator already preserves Dialog as the superclass and consumes these attributes, leaving users unable to configure the corresponding dialog features through the builder.
Useful? React with 👍 / 👎.
The guide described the previous editor: an events tab that was disabled, a widget control pad, autosnap and smart-inset checkboxes, GUI files under res/, and "DON'T EDIT" comments. None of that exists in the editor this branch builds, and every screenshot showed the old window. The section now covers the three-column workspace, the palette and hierarchy, the device canvas, the three inspector tabs, auto layout in terms of the controls that are actually there, nested containers, and the built-in CSS and companion-source editors including what the generated and user-code regions mean. The IDE shortcuts the archetype now ships are listed, so a reader does not have to reach for the command line. The screenshots come from a project created with the archetype and a form created with cn1:create-gui-form, not from the test fixtures, so they show what a reader sees on their own first form. Images of the old builder are deleted; the guide's unused-image check would otherwise fail. The .gui example is replaced with one in the format the editor writes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Pushed three commits addressing the review. Summary of what changed and what is still open. P1
P2 addressed
Beyond the review The Maven plugin's artifact resolution ignored offline mode: during The developer guide's GUI builder section described the previous editor; it is rewritten with screenshots taken from a project created with the archetype and Still open Palette-drop validation before mutating a table, a table cell for a displaced BorderLayout child, inline commits against a stale document, cells for pasted table children, dirty state derived from the last saved snapshot, rejecting additions to a full BorderLayout, mirroring overflow and side toolbar placements, prompting before closing an edited buffer or exiting with unsaved work, regenerating generated blocks before saving, action handlers for text inputs and sliders, and the MCP cursor advancing past undelivered events. These are next. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac77fcfd29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private boolean save() { | ||
| if (document == null) return true; | ||
| try { | ||
| ProjectIO.write(document.path(), document.toXml()); |
There was a problem hiding this comment.
When a designer change is saved, this path writes only the .gui XML and marks the document saved; it never calls defaultCompanionSource()/mergeGeneratedSource() or updates the companion Java file. Consequently, a newly added or modified component is absent from the running application unless the user separately opens the Code pane and saves it, and a newly scaffolded form can remain an empty UI despite a successful Save. Regenerate the companion source while preserving its user-code region as part of this save operation.
Useful? React with 👍 / 👎.
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks:
|
The Settings-era GUI Builder is a Swing tool tied to the pre-Maven project
layout. This replaces it with a standalone Codename One desktop app under
scripts/guibuilder, launched bymvn cn1:guibuilder— the same shape ascn1:settingsand the Game Builder: its own Maven build, its own executableJAR, and its own Maven Central coordinates
(
com.codenameone:codenameone-guibuilder).What it does
The editor edits
.guiXML undersrc/main/guibuilderand round-trips thegenerated Java next to it, so the design surface and the source stay in step.
Generated regions of that Java are protected in the embedded code editor rather
than merely regenerated over.
Guided Layout builds on
LayeredLayoutwith builder-owned relationships —match width/height, reference targets, anchors — stored by component name.
That is why the model enforces unique names and cascades renames, deletes, and
pastes across every relationship pointing at them; a stale name is a broken
layout and a duplicate name is a duplicate Java field.
Placement adapters cover Border, Layered, Box, Flow, Grid, and Table layouts.
The whole surface is also drivable over MCP (
-Dguibuilder.mcp.port=…), whichis how the interaction tests replay complete gestures.
Core changes
Kept to the minimum the editor needs:
CodeEditorgains protected-region markers and caret positioning.LayeredLayoutUNIT_BASELINEnow uses a component's reported baselineonly when the component also describes its baseline resize behavior. The
default
Component#getBaselinereturns the bottom content edge rather than atext baseline, so without this the documented font-ascent fallback was
unreachable and containers and text areas aligned on the wrong line.
SplitPaneandTabsno longer assumegetComponentForm()isnon-null. Both can be deinitialized by the same gesture that triggers the
callback, which the builder hits routinely when it rebuilds the inspector.
Tooling
cn1:guibuildernow forwards everyguibuilder.*property, passes the desktopidentity and
--add-exportsarguments the JavaSE runtime needs, and fails witha clear message when Maven runs on a JDK older than 17 (previously an
UnsupportedClassVersionErrorburied inguibuilder.log).scripts/**is excluded from PR CI, so.github/workflows/guibuilder.ymlisadded as the only job that compiles the editor against a freshly built core —
the exact way it can otherwise rot silently. The release workflow gains the
matching Central + R2 publish/confirm steps, wired into the completion gate.
Tests
scripts/guibuilder(JDK 21)core-unittestsCodeEditorTest,LayeredLayoutTest(JDK 8)OpenGuiBuilderMojoTestscripts/guibuilder/STATUS.mdcarries the full design notes, the knownlimitations, and the phased road map.
🤖 Generated with Claude Code