RFR: JDK-8294427 - Check boxes and radio buttons have rendering issues on Windows in High DPI env [v9]
Alexey Ivanov
aivanov at openjdk.org
Tue Jun 6 20:14:16 UTC 2023
On Thu, 1 Jun 2023 19:56:28 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%.
>
> Rajat Mahajan has updated the pull request incrementally with one additional commit since the last revision:
>
> code changes as per code review
Overall, it looks good to me except for minor (stylistic) comments.
Please update the copyright year in the modified files.
src/java.desktop/windows/classes/com/sun/java/swing/plaf/windows/XPStyle.java line 84:
> 82: import sun.swing.CachedPainter;
> 83:
> 84: import java.awt.geom.AffineTransform;
The import for `AffineTransform` is out of place, it should be above with other `java.awt.*` classes. Its place is between `Toolkit` and `BufferedImage`.
import java.awt.Toolkit;
import java.awt.geom.AffineTransform;
import java.awt.image.BufferedImage;
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 48:
> 46: private static final int defaultDPI = 96;
> 47: private static final Map<Integer, Map<String, Long>> dpiAwareWidgetToTheme
> 48: = new HashMap<Integer, Map<String, Long>>();
Suggestion:
private static final Map<Integer, Map<String, Long>> dpiAwareWidgetToTheme
= new HashMap<>();
You can use the diamond operator, that is drop the type parameters when invoking the constructor.
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 131:
> 129: }
> 130:
> 131: // mostly copied from the javadoc for ReentrantReadWriteLock
This comment belongs before `if (theme == null)`, it explains how it switches between read- and write-locks.
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 136:
> 134: Map<String, Long> dpiWidgetToTheme = dpiAwareWidgetToTheme.get(dpi);
> 135:
> 136: if (dpiWidgetToTheme != null) {
Is `widgetToTheme` a better name? You already resolved the dpi-aware part.
I'd remove the blank line between the initialiser and null-check, they're tightly related.
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 155:
> 153: private static native void paintBackground(int[] buffer, long theme,
> 154: int part, int state, int rectRight,
> 155: int rectBottom, int w, int h, int stride);
Mostly stylistic change, yet it could make parsing the list of parameters easier:
Suggestion:
int part, int state,
int rectRight, int rectBottom,
int w, int h, int stride);
Perhaps, wrap `stride` to the next line too.
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 165:
> 163: Dimension d = getPartSize(getTheme(widget, dpi), part, state);
> 164: paintBackground(buffer, getTheme(widget, dpi), part, state,
> 165: d.width, d.height , w, h, stride);
Drop the extra space:
Suggestion:
d.width, d.height, w, h, stride);
src/java.desktop/windows/classes/sun/awt/windows/ThemeReader.java line 321:
> 319: return getThemeBackgroundContentMargins(getTheme(widget, defaultDPI),
> 320: part, state, boundingWidth,
> 321: boundingHeight);
I suggest keeping both `boundingWidth` and `boundingHeight` on the same line.
Suggestion:
return getThemeBackgroundContentMargins(getTheme(widget, defaultDPI),
part, state,
boundingWidth, boundingHeight);
src/java.desktop/windows/native/libawt/windows/ThemeReader.cpp line 43:
> 41: #define GREEN_SHIFT 8
> 42:
> 43:
Perhaps, keep this blank line?
src/java.desktop/windows/native/libawt/windows/ThemeReader.cpp line 178:
> 176: HTHEME hTheme = OpenThemeDataFuncForDpi (
> 177: AwtToolkit::GetInstance().GetHWnd(),
> 178: L"Button", dpi);
Suggestion:
// We need to make sure we can load the Theme.
// Use the default DPI value of 96 on windows.
unsigned int dpi = 96;
HTHEME hTheme = OpenThemeDataFuncForDpi(
AwtToolkit::GetInstance().GetHWnd(),
L"Button", dpi);
Does it make sense to rename `dpi` to `defaultDPI` like it's done on the Java side.
Can we mark the `dpi` variable with [`constexpr`](https://en.cppreference.com/w/cpp/language/constexpr) specifier? It's available since C++11, I believe Java currently uses C++17, so this specifier can be used.
src/java.desktop/windows/native/libawt/windows/ThemeReader.cpp line 433:
> 431: rect.bottom = rectBottom;
> 432: rect.right = rectRight;
> 433:
Perhaps, the newly added blank is redundant here.
-------------
Changes requested by aivanov (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13701#pullrequestreview-1465903474
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1220183481
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1220194871
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1220206910
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1220201870
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1220214137
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1220214805
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1220220319
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1220229355
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1220239818
PR Review Comment: https://git.openjdk.org/jdk/pull/13701#discussion_r1220247503
More information about the client-libs-dev
mailing list