On Mon, 8 Mar 2021 13:22:07 GMT, Alexander Zuev <kiz...@openjdk.org> wrote:
> Fix updated after first round of review.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1044:
> 1042: new BufferedImage(iconSize, iconSize,
> BufferedImage.TYPE_INT_ARGB);
> 1043: img.setRGB(0, 0, iconSize, iconSize, iconBits, 0,
> iconSize);
> 1044: return img;
There are cases where the size of the buffered image is different from the
requested size. It could affect the layout because it breaks the assumption
that the returned image has the requested size but it may be larger. (Or is it
no longer possible?) I think it should be wrapped into
`MultiResolutionIconImage` in this case.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1114:
> 1112: bothIcons.put(getLargeIcon?
> SMALL_ICON_SIZE : LARGE_ICON_SIZE, newIcon2);
> 1113: newIcon = new
> MultiResolutionIconImage(getLargeIcon ? LARGE_ICON_SIZE
> 1114: : SMALL_ICON_SIZE,
> bothIcons);
I still propose to refactor this code to make it clearer. The code always
returns two icons: _small + large_ or _large + small_ — combined in
`MultiResolutionIconImage`.
You can get `smallIcon` and `largeIcon` and then wrap them into
`MultiResolutionIconImage` in the correct order and store each icon in the
cache.
My opinion hasn't changed here.
I still think there's not much value in getting smaller icon size in addition
to larger one. Or does it provide an alternative image for the case where the
system scaling is 200% and the window is moved to a monitor with scaling of
100%?
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1195:
> 1193: */
> 1194: static Image getShell32Icon(int iconID, int size) {
> 1195: boolean useVGAColors = true; // Will be ignored on XP and later
I cannot see where `useVGAColors` is used. Since the supported OS are later
than XP, it can be removed, can't it?
src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp line 983:
> 981: size = 16;
> 982: }
> 983: hres = pIcon->Extract(szBuf, index, &hIcon, NULL, (16 << 16)
> + size);
Suggestion:
hres = pIcon->Extract(szBuf, index, &hIcon, NULL, size);
Now you request only one icon.
test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 64:
> 62: }
> 63:
> 64: if (icon.getIconWidth() != size) {
Does it make sense to check that the icon is a multi-resolution image with the
expected sizes?
We know for sure `explorer.exe` contains the entire set of icons.
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
264:
> 262: * <p>
> 263: * The default implementation gets information from the
> 264: * <code>ShellFolder</code> class. Whenever possible, the icon
Do we continue using `<code>` elements? I thought that {@code} is the preferred
way to markup class names.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1112:
> 1110: Map<Integer, Image> bothIcons = new
> HashMap<>(2);
> 1111: bothIcons.put(getLargeIcon?
> LARGE_ICON_SIZE : SMALL_ICON_SIZE, newIcon);
> 1112: bothIcons.put(getLargeIcon?
> SMALL_ICON_SIZE : LARGE_ICON_SIZE, newIcon2);
Suggestion:
bothIcons.put(getLargeIcon ?
LARGE_ICON_SIZE : SMALL_ICON_SIZE, newIcon);
bothIcons.put(getLargeIcon ?
SMALL_ICON_SIZE : LARGE_ICON_SIZE, newIcon2);
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1146:
> 1144: }
> 1145: Map<Integer, Image> multiResolutionIcon = new HashMap<>();
> 1146: int start = size > MAX_QUALITY_ICON ?
> ICON_RESOLUTIONS.length - 1 : 0;
Does it make sense to always start at zero?
The icons of smaller size will never be used, will they?
Thus it's safe to start at the index which corresponds to the requested size if
`size` matches, or the index such as `ICON_RESOLUTIONS[index] < size &&
ICON_RESOLUTIONS[index + 1] > size`.
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1149:
> 1147: int increment = size > MAX_QUALITY_ICON ? -1 : 1;
> 1148: int end = size > MAX_QUALITY_ICON ? -1 :
> ICON_RESOLUTIONS.length;
> 1149: for (int i = start; i != end; i+=increment) {
Suggestion:
for (int i = start; i != end; i += increment) {
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1422:
> 1420: int w = (int) width;
> 1421: int retindex = 0;
> 1422: for (Integer i: resolutionVariants.keySet()) {
Suggestion:
for (Integer i : resolutionVariants.keySet()) {
-------------
PR: https://git.openjdk.java.net/jdk/pull/2875
