8346281: [Windows] RenderScale doesn't update to HiDPI changes #1964

jperedadnr · 2025-11-05T12:32:09Z

This PR adds the missing native implementation for Windows, GlassWindow::HandleDPIEvent, to notify the (Java) window when there is a DPI change event, which can happen when the user changes the resolution of the screen (via Settings -> System -> Display -> scale), while the JavaFX application is running, in a similar way as Java Desktop handles this DPI event.

When such WM_DPICHANGED event happens, GlassWindow::HandleDPIEvent notifies the (Java) window, which changes its platform scale via Window::notifyScaleChanged, and GlassScreen::HandleDisplayChange(); is needed too, to update the platform scale of the screen where the window is at as well.

The change in Screen:: notifySettingsChanged is needed in order to prevent disposing screens that are not referenced by windows, and only do it when the screen instance of a given window does change.

There are no tests added to this PR, since these would require manual intervention to change the resolution of the display. In any case, the test case added to the issue runs fine now when the app runs on a given screen and the user changes its resolution. It has also been tested that it doesn't have any side effect on macOS.

For the case when the app runs on a different display that the one that was changed, see https://bugs.openjdk.org/browse/JDK-8371302 and a possible fix with #1963.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8346281: [Windows] RenderScale doesn't update to HiDPI changes (Bug - P4)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1964/head:pull/1964
$ git checkout pull/1964

Update a local copy of the PR:
$ git checkout pull/1964
$ git pull https://git.openjdk.org/jfx.git pull/1964/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1964

View PR using the GUI difftool:
$ git pr show -t 1964

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1964.diff

Using Webrev

Link to Webrev Comment

…windows replace them

bridgekeeper · 2025-11-05T12:32:36Z

👋 Welcome back jpereda! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-11-05T12:34:31Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

mlbridge · 2025-11-05T12:37:41Z

Webrevs

01: Full - Incremental (fbf6c6a3)
00: Full (9ec63bb5)

modules/javafx.graphics/src/main/java/com/sun/glass/ui/Screen.java

kevinrushforth · 2025-11-05T14:13:12Z

I'd like to review and test this, both in isolation and in connection with #1963

/reviewers 2

openjdk · 2025-11-05T14:14:35Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

andy-goryachev-oracle

no ill effects on macOS 15.7.1, will test on windows shortly

andy-goryachev-oracle · 2025-11-06T17:04:52Z

modules/javafx.graphics/src/main/native-glass/win/GlassWindow.cpp

+void GlassWindow::HandleDPIEvent(WPARAM wParam, LPARAM lParam)
+{
+    JNIEnv* env = GetEnv();
+    float scale = (float) LOWORD(wParam) / (float) USER_DEFAULT_SCREEN_DPI;


could we simply declare USER_DEFAULT_SCREEN_DPI to be a float?

No need, I've removed the casting, it wasn't needed.

if you declare this constant as float, you won't have to cast and it won't be converted each time it's used. saves 0.3 nanoseconds! :-)

The cast will almost certainly be done at compile time, so it is fine as is.

More importantly, I see that the constant is in an ifndef. Presuming that if the constant exists in the header file, it would be defined as an int, it would be wrong to define it as a float in the ifndef case.

andy-goryachev-oracle

Tested on windows in combination with #1963 , good job!

(I'll reapprove if/when you push the Set changes)

jperedadnr · 2025-11-06T21:39:50Z

(Sorry, pushed now)

andy-goryachev-oracle · 2025-11-06T21:57:33Z

GHA failure in linux is intermittent https://bugs.openjdk.org/browse/JDK-8365551 , please ignore.

kevinrushforth

The code changes look good. My initial testing looks good. I'll do some final testing tomorrow.

kevinrushforth · 2025-11-06T23:53:10Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/Screen.java

    public static void notifySettingsChanged() {
        // Save the old screens in order to dispose them later
-        List<Screen> oldScreens = screens;
+        Set<Screen> oldScreens = new HashSet<>();


Does the order matter? I don't think so, but if it does, LinkedHashSet would preserve the order.

No, it doesn't matter.

The old screens are "disposed" just by setting their native pointer to 0L, so they can't be reused, while the new screens instances are passed to the windows, to keep an updated instance.

Note that even if old screen and new screen have the very same information (nothing changed for this particular screen), since WinWindow::notifyMoving uses the equality operator (screen 1 == screen 2), we need to keep a valid instance in Window::screen, and therefore Screen::dispose is just a way of invalidating old instances. Then, since they are no longer referenced by any window, they can be gc'ed.

I had exactly the same question, and looked into the implementation.
Is it likely the implementation would change and the order be important in the future?

I don't think so. And I would also argue that it would be weird, if not even bad if the order of disposing a Screen would matter

Agreed. In this case I don't see how it could ever matter.

I only raised the question because whenever I see an ordinary HashSet being iterated, I always ask myself whether the iteration order matters.

I only raised the question because whenever I see an ordinary HashSet being iterated, I always ask myself whether the iteration order matters.

I agree 100%, especially since bugs in that case are really hard to find and debug.

I remember from my personal experience tests that sometimes failed, because the test did call iterator().next() on a Collection, which was a HashSet.😄

I just saw that I forgot to quote Andy on my previous answer. Just to emphasize this: I don't see how the implementation could be changed in such a way that the order would matter in the future.

kevinrushforth

All my testing is good. It fixes the bug and as far as I can tell, doesn't introduce new regressions.

My only question is that I'm still not sure why the changes in the shared code in Screen.java are needed.

kevinrushforth · 2025-11-07T21:48:27Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/Screen.java

+        // Dispose the old screens, if any
+        for (Screen screen : oldScreens) {
+            screen.dispose();
        }


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 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:

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.

Notify window and screen when dpi changes, dispose screens only when …

9ec63bb

…windows replace them

openjdk bot added the rfr Ready for review label Nov 5, 2025

Maran23 reviewed Nov 5, 2025

View reviewed changes

modules/javafx.graphics/src/main/java/com/sun/glass/ui/Screen.java Outdated Show resolved Hide resolved

kevinrushforth self-requested a review November 5, 2025 14:13

kevinrushforth mentioned this pull request Nov 5, 2025

8371302: [Windows] Stage coordinates in secondary display are not properly updated after changing settings of first display #1963

Closed

3 tasks

andy-goryachev-oracle reviewed Nov 6, 2025

View reviewed changes

Use set and remove unnecessary casting

fbf6c6a

andy-goryachev-oracle approved these changes Nov 6, 2025

View reviewed changes

kevinrushforth reviewed Nov 7, 2025

View reviewed changes

8346281: [Windows] RenderScale doesn't update to HiDPI changes #1964

Are you sure you want to change the base?

8346281: [Windows] RenderScale doesn't update to HiDPI changes #1964

Conversation

jperedadnr commented Nov 5, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 5, 2025

Uh oh!

openjdk bot commented Nov 5, 2025

Uh oh!

mlbridge bot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

kevinrushforth commented Nov 5, 2025

Uh oh!

openjdk bot commented Nov 5, 2025

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

jperedadnr commented Nov 6, 2025

Uh oh!

andy-goryachev-oracle commented Nov 6, 2025

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

jperedadnr commented Nov 5, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 5, 2025 •

edited

Loading

andy-goryachev-oracle Nov 6, 2025 •

edited

Loading