On Mon, 28 Sep 2020 15:20:33 GMT, Alexander Zuev <kiz...@openjdk.org> wrote:
> Moving review from Mercurial. See > https://mail.openjdk.java.net/pipermail/awt-dev/2020-August/016078.html for > previous > iteration. src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp line 986: > 984: hIcon = hIconLow; > 985: } else { > 986: fn_DestroyIcon((HICON)hIconLow); I wonder why you extract two icons, you never use both. This adds a delay: only one of the extracted icons is ever used. If the idea is to limit the size by 16, then min(16, size) passed as the icon size would do the trick. Each time we use this function, we request two icons but we never use both of them. Suggestion: if (size < 24) { size = 16; } hres = pIcon->Extract(szBuf, index, &hIcon, NULL, size); And `hIconLow` can be removed. src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 263: > 261: * a system file browser for the requested size. > 262: * > 263: * The default implementation gets information from the Suggestion: * <p>The default implementation gets information from the src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 264: > 262: * > 263: * The default implementation gets information from the > 264: * <code>ShellFolder</code> class. Whenever possible the icon Suggestion: * <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/share/classes/javax/swing/filechooser/FileSystemView.java line 269: > 267: * magnification factors. > 268: * > 269: * Example: <pre> Suggestion: * <p>Example: <pre> src/java.desktop/share/classes/sun/awt/shell/ShellFolder.java line 207: > 205: * Returns the icon of the specified size used to display this shell > folder. > 206: * > 207: * @param size size of the icon > 0(Valid range: 1 to 256) Suggestion: * @param size size of the icon > 0 (Valid range: 1 to 256) src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1035: > 1033: new BufferedImage(iconSize, iconSize, > BufferedImage.TYPE_INT_ARGB); > 1034: img.setRGB(0, 0, iconSize, iconSize, iconBits, 0, > iconSize); > 1035: 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. src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1105: > 1103: bothIcons.put(getLargeIcon? > SMALL_ICON_SIZE : LARGE_ICON_SIZE, newIcon2); > 1104: newIcon = new > MultiResolutionIconImage(getLargeIcon ? LARGE_ICON_SIZE > 1105: : SMALL_ICON_SIZE, > bothIcons); I propose to refactor this code into a separate method which always fetches small and large icon and puts into a corresponding cache. However, 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 1163: > 1161: > 1162: multiResolutionIcon.put(s, newIcon); > 1163: if (size < 8 || size > 256) { Suggestion: if (size < MIN_QUALITY_ICON || size > MAX_QUALITY_ICON) { src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolderManager2.java line 414: > 412: if (i >= 0) { > 413: return Win32ShellFolder2.getShell32Icon(i, > 414: key.startsWith("shell32LargeIcon > ")?LARGE_ICON_SIZE : SMALL_ICON_SIZE); Suggestion: key.startsWith("shell32LargeIcon ") ? LARGE_ICON_SIZE : SMALL_ICON_SIZE); test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 69: > 67: } > 68: } > 69: } Does it make sense to check that the icon is a multi-resolution image with the expected sizes? ------------- PR: https://git.openjdk.java.net/jdk/pull/380
