On Sat, 8 Nov 2025 14:17:30 GMT, Jose Pereda <[email protected]> wrote:
>> modules/javafx.graphics/src/main/java/com/sun/glass/ui/Screen.java line 403:
>>
>>> 401: for (Screen screen : oldScreens) {
>>> 402: screen.dispose();
>>> 403: }
>>
>> I'm still a little puzzled as to why a change was needed in the class at
>> all. Even with the change you made to the native Windows glass code, it
>> seems that calling dispose for all of the old Java screen objects is what we
>> want. Am I missing something?
>
> There is a reason for it, otherwise I wouldn't have modified `Screen`.
>
> If you run the
> [test](https://bugs.openjdk.org/secure/attachment/112708/Menu_RenderScale_8346281.java)
> in the JBS issue, on Windows with two monitors (primary with scale > 100%),
> with the changes from `GlassWindow.cpp` only, without changing `Screen`, when
> you open the File menu of the window in the secondary display, the context
> menu is misplaced:
>
> <img width="404" height="341" alt="Screenshot 2025-11-08 143811"
> src="https://github.com/user-attachments/assets/5844155e-c6cb-4b9a-9edb-c81b1140b8bb";
> />
>
> Adding some debugging info, it can be seen that, when the application starts
> (without doing any external change in Windows settings), there is a DPI
> event, that calls `notifySettingsChanged`, at a point where there is only a
> `Window` instance (the one in the primary screen), instead of two. The two
> screens get disposed (that is, `Screen.ptr = 0`), and later on, when the
> secondary window is created, it gets assigned the disposed secondary screen,
> with this `ptr = 0`.
>
> When opening the `File` menu of the secondary window, another DPI event is
> triggered, `notifySettingsChanged` is called, but since there was no real
> change in the screens, the new screens are the same as the old ones (the
> valid `ptr` didn't change after the event). However, the valid secondary
> screen is not assigned to the secondary Window, because the if test, (ptr
> from old screen was 0, and new screen ptr >0 doesn't match), so it keeps the
> old screen instance with `ptr = 0`.
>
> This causes some unexpected issues for instance in `WinWindow::notifyMoving`,
> as there is a mixture of valid screens (from `Screen.getScreens()`) and
> invalid one in `Window::getScreen`, and the equality checks (`screen1 ==
> screen2`) fails, ultimately providing wrong screen information for the popup.
>
> So the change in `Screen` that I propose in this PR removes the disposal of
> screens if these are not assigned to a window yet.
>
> Does that make sense?
Hmm. I see what you are saying, and can confirm the behavior. With your
proposed fix, though, it seems like we will still have a situation where an
in-use screen object is no longer in the list of screens. When this happens it
will point to the same native screen object as one of the newly-created
screens. This seems fragile.
Maybe we could still dispose all old screens, but do it in a way that allows it
to later be mapped to a new screen? Or, thinking out loud, maybe "put back" the
screens that didn't actually change as the result of the DPI change
notification?
I'll want to think about this and take a closer look, but it will be a couple
days before I can get to it.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1964#discussion_r2512140208
