Replace the GUI Builder with a Maven-first Codename One app - #5523
Open
shai-almog wants to merge 57 commits into
Open
Replace the GUI Builder with a Maven-first Codename One app#5523shai-almog wants to merge 57 commits into
shai-almog wants to merge 57 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>
Contributor
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.
![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".
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
Cloudflare Preview
|
Collaborator
Author
|
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>
Contributor
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".
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".
Contributor
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);
Collaborator
Author
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
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>
Contributor
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".
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>
All four are in code from 43c979b; three of them are the same mistake, which is guarding a behaviour in one place and leaving the rest of its lifetime open. The deferred model rewrite was a bare boolean, so accepting it and then switching forms with Discard left it set and the next save of any other form overwrote that form's model with generated content. It is held as the document that asked for it and compared by identity. That rewrite also swallowed its IOException, so a read-only or full disk left the companion on the new strategy, the old model beside it, and Save reporting success. It returns a result now, Save fails on it, and the request is put back so the next save retries. The dragged-span check ran after the reparent and the occupant's reassignment. endTransaction commits rather than rolling back and a false return does not refresh the canvas, so a visibly rejected drop still changed the saved document. It runs before the first mutation, with a comment saying that any future check able to refuse a drop belongs there too. draggedSpanFits bounded the span against the column count and never against the row count, so a vertically spanning component dropped on the bottom row stored a rectangle outside the table. normalizeTableCells grows rows from anchors only and would not have repaired it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
CodenameOne/CodenameOne/src/com/codename1/ui/editor/EditorView.java
Lines 1789 to 1792 in 3ec61d7
Remember shortcut presses until their key releases
When Ctrl/Cmd is released before the shortcut letter, the letter's release observes no active modifier here even though its press was handled as Copy, Cut, Paste, Select All, Undo, or Redo. The release then falls through to the printable-character path and inserts the shortcut letter into the document; track that the corresponding press was consumed rather than inferring it from modifier state at release time.
ℹ️ 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".
The same-kind exemption I added to the discard prompt silenced the question without changing what followed it: the pane was still re-read from disk, so clicking Code, Model or CSS while that pane was open and dirty threw the edit away without asking. keptBuffer() now hands the live buffer back for the pane already on screen, which is what makes skipping the prompt honest -- there is nothing to discard because nothing is re-read. The comment on the exemption says so, since removing one without the other reintroduces the loss. Choosing None after accepting model regeneration left the request armed against the document, so Save regenerated the model under strategy none and replaced the developer's file with an empty disabled-binding class while the status line said only that it was no longer referenced. Selecting None clears the pending request. 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: 42fe057cb8
ℹ️ 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".
The table cell and span fields validated the declared table size and nothing
else, so a coordinate inside the table could still name a cell another child
holds and a span could swallow its neighbour. TableLayout throws on the
duplicate rather than laying it out, which leaves the form unrenderable. Both
now run the same rectangle test the drop path uses -- rectangleFree() was
extracted from draggedSpanFits() so the drag and the inspector enforce one rule
instead of two partial ones.
ensureHandler treated any occurrence of "name(" as the stub already existing. A
handler called buildUI matched the generated zero-argument buildUI() and the
overload the listener needs was never written, and a comment mentioning the
name did the same. It looks for a declaration taking an ActionEvent.
classDeclaration took the first "class " in the file, which in a legacy
companion is usually the licence header or Javadoc saying "this class is". The
wrong class name followed, and the old constructors survived beside the
regenerated ones. It searches a comment-masked copy that preserves offsets, so
the index still refers to the original text.
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: 9f8754c7c5
ℹ️ 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".
I checked the dragged component's span rectangle and left the displaced one
anchored by a single cell, so swapping a spanning occupant into the vacated
cell could put it past the table edge or on top of a neighbour. spanFitsAt()
tests the occupant's own rectangle, firstFreeRectangle() looks for an anchor
that can hold it, and the drop is refused when the table has no room rather
than storing a constraint TableLayout cannot honour.
declaresActionHandler still required the exact text "name(", so a declaration
formatted as "void onClick (ActionEvent event)" was missed and Save appended a
duplicate method, while a commented-out handler still counted and suppressed a
stub that was needed. It masks comments, allows space before the parenthesis
and requires a whole-word match, so onClickHandler no longer satisfies a search
for onClick.
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: 0a8d7d2c33
ℹ️ 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".
The span check I added to assignFreeTableCell sits inside an unbounded search, so a component pasted from a wider table -- one whose horizontal span exceeds this table's column count -- failed every candidate and looped forever on the EDT, freezing the builder. The span is clamped to the destination width before the search and the clamp is stored, so the constraint matches what the table can hold. The relocation search handed back the occupant's own cell: rectangleFree skips the child it is placing, so that cell reads free. Both components then shared an anchor and normalizeTableCells shuffled one away instead of completing the drop. The rectangle the dragged component is taking is excluded from the search. Save wrote the .gui and the companion before deferred model regeneration could fail, so a locked or read-only model left the pair mismatched on disk while Save reported failure. Each write is atomic on its own but the sequence was not; the model goes first, being the output most likely to fail, so a failure leaves every file describing the strategy it already did. A TextField added from the palette now stores the hint the preview shows, for the same reason as the other palette defaults -- saving an untouched field removed the only prompt the canvas displayed. 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: c4f3133107
ℹ️ 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".
…name lands Switching forms cleared the reopen callback and left the buffer, its clean baseline and its kind describing the form that had just been replaced. keptBuffer() matches on kind alone, so opening the same pane on the new form handed back the previous form's text: saving the model wrote it over the new form's model, and saving source replaced the new form's user region. All three are cleared when a form opens, along with any deferred model rewrite, which belonged to the form that asked for it. Undo can take the binding strategy back to what it was. The rewrite authorised for that change stayed armed, so the next save regenerated the developer's model for an edit that no longer existed. The strategy is compared across the undo and the request dropped when it changed. ProjectIO.write deleted the target before renaming the replacement over it, so a rename that failed -- silently, as File.renameTo does -- left nothing but a .cn1tmp the user has no reason to look for. The old content moves aside to .cn1bak and is put back when the rename does not land, so a failed save costs the new text rather than the file. This was the hole left in the atomic-save fix from the start of this branch. The live CSS test waits on a deadline rather than a fixed attempt count: the same work takes 111s on an idle machine and 169s under load, and the previous budget expired before the colour arrived. Failing after the deadline still catches a colour that never applies. 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: 8310f0ebb9
ℹ️ 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".
Writing the model first was not enough, as the review pointed out: the model could land and the .gui or the companion then fail, leaving the project describing two different strategies with no way back. Everything is prepared and every existing file read before anything is written, each write records what the file held, and any failure restores the files already written, newest first. A failed save now changes nothing on disk. A file that did not exist before is left in place rather than deleted on rollback. Restoring content is safe; removing a file this editor may not have created is the more destructive mistake, so a failed save can leave a new model behind with everything else as it was. saveSource returned void, so a companion that could not be written still went on to create the binding model and replaced the error with "Saved form source", telling the user their edit was on disk when the file had not changed. It returns its result and the sequence stops. 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: 5ceb164387
ℹ️ 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".
Undo cancels the pending model rewrite; redo puts the strategy back and left it cancelled, so Save generated the companion for the redone strategy beside a model built for the other one. Redo asks the question again, mirroring the picker. The occupant's destination was still being chosen after moveSelectedToParent had reparented the dragged component, so a drop refused for want of space left that reparent committed by endTransaction. Both decisions are made before anything moves -- which is what the comment already in this method says to do, and which I did not follow when adding the search. Rollback skipped files whose recorded previous content was null, leaving a model created by a failed save on disk under a "nothing was changed" report. I had defended that as deliberate on the grounds that the editor might not have created the file; writeTracked observes its absence immediately before writing it, so the editor did create it and removing it is the restoration. ProjectIO.delete verifies the removal rather than trusting the silent delete(). 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: d165077385
ℹ️ 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".
dropStubsAlreadyWritten still used the exact "name(" search that ensureHandler
had already outgrown, so a legacy method written as "void onClick (ActionEvent
event)" went unrecognised, the generated stub was kept beside it, and the first
save produced two methods with one signature. It calls declaresActionHandler
now -- the same predicate, not a second implementation of it.
The code pane wrote the companion and then created the model it references
with nothing staged, so a failure between the two writes left source committed
against a type that did not exist. It uses the writeTracked/restore pair the
form save uses, so neither output is kept unless both land.
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: 06881da48c
ℹ️ 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".
The CSS buffer was only updated inside the 120ms debounce, so closing the pane within that window left it equal to the file: Close saw a clean editor and asked nothing, then the scheduled callback bailed out because activeCodeEditor had already changed and the edit went with it. The text is mirrored on every change and only the compilation is debounced, from the one listener that was already there. applyProjectCss installs the edited theme as it is typed, so discarding a dirty CSS buffer left the canvas showing styling the user had just rejected and every layout decision after that was taken against a preview the project could not reproduce. discardLivePreviewStyling() reloads theme.css and recompiles, from both Close and the switch-away prompt. Making the files transactional while leaving the visible state stale was half a fix. editorBufferOnDisk moved to the buffer after the companion write but before the model write, so a failed model write rolled the companion back while Close, form switching and exit all believed nothing was unsaved. It moves once every write has landed. 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: 44f2a57ef8
ℹ️ 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".
Switching a form onto a binding strategy with no model wrote that model at
picker time. It was generated from unsaved state, survived abandoning the
change, and made Save skip regeneration afterwards because the file existed --
so anything added or renamed in between never reached it. It is deferred to
Save like a rewrite.
Clicking Code, Model or CSS while that pane was already open ran
refreshEditor(), which restored the pane through activeEditorReopen, and the
method then wrapped that split pane in another one; repeating it halved the
design surface each time. The reopen callback is dropped for the duration of an
explicit open.
removeConstructors still used the exact "name(" search that ensureHandler and
declaresActionHandler had already outgrown, so a reformatted "MyForm (Resources
res)" survived migration beside the regenerated constructors, still calling the
initGuiBuilderComponents that had been removed. It shares the whitespace and
comment handling through constructorAt(). A sweep of the remaining "+ \\"(\\"" in
this file found only source generation, no other matching.
The launch guard was a JVM-wide system property, so in mvnd or an IDE Maven
server it outlived the session and every later invocation returned at the guard
with the run configuration appearing to do nothing. It lives in the plugin
context, which Maven scopes per session.
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: 0a9c96fe13
ℹ️ 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".
keptBuffer() carrying the dirty text forward on a same-pane open was right, but the line after it recorded that text as the on-disk baseline, because the explicit-open path runs with reopeningEditor false. Clicking Code or Model a second time therefore made unsaved edits look saved, and Close or a form switch discarded them without asking -- the loss the same-pane work exists to prevent, one step further down. trackEditorBuffer takes fromDisk and moves the baseline only for content that came from the file; openCss applies the same test on keptCss. The rule is stated rather than inferred from reopeningEditor, which stopped being equivalent once there were three ways to fill the buffer. 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: 4a6b287c54
ℹ️ 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".
…pane declaresActionHandler accepted a receiver-qualified call: "." is not an identifier character, so helper.onClick(new ActionEvent(this)) satisfied the whole-word test and its arguments mentioned ActionEvent, suppressing the stub the generated this::onClick listener needs. It rejects a preceding dot and requires a brace after the parameter list, which is what separates a declaration from a call. This predicate has now been tightened four times, once per dimension -- whitespace, comments, whole word, receiver -- because it was written as string conditions rather than against the shape of a declaration. assignJavaNames guaranteed unique fields, but the @bindable model derives getX/setX by capitalising, so "email" and "Email" are two fields with one pair of accessors and a model that does not compile. Uniqueness claims the accessor stem as well as the field name. A clean Model pane kept its pre-regeneration text after a save that rewrote the model, so its Save button would put the previous strategy's model back while the companion had moved on. A clean pane is refreshed; a dirty one is left alone, because that text is the user's and the discard prompt is what covers it. 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: 7317c311b1
ℹ️ 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".
A palette drop inserts a candidate before the plan is known. Cleaning that up with compensating edits put both the insertion and its deletion on the undo stack, so the next Undo brought back the component the user had just been told was discarded, and a document that had not otherwise been touched came back modified. applyDropPlan returning false was not handled at all, so a component that could not be placed -- a full table with no room for the displaced occupant -- stayed attached to the selection. Both are the same mistake, so the speculative insert now runs inside one transaction that is either committed or abandoned. GuiDocument.abortTransaction restores the snapshot beginTransaction already captures and records nothing. restoreTableRows went with it: hand-undoing the table row growth was the same workaround, and the snapshot covers it. skipSpaces consumed only spaces and tabs while the comment beside it said the newline form was handled, so a handler written with the brace on the next line was missed and Save appended a second method with that signature. It skips whitespace, which is what the comment always claimed. 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: 221ffaf576
ℹ️ 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".
normalizeTableCells reserved only each placed child's anchor, so an unplaced sibling could be assigned a cell inside somebody else's span and the rebuild reflowed both. It reserves the whole rectangle, looks for a gap the unplaced child's own rectangle fits, and clamps a span wider than the table. Reordering swapped anchors without checking either span, and normalizeTableCells accepts an in-range anchor without looking at the rectangle, so moving a two-column child to the last column truncated it at runtime with nothing reporting it. swapTableCells validates both directions and reports a refusal. The reorder has already moved the child in XML order by then, so a refused swap abandons the transaction rather than leaving it reordered in its old cell. declaresActionHandler accepted a legal overload such as "void onClick(String value, ActionEvent event)", which cannot serve as an ActionListener, and suppressed the stub this::onClick needs. This is the fifth time this predicate has been tightened, so it parses the parameter list -- exactly one parameter, type read past qualifiers, generics and modifiers -- rather than gaining another substring condition. 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: 4e9d4225cb
ℹ️ 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".
Comment on lines
+5321
to
+5323
| String existingCompanion = ProjectIO.exists(sourcePath) ? ProjectIO.read(sourcePath) : null; | ||
| String companion = companionSourceFor(existingCompanion); | ||
|
|
There was a problem hiding this comment.
When an older GUI file uses the standard actionEvent="true" representation, its companion contains a handler such as onButton_1ActionEvent, but this code treats the literal value as a method name and generates this::true_ plus an empty true_ stub. Migration preserves the original handler but never invokes it, so buttons silently lose their behavior after the first save; map legacy boolean event attributes to the old component-derived handler name before generating the listener.
Useful? React with 👍 / 👎.
Comment on lines
+5266
to
+5268
| String existingCompanion = ProjectIO.exists(sourcePath) ? ProjectIO.read(sourcePath) : null; | ||
| String companion = companionSourceFor(existingCompanion); | ||
|
|
There was a problem hiding this comment.
Legacy generated companions declare component fields with names such as gui_Button_1, and developer methods outside the generated markers can legally reference those fields. Migration carries those methods unchanged while removing the old generated block, but this generator recreates the field as Button_1; the migrated companion therefore stops compiling whenever preserved user code references a component field. Retain compatible aliases or rewrite those references as part of migration.
Useful? React with 👍 / 👎.
Comment on lines
+6024
to
+6026
| String existingCompanion = ProjectIO.exists(sourcePath) ? ProjectIO.read(sourcePath) : null; | ||
| String companion = companionSourceFor(existingCompanion); | ||
|
|
There was a problem hiding this comment.
For companions created by earlier versions of cn1:create-gui-form, this removes the public Form(Resources) constructor along with the generated constructor body, while the replacement source exposes only the no-argument or model constructor. Any application code that still constructs the form through its previously generated public Resources overload fails to compile immediately after migration; emit a compatibility overload that delegates to the new constructor.
Useful? React with 👍 / 👎.
Comment on lines
+1459
to
+1462
| added = document.addComponent(type); | ||
| if (added == null) { | ||
| ToastBar.showErrorMessage("That container is a BorderLayout and all five regions are taken"); | ||
| setStatus("Nothing added: the drop target is a full BorderLayout"); |
There was a problem hiding this comment.
When the current selection is a full BorderLayout, addComponent() returns null here before the pointer's target is examined. Dragging a palette item onto a different container that has ample room is therefore rejected solely because the unrelated selected container is full; derive the provisional parent from the drop target, or create an unattached candidate before planning the drop.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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