On Wed, 7 Oct 2026 14:55:59 GMT, Florian Kirmaier <[email protected]> wrote:
>> Florian Kirmaier has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8388313: PR feedback1
>
> These are the checks usually done before the owner properties are set in
> PopupWindow:
>
>
> public void show(Node ownerNode, double anchorX, double anchorY) {
> Toolkit.getToolkit().checkFxUserThread();
> if (ownerNode == null) {
> throw new NullPointerException("The owner node must not be null");
> }
>
> final Scene ownerNodeScene = ownerNode.getScene();
> if ((ownerNodeScene == null)
> || (ownerNodeScene.getWindow() == null)) {
> throw new IllegalArgumentException(
> "The owner node needs to be associated with a window");
> }
>
> final Window newOwnerWindow = ownerNodeScene.getWindow();
> validateOwnerWindow(newOwnerWindow);
>
>
> To call `PopupWindow.validateOwnerWindow`, I could make it public with the
> accessor pattern.
>
> Should I just add these checks to the ContextMenu.show method,
> or leave it as is?
>
> We could also make a reusable method in PopupWindow, which can be used more
> freely to check and prepare the Window for an anchor node. Maybe even public,
> because it's a valid use case to use the size of a Popup to decide its
> position.
>
>
>
> Some words on how often the bug appears and with which symptoms.
> The motivation for the fix was never performance and always correctness.
> I'm seeing it quite often now, with different symptoms and in different
> projects
> Possible symptoms are:
> - Double evaluation of skins
> - Wrong size/position computation of ContextMenu
> - Error messages for invalid CSS variables, without any visual effect
> because it's corrected slightly later.
> @FlorianKirmaier Please do not rebase or force-push to an active PR as it
> invalidates existing review comments. Note for future reference, the bots
> always squash all changes into a single commit automatically as part of the
> integration. See [OpenJDK Developers’
> Guide](https://openjdk.org/guide/#working-with-pull-requests) for more
> information.
Specifically, please use `git merge master` rather than `git rebase master`
next time.
> > I could make it public with the accessor pattern.
>
> Are there any scenarios where some application code may need to call that new
> method? If not, it's an implementation detail that should be hidden.
>
> My guess the public `show()` method is all that's needed.
Agreed. Adding a new public method is only done that API is generally useful to
applications, not as an implementation convenience.
-------------
PR Comment: https://git.openjdk.org/jfx/pull/2215#issuecomment-6044339506
