RFR: JDK-8294427 - Check boxes and radio buttons have rendering issues on Windows in High DPI env
Alexey Ivanov
aivanov at openjdk.org
Tue May 2 15:31:30 UTC 2023
On Thu, 27 Apr 2023 19:25:55 GMT, Rajat Mahajan <rmahajan at openjdk.org> wrote:
> Problem:
>
> Check boxes and radio buttons in Windows Look-and-Feel have rendering issues when window is moved from display with one scale to display with a different scale on a multi-monitor setup:
>
> - Scrawly ticks in checkboxes;
> - Wrong circle relations in selected radio buttons.
>
> Root-cause:
> We open theme on AWT Toolkit Window which always has Primary Monitor DPI.
> Due to this when the app window goes to Secondary Screen with different DPI UI buttons
> appear incorrectly rendered.
> Following is a list proposed changes to fix this issue.
>
>
> Proposed Fix with Summary of changes:
>
> 1. Open a new Theme Handle per the DPI of the Screen where the App window is.
> --> This makes sure we get the correct size for UI buttons based on the DPI of the screen where the app.
> window is.
>
> 2. GetPartSize() of icons returns size based on standard size = 96 DPI.
> --> This change makes sure that the default size of UI buttons is 96 since we use this on Java side to layout UI.
>
> 3. Rect size for icons in native paintBackground() function is fetched from Windows that specific DPI.
> -->This makes sure that the UI buttons aren't stretched because the size calculated on Java side is different from what Windows returns. Thus UI buttons are scaled correctly once we get their size back from Windows.
>
> 4. Adjust width and the height of the resolution variant image so that for scaling values of 1.25 , 2.25 , and such we always floor, while for 1.5, 1.75, 2.5, 2.75 , and such we always ceil.
> --> This helps make sure that for .25s scaling we get better rendering.
> This is because when we go from Double to Int for pixel rendering we sometimes need to floor or ceil to get crisp rendering.
>
> As of now with these changes the rendering is crisp and good for all scaling factors with the exception .25s wherein some small artifact is seen sometimes in rendering of buttons but is still much better compared to what we have now.
>
>
> Testing:
>
> Tested locally on my Windows machine with a 2 monitor setup 125%, 150%, 175%, 225% scaling values and on mach5.
>
> ___________________________________
> Monitor 1 | Monitor 2
> (Primary) |
> |
> 125% | 175%
> 150% | 175%
> 150% | 225%
> 175% | 175%
> 175% | 150%
> 175% | 225%
> _____________________ |_____________
>
> Also tested on setup with scaling values of up-to 350%.
src/java.desktop/share/classes/sun/swing/CachedPainter.java line 318:
> 316: public Image getResolutionVariant(double destWidth, double destHeight) {
> 317: int w = (int) Math.floor(destWidth + 0.5);
> 318: int h = (int) Math.floor(destHeight + 0.5);
Isn't it what `Region.clipRound` does?
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/XPStyle.java line 63:
> 61:
> 62: import java.lang.Math;
> 63:
The imports should be sorted, static imports follow regular class imports.
Please expand all wildcard imports with per-class imports, which is done when classes are updated.
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 108:
> 106: if(dpiAwareWidgetToTheme.containsKey(dpi))
> 107: {
> 108: if (!dpiAwareWidgetToTheme.get(dpi).isEmpty())
It is better to skip `containsKey` and use `get`; if `get` returns `null`, put the key into the map.
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 136:
> 134: if (!dpiAwareWidgetToTheme.isEmpty()) {
> 135: for (Long value :
> 136: dpiAwareWidgetToTheme.get(dpi).values()) {
I think it's still possible that `get(dpi)` returns `null`, you should handle `null` gracefully.
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 140:
> 138: }
> 139: dpiAwareWidgetToTheme.get(dpi).clear();
> 140: dpiAwareWidgetToTheme.clear();
You clear the entire map, yet it don't call `closeTheme` for dpi values other than the passed one.
You should iterate over `dpiAwareWidgetToTheme.values().values()` (pseudo-code).
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 155:
> 153: if(dpiAwareWidgetToTheme.containsKey(dpi))
> 154: {
> 155: theme = dpiAwareWidgetToTheme.get(dpi).get(widget);
Avoid using `contains` followed by `get`, use `get` instead and handle `null` value.
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 179:
> 177: try {
> 178:
> 179: /* We get the part size vased on the theme for current screen DPI and pass it to paintBackground */
Suggestion:
/* We get the part size based on the theme for current screen DPI and pass it to paintBackground */
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 185:
> 183: part, state,
> 184: (int)d.getWidth(),
> 185: (int)d.getHeight() , w, h, stride);
Suggestion:
d.width,
d.height, w, h, stride);
You can access the fields directly and avoid casting.
src/java.desktop/windows/native/libawt/windows/ThemeReader.cpp line 243:
> 241: (JNIEnv* env, jclass klass, jstring widget, jint dpi) {
> 242:
> 243: LPCTSTR str = (LPCTSTR)JNU_GetStringPlatformChars(env, widget, NULL);
I'd rather keep the original formatting for casts as well as for `JNIEnv *env` in the declaration.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1182682786
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1182685469
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1182690251
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1182694155
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1182696873
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1182699492
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1182702793
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1182704389
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1182709855
More information about the client-libs-dev
mailing list