Native desktop windows - #5556
Open
shai-almog wants to merge 284 commits into
Open
Conversation
![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
Dispatch key events to the window's focused component
When f is a Window, this invokes the inherited Container.keyPressed(), because Window does not override the key handlers. That implementation only forwards to a container lead component, so ordinary focused controls receive no physical-key input, focus traversal never runs, and the listeners stored by Window.addKeyListener() are never fired. Window needs form-equivalent key pressed, released, repeated, and long-press dispatch.
ℹ️ 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 12 screenshots: 12 matched. |
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
Contributor
Cloudflare Preview
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 338ee1a6f1
ℹ️ 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
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13311a9bed
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1670ca0579
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bea352c1ee
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7832a8ae0
ℹ️ 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 144 screenshots: 144 matched. |
showModal() from a background thread returned before the window appeared. show() only queues its work off the event dispatch thread, so the wait saw a window that was not visible yet, concluded the modal was already over and returned; the queued show() then acquired modality afterwards. The show is awaited when the caller is not on the event dispatch thread. Changing the owner of a window that already exists silently did nothing useful and stranded its modal blocker: native ownership is fixed at creation on every platform -- the owner HWND, the transient parent, the JDialog's owner -- and release derives the blocked window from the field, so it would have enabled the new owner and left the old one disabled. It throws now rather than pretending. The simulated pinch drag on JavaSE went to the main form even though the press that started the gesture was tagged with the window, so a right button or shift drag over a window dragged unrelated main form content. Tooltips cannot work inside a window: TooltipManager schedules only when getComponentForm() is non-null and displays through an InteractionDialog on the current form. The half-wired call is gone and they are listed with the other form-coupled overlays in the guide, rather than appearing to work and silently doing nothing. 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: bc45217f4f
ℹ️ 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".
Minimizing a modal window ended its modality. hideNotify clears nativeVisible, which showModal read as "the modal is over": the wait returned and the blocker was dropped, so restoring the still-open window left a modal on screen with input flowing to the windows behind it. Iconification is tracked separately from an explicit hide now. A native editor opened in a desktop window was removed from the primary canvas, which is not its parent, so the removal did nothing: the editor stayed on screen swallowing input and every further edit stacked another one on top. Both teardown paths remove from the parent the editor actually has, and the legacy AWT editor path now attaches to the owning window's canvas too rather than always to the primary one. hide() ran on the calling thread, unlike show() and dispose(), so hiding from a background thread raced the event dispatch thread while it painted or dispatched input. It marshals now, which is what the guide's threading section already promised. 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: fc3370ca1b
ℹ️ 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 217 screenshots: 217 matched. |
Mac Catalyst pointer coordinates arrived unscaled. UIKit reports a touch location in points while the window is laid out in device pixels, so on a Retina display every press, drag and release landed at half its rendered position and only the top left of a window could be clicked where it looked like it was. A Catalyst secondary window received no hardware keyboard input at all. Its scene is rooted at the window's own controller, and only the main view controller implemented the UIKit press handlers, so nothing reached the focused component. The window controller implements them now and shares the main controller's UIKey mapping rather than duplicating a hundred-case switch that would drift. A CEF or browser peer inside a JavaSE window was inert: attaching and positioning were fixed earlier, but CN1JPanel still hit tested, converted coordinates and forwarded events against the primary canvas, which the peer is no longer inside. It resolves its own window's canvas from its Swing ancestry now, cached and dropped on re-parenting since these run for every mouse move. Linux secondary windows sent the raw GDK keyval as the key code. GDK encodes many Unicode keyvals differently from their code point, so any non-ASCII key produced the wrong character; the main window's gdk_keyval_to_unicode conversion is mirrored. 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: 259c862b4b
ℹ️ 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 multi-pointer drag reached Component's version rather than the window's, so a pinch was tested against the window itself and then collapsed to a single coordinate: the pressed child got an ordinary one-finger drag and never its pinch callbacks. Moving a window within one monitor reported nothing. Every port only raised the monitor-changed event, so the documented Moved event never fired for an ordinary move and nothing could persist a window position. All three desktop ports raise it now. Releasing modality on JavaSE cleared an always-on-top the application had asked for, because both went through the same frame property. The window kept claiming it was always on top and never reapplied it, since the native window already existed. The application's setting and the temporary elevation a modal gets are tracked separately. The fake window manager handed back a marker object for a window's graphics, so flushing the event dispatch thread with a window open threw rather than painting -- no existing test had pumped the loop with one open. It returns real graphics sized to the window now. 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: a128cc7ff2
ℹ️ 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 negative coordinate meant "no position given", but a monitor left of or above the primary display has a negative origin, so a window restored onto one was centred on the primary display instead. All three ports take an explicit flag now and honour the coordinate whatever its sign. Minimizing on native Windows arrived only as a resize to zero, and Linux had no iconification handler at all, so the framework went on treating a minimized window as displayed: still painted, and an animation in it still keeping the event dispatch thread awake. Both report hidden and shown now, which is what the JavaSE port already did. hide() skipped everything when the window was iconified, because that also clears nativeVisible. The native window stayed alive, the modal blocker stayed registered and showModal stayed parked even though the application had explicitly asked to hide it. A null parent peer meant two different things -- no owner, or an owner that is the main form, which has no peer -- and ports could not tell them apart: JavaSE ignored a form owner while Windows and Linux made even unowned windows children of the main one. The flag is explicit now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Produced by the cross-compiled Windows producer job and inspected rather than taken on trust. Every one comes out at exactly the requested size minus the platform's chrome -- 400x300 renders at 384x261, 900x700 at 884x661 -- which is the contract the harness checks, and the content is real: the layout case shows every widget, the scroll case its list and scrollbar, the overlay case its layer over the base content, and the modal case a background window that is still painting while a modal is up. The mappings needed globs. They are matched with fnmatch, so a bare "Window-Layout" pattern matched nothing once the size suffix was appended and every golden reported as unmapped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Produced by the Linux producer job and inspected, not taken on trust. Unlike Windows these come out at exactly the requested size, because the GTK drawing area fills the window rather than sitting inside native chrome, and the content is real: every widget in the layout case, the list and its scrollbar in the scroll case, and a background window that is still painting while a modal is up. x64 and arm64 are byte identical, which is worth noting on its own -- the port renders the same on both architectures -- but they are stored in the two directories the workflow compares against separately, as every other Linux baseline is. The musl variant is not compared and needs none. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
Form implements TopLevelContainer now, so 43 of its methods override an interface method and the MissingOverride rule wants them annotated. The volatile rule has no exception list, so the two volatile fields are gone rather than suppressed: disposed is published and read under Display.lock, which is the monitor showModal already parks on, and paintedOnce is written and read only on the event dispatch thread. Two index loops are deliberate -- a window can be disposed part way through a nested event loop, and an animation can deregister itself while it is being iterated -- and are marked, matching the existing NOPMD in Form.loopAnimations. The owned-window loop had no such reason and is a foreach now. 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: 47e0aa010f
ℹ️ 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 163 screenshots: 163 matched. Benchmark ResultsDetailed Performance Metrics
|
…roll over Two gaps against the Form path, both in Window.pointerPressed. The press handle was created after the window's pointer-pressed listeners ran. A listener can enter a nested event loop -- showModal() does -- and the matching physical release is then processed inside it. With the handle created afterwards that nested release found no gesture to clear, and the method went on to install a fresh press whose release had already happened, leaving the component latched until some later gesture freed it. Form creates its handle before firing listeners; so does the framework's own press record, for exactly this reason. Moved. A press landing on a still-gliding container stopped the motion, cleared pressedCmp and returned. Stopping the glide is right, but it was only half of what Form does: Form re-enters the drag path so the same physical gesture takes the scroll over, while here every following drag packet had no target and the user had to lift and press again. The press now cancels the glide, primes drag and drop, and re-enters through this window's drag path -- not Display.pointerDragged(), which is the main surface's and would deliver to the current Form instead. It hands pressedCmp to the component the scroll was taken over from. Form reaches the same place differently, by re-resolving the component under the pointer whenever it has no pressed one; giving this window's drag path that same fallback was tried and rejected -- it changed routing for every gesture and broke five unrelated tests. While here, drag events now carry setPointerPressedDuringDrag as Form's do, read and cleared in the scalar path and reported without clearing in the multi-pointer one, matching Form on both counts. Both tests fail without their own fix. Each disposes its window in a finally, because an assertion that throws before dispose leaves a window showing and times out the next test's setup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Window.capture() falls back to re-rendering the component hierarchy when the window manager returns no pixels. That fallback produces a plausible image of the right size, so a capture path that never reads the real surface looks exactly like one that does -- which is how the missing Windows override went unnoticed in the first place. The Windows manager now says so, once per process, when the native capture comes back empty. Without it the only way to tell a live readback from a silent fallback is to find a pixel that differs, and the windowed goldens are byte-identical either way: the harness stops editing before capturing, so no native editor is in frame to give it away. 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: 150a2f2890
ℹ️ 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".
setWindowLocation already carries a comment about this trap: the read has to happen on the event dispatch thread with the write, because setWindowBounds marshals itself and reading beforehand queues a move carrying the old size. centerOnDesktop(), centerOn() and restore() had the same shape and were missed. Both centring methods read the window's bounds, compute a position and only then call something that marshals. From a background thread that computes against geometry a queued resize is about to replace, so a caller that resized and then centred got a window centred for the size it no longer has. restore() is an ordering problem rather than an arithmetic one. showOwnerChain() may queue the owner's show(), while the native restore ran immediately -- so the child could reach the platform ahead of its owner, and a WindowManager call happened off the event dispatch thread, which is the only context that SPI is defined in. All three now marshal the whole method. Checked the rest of the class by enumerating every public method that touches the window manager or reads bounds rather than grepping for the pattern: setTitle, setResizable, setDecorated, setAlwaysOnTop, setUtilityWindow, setWindowIcon, setMinimumWindowSize, minimize, toggleMaximize and requestWindowFocus are each a single call with no read to go stale, and every port marshals internally, so they are left alone deliberately. Both tests fail without their own fix: the centring one is out by exactly half the size difference, and the restore one sees the port called before the queue drains. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reverts the Catalyst halves of c1e1a76 (outer-bounds contract) and 1c5ee65 (raster release). The Linux half of the bounds work is untouched. The mac-native suite went from 162/162 to 148/162 on this branch, and not by the golden churn I predicted. The app died of an uncaught NSException during WindowScrollTest, so the eleven windowed tests after it never ran at all: -[MTLRenderPassColorAttachmentDescriptorArrayInternal mtlMutableTexture]: unrecognized selector sent to instance 0x600001d421c0 at CN1MetalBeginMutableImageDraw + 40 from -[CodenameOne_GLViewController drawFrame:allowInactive:] That is a use-after-free wearing a wrong-selector costume: the GLUIImage had been freed and its memory recycled into a Metal descriptor array. The freed image is the window raster released in 1c5ee65. The reasoning behind that release was wrong in a specific way worth recording: I checked that capture() copies rather than retaining the live raster, and that the paint loop re-fetches the graphics each frame, and concluded nothing else held it. Both are true and neither is sufficient -- Catalyst defers drawing through queued ExecutableOps, and CodenameOne_GLViewController passes their opTarget straight to CN1MetalBeginMutableImageDraw. An op queued before the release still names the raster, so freeing it early is exactly the unretained-op-target crash this port has hit before. Doing it safely means draining or retaining through that queue, which is why the finalizer was left to do the freeing. The bounds half goes back with it for a different reason: it had no effect. Window-Layout still matched its stored golden pixel for pixel, so the captured content was the full requested size and the outer-frame request never reached the window. Keeping an inert change that ships alongside a crash helps nobody. So Catalyst keeps the contract it had: setWindowBounds sizes the content rather than the frame, which still disagrees with Windows. The Linux fix stands, and the review thread stays open, because two of the three ports still do not agree. 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: ce37e11785
ℹ️ 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".
Hiding an owner cascades to the windows it owns, and every port reports those children through windowHideNotify(). That is the minimize path, so the child keeps its modal registration deliberately -- hideNotify() cannot tell an owner cascade from a minimize, and a minimized modal is still open and still modal. But isBlockedByModal() consulted every registered modal without ever asking whether it was reachable, so an application modal went on blocking the main surface and every unrelated window while being on nobody's screen, with nothing available to dismiss it, until the owner was shown again. Two halves, and the test needs both. isBlockedByModal() now skips a modal with an owner that is not showing. The modal's own visibility is deliberately not consulted: a minimized modal must keep blocking, which is what stops a modal quietly releasing the application. An owner that is not showing is different -- its children went with it and cannot be restored on their own. And syncNativeModalBlocking() now runs on the SHOWN and HIDDEN callbacks. It was called only from push, pop and dispose, so the ports were never told the block had changed: the framework would have stopped blocking while the platform kept the main surface disabled. The test asserts through an unrelated window's peer rather than a private predicate, because what an application modal costs when it will not let go is every other surface. It fails with either half removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…splay Display had grown 27 arrays, all of them parallel tables keyed by window id, with three separate slot allocators handing entries out and taking them back. That was the wrong shape, and it was not a theoretical problem: two of the defects found in review last round were caused by it rather than found by it. The drag-activation filter silently switched off for the whole application once eight windows had been disposed mid-press without returning their slots, and the pressed-selection bug was "fixed" by adding three more arrays. State that belongs to a window now lives on the window, as fields: drag path, drag-occurred, pressed-selection and its coordinates, key repeat and long key press timers, long pointer press timer Display keeps exactly what it kept before this branch for the main surface -- its own dragPathX/Y/Time ring, keyRepeatCharged, longPressCharged, longPointerCharged, keyRepeatValue, nextKeyRepeatEvent, longKeyPressTime -- so window zero is the special case it always was, on the code it always ran. The event loop services those the way master does, then walks the windows that are actually open rather than a fixed table mostly full of empty entries. Removed: 17 arrays, dragHistorySlot(), keyRepeatSlot(), the inline allocator in chargeLongPress(), and releaseDragHistory() with its three call sites -- including the one guarded against a nested invokeAndBlock, which only had to exist because the ring was shared. PointerDragHistory is the one piece worth sharing: the sample ring and its wrap arithmetic. Display owns one, each Window owns one. Everything else is a field, and a field on the right object needs no sharing. Two tests were rewritten rather than repaired. dragHistorySlotsAreReclaimed- AfterEachGesture reflected into the slot table to prove slots came back; there are no slots, so it now asserts the behaviour that mattered -- a window opened after ten others have gestured can still drag. The key-repeat and long-press helpers likewise ask the surface that owns the timer instead of reading a table out of Display. 5372 core tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tion Two of the things that made this branch spread. A Window had a title area and a Toolbar, which is the mobile title bar standing in for chrome the platform already draws -- so a window carried two titles and gave up content space to the one nobody asked for. TopLevelContainer no longer declares getTitleArea(), getToolbar() or setToolbar(); a Window's title goes straight to the platform, and Toolbar and its search bar are Form-only again. That alone reverts Toolbar.java to master exactly: 285 lines and 26 of the getTopLevelContainer() call sites, gone because the question does not arise. The other is the pattern the sweep kept repeating. Registering a component for animation had turned into "resolve the top level, null-check it, remember it in a private field so deregistering can use the same one" -- twelve lines and a field, replacing a single getComponentForm().registerAnimated(this). It was written out again in each class that needed it, and the field was there because of a real defect: register against one top level, get moved, deregister resolves another, and the first goes on animating a component that left it. Component.registerForAnimation() and deregisterFromAnimation() hold that. The component remembers what it registered with and deregisters from the same thing, so the defect is fixed once instead of a dozen times, and the call sites are shorter than they were before this branch. ImageViewer's three of them go from twelve lines each to one. Nine tests went with the toolbar: eight exercised toolbars and side menus inside a Window, which is no longer a thing. The ninth is about animation registration in a window, which still is, so it stays with the toolbar part removed. One test changed for a real behaviour change rather than a fix: aPressDraggedOutOfAButtonInAWindowIsCancelled released at (2,2) as "outside the button", which only worked while a title area occupied the top of the window. With the content pane filling the window a centred button covers every in-window point, so it now releases outside the window. 5364 core tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…pers Ten more sites that had each written out "resolve the top level, null-check it, register this" -- and in two cases the matching resolve-and-deregister -- now call registerForAnimation() and deregisterFromAnimation(). InfiniteProgress, ScaleImageButton, ScaleImageLabel, Button, Label, TextField and Component. Worth recording how the sweep went wrong, because the pattern-match nearly repeated the mistake it was cleaning up: the substitution also consumed the declaration it matched, and in TextField.deinitialize() that variable had a second use further down. The compiler caught it; nothing else in the batch had the same shape. Enumerating would have been safer than matching, again. 5364 core tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Modality is a question about windows, and the window registry is on Desktop. Display was holding the modal list, the push/pop, the native blocking sync and the four predicates that decide what a given modal blocks -- 153 lines about a concept the main surface has no stake in. They move to Desktop. Display now asks one question, isWindowInputBlocked(id), and does not need to know what a modal is; Window pushes and pops its own registration directly rather than through Display. Display's diff against master is down from +2249 to +1968 lines with this and the earlier state moves. 5364 core tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…o Desktop
Display is already a god class; the window work was making it worse. Moving
out what is genuinely about windows rather than about the display:
window lifecycle shown, hidden, focus, moved, resized, monitor changed,
close requested, closed natively, activation failed,
monitorsChanged and the WindowCallback that carries them,
plus the pending-size table and the coalescing guard
geometry windowWidth, windowHeight, windowDragRegionStatus
input entry the pointer and key events a port reports for a window
All of it resolves an id to a window, and the registry that does that is on
Desktop. Display's diff against master drops from +2249 to +1504 lines, and
the ports now report window events to Desktop instead of to Display.
Some of it deliberately stayed. windowInputCancelled and windowDisposed tear
down input state Display owns. windowMouseWheelEvent, windowMagnifyGesture
and windowRotationGesture carry the shared implementation the main surface
uses as well, so moving them would have duplicated it. windowKeyReleased and
the two hover-press entry points only pack an event and put it on the queue,
which is Display's actual job -- moving those would have meant widening the
queue's internals to the package to buy nothing.
pointerReleasedImpl now cancels the long-press timer itself rather than
relying on its callers to, since a release ends the gesture either way.
5364 core tests pass; core, javase, windows, linux and ios all compile.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
createWindowGraphics, the paint pass over the open windows and the "does any window still animate" check were all "walk the windows and do something", which is Desktop's job. Display's event loop keeps the loop and calls Desktop once per pass for the window half. repaintTopLevels splits the same way: the current Form is Display's to repaint, the windows are Desktop's. wakeEdt stays -- it notifies Display's own lock. Display's diff against master is now +1459 lines, down from +2249 when this round started, and what remains is the input queue, the main surface's own state and the shared implementations window zero uses too. 5364 core tests pass; core, javase, windows, linux and ios all compile through the reactor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SpotBugs is a zero-findings gate and caught five leftovers from moving the window API out of Display: a local that lost its last use when windowWidth moved to Desktop, and four private methods -- isSelectionPressed, longPressKey, repeatTarget, selectionX and selectionY -- whose only callers went with the code that moved. This is the case CLAUDE.md warns about: removing a caller can make a private method dead, and the gate fails on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five iOS jobs -- build-ios, build-ios-watch, build-ios-metal, native-ios and packaging -- were failing on the same six errors: call to undeclared function 'com_codename1_impl_ios_IOSWearableCallbacks_nativeMessageReceived___...'; ISO C99 and later do not support implicit function declarations Nothing about them had changed. The WatchConnectivity block in IOSNative.m calls six translated static methods on IOSWearableCallbacks, ParparVM emits their definitions, and the file includes no header that declares them -- so every call was an implicit declaration. C99 dropped those, and this only compiled while the toolchain treated it as a warning. The same commit passed this job at 17:42 and failed it at 00:07 with no relevant change in between, which is what a runner image moving to a stricter clang looks like. Declared explicitly, inside the same CN1_USE_WATCHCONNECTIVITY guard as the calls. check-native-signatures.sh reports no MISSING or SIGNATURE findings for the ios port. Not caused by this branch -- the wearable code came from master in #5487 -- but it blocks it, so it is fixed here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Moving the key-repeat and long-press timers onto the windows dropped a check the old routing helper made. repeatTarget() returned null for a surface that a modal was blocking, so a key held down before the modal appeared stopped repeating into the window behind it. serviceInputTimers() checked only that the window was visible, so those repeats started getting through again, and the main surface had lost the same guard. Both now consult Desktop.isWindowInputBlocked() before firing. Found by refusing to delete a test. SpotBugs reported repeatTarget() as an uncalled private method once its production callers moved to Desktop, and I deleted it -- but a test reached it by reflection, which SpotBugs cannot see, so the build went from a clean gate to a NoSuchMethodException that killed the event dispatch thread and left every later test timing out at five seconds a piece. Reflecting into privates is what made that possible, so the test is rewritten against behaviour: it holds a key, drives the timers with an explicit clock rather than waiting out the 800ms first-repeat delay, and asserts the repeats stop when a modal goes up. It fails without the fix. 5364 core tests pass; SpotBugs, PMD and Checkstyle report nothing. 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: 4b1456cef9
ℹ️ 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".
… goldens build-test (8) failed on seventeen forbidden PMD violations, all of them residue from moving the window API to Desktop: nine index loops over the Window[] getWindows() returns ForLoopCanBeForeach MAIN_LONG_PRESS_ID, orphaned by the slot removal UnusedPrivateField WindowManager import, orphaned by the modality move UnnecessaryImport TopLevelContainer import in ScaleImageButton UnnecessaryImport five fully qualified Desktop.getInstance() calls UnnecessaryFullyQualifiedName Worth recording why local verification missed them: PMD is enforced by .github/scripts/generate-quality-report.py against its own forbidden list, not by the Maven build, so `mvn verify` passes with violations present. Reading maven/core-unittests/target/pmd.xml is the local equivalent, and it now reports zero -- as do SpotBugs and Checkstyle. The Linux windowed goldens are re-recorded, both arches. Taking the title area out of Window changed exactly what it should: the in-content title label is gone and the content starts at the top of the window instead of below it. The window dimensions are unchanged, and the fourteen windowed captures were the only ones that moved -- nothing else in the suite differs. Checked by looking at the images side by side rather than by trusting the dimensions. Windows and Mac Catalyst carry the same fourteen and will need the same treatment once their runs report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fix itself went in with a14ff33, whose subject is about PMD and goldens and says nothing about it -- my mistake, and worth stating rather than quietly leaving the history misleading. The defect: the material pull-to-refresh drag listener is built once and kept for the life of the component, capturing the top level it was created in. A component moved to another Form or Window is re-registered on the new one but the listener still targets the old, so the overlay goes up on the top level the component left and the release arriving on the new one finds nothing to finish -- the refresh task never runs. The host is now resolved when the drag happens instead of captured when the listener is built. Pre-existing: master captures `final Form p` in exactly the same place. This branch only widened where it bites, by letting a component move between a Form and a Window. The test moves a scrollable pull-to-refresh container from a Form into a Window, pulls, and asserts the overlay lands on the window and nothing is added to the form. It fails with the captured host. The container is given content that overflows on purpose -- the gesture is gated on the container actually being scrollable, and a first attempt with a single label produced no overlay either way, which would have passed for the wrong reason. 147 WindowTest tests pass. 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: 0b0be1a5cb
ℹ️ 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 array and dict tags were made structural earlier in this branch; the key tags were left matching "</key>" as a literal, so a fragment writing "</key >" -- which is valid XML -- reported the key as absent. Both callers then fail, in opposite directions. plistKeyIndex tells the injection path there is no UIApplicationSceneManifest, so it appends a second one beside the application's own and the bundle ships duplicate keys. plistKeyEnd loses the key's value, so the validation path rejects a Mac Catalyst build that is correctly configured. Both now use plistCloseElementIndex, the closing-tag counterpart already written for the container tags. Two tests, and both fail against the literal matching: one on a single key with a spaced closing tag, one on a whole scene manifest where every closing tag is spaced. 912 plugin tests pass; SpotBugs zero. 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: 6c4d17bba7
ℹ️ 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".
…restore AWT delivers componentShown / componentHidden for every visibility change, whoever caused it, and the port turned those into windowShowNotify / windowHideNotify. The existing comment claimed that was safe for the explicit path because Window.hide() and show() set nativeVisible before calling the manager, so the notification would find the state already correct. That holds only if the notification is delivered inline, and it is not: the AWT callback runs on the AWT thread and queues onto the Codename One event dispatch thread. A show and a hide in the same turn therefore both queue, and both run afterwards against the state the second one left. The pair reads as a minimize followed by a restore, and in the show-then-hide order the window finishes hidden while still marked iconified -- which is the state showModal() waits on, so its caller waits for good. The port now counts the visibility events it is about to cause, on the AWT thread where they are also delivered, and the listeners consume that count instead of reporting a lifecycle change. Counted only when the frame is really changing state, since AWT delivers nothing when it is not and the count would otherwise be spent on a later event the user caused. The test drives show() then hide() in one turn and asserts no Minimized or Restored is reported. Without the correlation the window reports: [Resized, Shown, Hidden, Moved, Resized, Restored, Minimized] 278 JavaSE port tests pass. 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: 5a53552596
ℹ️ 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".
… sweep Two findings, both mine. edtLoopImpl takes an early return while the main form is running a transition, which is right for the main surface and wrong for the windows: a secondary window is an independent native window with no part in that transition, and it stopped painting and animating for the transition's duration. The new test shows it painting zero times either side of one. The window pass now runs before that return. Input deliberately stays queued there rather than being dispatched. That is how the main surface already behaves during a transition, and draining the shared queue from that branch would change the main path's semantics to fix a window's -- the wrong trade for a few hundred milliseconds of deferred input. The other is the third report of one bug class: a closing tag matched as a literal. plistStringValueAfter searched for "</string>", so a delegate written "</string >" looked absent and aborted a correctly configured Catalyst build. Rather than fix the reported line alone, every remaining literal closing tag in the file was enumerated -- which turned up a second one, mergeUserActivityTypes parsing "<array>" and "</array>" out of the injected fragment. Both now use the structural lookups. The one literal left reads our own generated Info.plist line by line, where we control the spelling. Enumerating should have happened when the first of these was fixed; matching the reported pattern is what let it come back twice. 5366 core tests and 92 IPhoneBuilder tests pass. 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: 8a785b1283
ℹ️ 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".
minimize(), toggleMaximize() and requestWindowFocus() called the window manager straight from the calling thread, unlike show(), hide(), restore() and dispose(). setResizable() and setAlwaysOnTop() did the same with their native half. I had looked at exactly these methods when centerOn() and restore() were marshalled, and left them alone on the grounds that each was a single call with no read to go stale and that the ports marshal internally. The second half of that was wrong. WindowsWindowManager.minimize() resolves the peer to a slot index on whatever thread calls it and hands that index to the native layer -- so a background caller can read a slot an EDT disposal is tearing down, and the same shape appears in setTitle, setResizable, setAlwaysOnTop, setMinimumSize, requestFocus and toggleMaximize. The developer guide also promises callers this is marshalled for them. The three commands marshal whole. The two setters write their field on the calling thread, so a getter stays consistent, and marshal only the SPI call. TestWindowManager now records any window-manager call that arrives off the event dispatch thread, which is the property worth asserting -- not whether the call eventually happened. The test drives all three controls from a background thread and expects an empty list; without the fix it names the offender: expected <[]> but was <[minimize]>. 5367 core tests pass. 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: 1c1298262d
ℹ️ 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".
Window.getMonitor() answers from a lazy cache, and moving the window is exactly what invalidates it -- but moved() fired the Moved event without touching it. The cache is otherwise refreshed only by monitorChanged(), which both the JavaSE componentMoved path and the Windows WM_MOVE path enqueue *after* Moved, so a Moved listener asking getMonitor(), getScale() or getDensity() was told which monitor the window had been on before it moved, and nothing later told the application to ask again. Cleared rather than recomputed, so a move nobody asks about costs nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same 14 that shifted on Linux, for the same reason: a Window no longer paints a title area into its own content, since the title belongs to the native chrome the capture does not include. Content moves up by the height of that strip and is otherwise unchanged. The x64 and cross-compiled runs produce these byte-for-byte identically, so one golden set covers both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Codename One has no windowing API. Even on JavaSE, Mac, Win32 and Linux, where the OS has real windows, an app gets exactly one, welded to a single global "current Form":
CodenameOneImplementationholds onecurrentForm,Display.edtLoopImplpaints one surface per tick,paintDirtyuses one global paint queue clipped togetDisplayWidth()/getDisplayHeight(), andhandleEventroutes every input event to one form. Everything that looks like a second window today —Sheet,InteractionDialog,ToastBar,Dialog— is an overlay inside the current form's layered panes.This adds real native windows, each rendering its own component tree, on all four desktop targets, without changing the single-form model mobile depends on.
API
TopLevelContaineris the shared contractFormandWindowboth implement. Its members were chosen by counting actualgetComponentForm().<method>()chains inCodenameOne/src, and every one of them was already public onFormwith an identical signature, soFormneeded nothing beyond theimplementsclause andasContainer()— a Java interface cannot extend a class, so without that bridge aTopLevelContainerreference cannot go anywhere aComponentis wanted.Window extends Container implements TopLevelContainer. Inside a windowgetComponentForm()returnsnull, by design;Component.getTopLevelContainer()is the new resolution API, and core now uses it internally.DesktopandMonitorare the public parallel toDisplayfor "what screens exist and what windows are open", including per-monitor DPI and backing scale;Displaykeeps meaning "the main app surface" exactly as before.Modality is enforced in core rather than per port, so it behaves identically everywhere:
Displaykeeps a modal stack andhandleEventdrops input to blocked windows.showModal()parks the caller throughinvokeAndBlockthe wayDialogalready does, which re-enters the event loop — so every other window stays live and repainting while a modal is up.Implementation
The impl SPI is a single
WindowManagerfacade returned fromCodenameOneImplementation.getWindowManager(). Returningnullis the capability query, so there is no separateisMultiWindowSupported()that could drift from it. Only genuinely universal operations are abstract; anything a port might not offer has a no-op default, so adding a capability later never breaks a port.Per-window paint state moves into a
PaintSurfacevalue object with the main window as instance zero;getCodenameOneGraphics(),repaint(Animation),cancelRepaintandhasPendingPaints()keep their signatures, so every existing port still compiles and behaves.paintDirty()'s body is parameterized rather than globally rebound — a global "active surface" was rejected becauseDisplay.getDisplayWidth()is public and callable off the EDT, so a live binding would change its answer re-entrantly across ~210 call sites.Events pack the window id into the type word (
type | (windowId << 8)). Window 0 is numerically identical to the previous wire format, so drag coalescing and the stack-swap logic are untouched. The port is handed the id at creation and echoes it back, so there is no peer-to-window map on the off-EDT input path.Ports: JavaSE (per-canvas graphics de-singletonization —
getNativeGraphicsused to return one shared instance, andisScreenGraphicswas an identity check against one buffer, so a second window would have drawn into the first window's pixels), native Windows (Direct2D per-window render targets,GWLP_USERDATAidentity,WM_DPICHANGED), native Linux (per-window cairo back buffer, GTK closure data), and Mac Catalyst (UIWindowSceneper window). Peer components and native text editing work in every window on every one of the four. iOS, Android and JavaScript need no port changes at all: they inherit the false capability and the throw lives in core.Latent bug fixed on the way
handleEventreturnedoffsetunchanged when the form was null, while the caller loopswhile (offset < actualTmpPointer)— an infinite EDT spin. It is unreachable today only because all nine entry points guard ongetCurrentForm() != null; window disposal with events in flight makes it reachable. It is now askipEventthat drains the packet so the rest of the batch still dispatches.Testing
Core unit tests drive a scriptable fake
WindowManageronTestCodenameOneImplementation— settable, defaulting to null, so the unsupported path is the default — covering lifecycle, paint isolation, event routing, modality including a modal window nested in a modal dialog, theTopLevelContainercontract, and a fake multi-monitor table at mixed DPI. JavaSE port tests cover the per-canvas graphics resolution, which is the riskiest edit here and had no coverage before.The centrepiece is a windowed screenshot family in
scripts/hellocodenameone: representative UI re-run inside a real window at several sizes and compared against its own goldens. A picture of a window proves nothing; layout, scrolling, graphics, layered overlays, native editing and modality rendering correctly on a non-primary surface is the actual claim. The three sizes, including a deliberately non-square one, are what prove content lays out to the window rather than toDisplay.getDisplayWidth(). This needed per-window capture on every port, since the existing pipeline can only see the main framebuffer.Mac Catalyst was built and run on real hardware for this branch rather than left to CI, because it is the hardest of the four. That found four defects compiling never would have:
capture()was unimplemented; the readiness probe was a false positive; captures were taken before the first paint; and the scene was never asked for the geometry the window was created with, so several captures came out at the main display size with the window's content in the corner.Known scope limits, documented
HTMLComponent, accessibility on secondary windows,Dialog.show()from inside a window and form transitions into or out of one are out of scope for v1 and called out in the guide.Display.getDisplayWidth()/getDisplayHeight()keep reporting the main window; components inside a window use their top level's size.🤖 Generated with Claude Code