Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+25 −7
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ec63bb
Notify window and screen when dpi changes, dispose screens only when …
jperedadnr Nov 3, 2025
fbf6c6a
Use set and remove unnecessary casting
jperedadnr Nov 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions modules/javafx.graphics/src/main/java/com/sun/glass/ui/Screen.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -26,7 +26,9 @@

import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

public final class Screen {

Expand Down Expand Up @@ -370,7 +372,7 @@ public static void setEventHandler(EventHandler eh) {
*/
public static void notifySettingsChanged() {
// Save the old screens in order to dispose them later
List<Screen> oldScreens = screens;
Set<Screen> oldScreens = new HashSet<>();
Copy link
Member

@kevinrushforth kevinrushforth Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@jperedadnr jperedadnr Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@Maran23 Maran23 Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@kevinrushforth kevinrushforth Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Maran23 Maran23 Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Get the new screens
initScreens();
Expand All @@ -389,16 +391,15 @@ public static void notifySettingsChanged() {
for (Screen newScreen : screens) {
if (oldScreen.getNativeScreen() == newScreen.getNativeScreen()) {
w.setScreen(newScreen);
oldScreens.add(oldScreen);
break;
}
}
}

// Dispose the old screens
if (oldScreens != null) {
for (Screen screen : oldScreens) {
screen.dispose();
}
// Dispose the old screens, if any
for (Screen screen : oldScreens) {
screen.dispose();
}
Comment on lines +400 to 403
Copy link
Member

@kevinrushforth kevinrushforth Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@jperedadnr jperedadnr Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Screenshot 2025-11-08 143811

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?

Copy link
Member

@kevinrushforth kevinrushforth Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

Expand Down
17 changes: 17 additions & 0 deletions modules/javafx.graphics/src/main/native-glass/win/GlassWindow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,10 @@ LRESULT CALLBACK GlassWindow::CBTFilter(int nCode, WPARAM wParam, LPARAM lParam)
return ::CallNextHookEx(GlassWindow::sm_hCBTFilter, nCode, wParam, lParam);
}

#ifndef USER_DEFAULT_SCREEN_DPI
#define USER_DEFAULT_SCREEN_DPI 96
#endif

#ifndef WM_DPICHANGED
#define WM_DPICHANGED 0x02E0
#endif
Expand Down Expand Up @@ -365,6 +369,10 @@ LRESULT GlassWindow::WindowProc(UINT msg, WPARAM wParam, LPARAM lParam)
}
HandleViewSizeEvent(GetHWND(), msg, wParam, lParam);
break;
case WM_DPICHANGED:
HandleDPIEvent(wParam, lParam);
GlassScreen::HandleDisplayChange();
break;
case WM_MOVING:
m_winChangingReason = WasMoved;
break;
Expand Down Expand Up @@ -810,6 +818,15 @@ void GlassWindow::HandleSizeEvent(int type, RECT *pRect)
CheckAndClearException(env);
}

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

env->CallVoidMethod(m_grefThis, midNotifyScaleChanged, scale, scale, scale, scale);
CheckAndClearException(env);
}

void GlassWindow::HandleActivateEvent(jint event)
{
const bool active = event != com_sun_glass_events_WindowEvent_FOCUS_LOST;
Expand Down