On Thu, 27 Apr 2023 19:25:55 GMT, Rajat Mahajan <rmaha...@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
