On Fri, 7 Nov 2025 21:48:27 GMT, Kevin Rushforth <[email protected]> wrote:
>> Jose Pereda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Use set and remove unnecessary casting
>
> 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?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1964#discussion_r2506946177
