On Thu, 1 Jun 2023 19:56:28 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%.
>
> 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
