<AWT Dev> RFR: 8182043: Access to Windows Large Icons

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

<AWT Dev> RFR: 8182043: Access to Windows Large Icons

Alexander Zuev-3
Fix updated after first round of review.

-------------

Commit messages:
 - 8182043: Access to Windows Large Icons

Changes: https://git.openjdk.java.net/jdk/pull/2875/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8182043
  Stats: 335 lines in 6 files changed: 270 ins; 29 del; 36 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2875.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875

PR: https://git.openjdk.java.net/jdk/pull/2875
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8182043: Access to Windows Large Icons

Alexander Zuev-3
On Mon, 8 Mar 2021 13:22:07 GMT, Alexander Zuev <[hidden email]> wrote:

> Fix updated after first round of review.

Continuation of review at https://github.com/openjdk/jdk/pull/380

-------------

PR: https://git.openjdk.java.net/jdk/pull/2875
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8182043: Access to Windows Large Icons

Sergey Bylokhov-2
On Mon, 8 Mar 2021 13:23:00 GMT, Alexander Zuev <[hidden email]> wrote:

>> Fix updated after first round of review.
>
> Continuation of review at https://github.com/openjdk/jdk/pull/380

Just to continue the thread from the old review, did you have a chance to look at the possibility of loading all existed icons embedded in the file? It was discussed here:
https://github.com/openjdk/jdk/pull/380#issuecomment-702999573

-------------

PR: https://git.openjdk.java.net/jdk/pull/2875
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8182043: Access to Windows Large Icons

Alexander Zuev-3
On Wed, 10 Mar 2021 00:55:00 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> Continuation of review at https://github.com/openjdk/jdk/pull/380
>
> Just to continue the thread from the old review, did you have a chance to look at the possibility of loading all existed icons embedded in the file? It was discussed here:
> https://github.com/openjdk/jdk/pull/380#issuecomment-702999573

> Just to continue the thread from the old review, did you have a chance to look at the possibility of loading all existed icons embedded in the file? It was discussed here:
> [#380 (comment)](https://github.com/openjdk/jdk/pull/380#issuecomment-702999573)

Yes, i did. There were 3 shortcomings: 1. It works in about 10% of the cases in all other cases it reports only one icon available (32x32) even if there are other resolution icons. 2. It is quite  slow, especially on network drives. 3. Does not work at all in files that are within AppData\Roaming folder - returning list is just empty.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2875
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8182043: Access to Windows Large Icons

Alexey Ivanov-2
In reply to this post by Alexander Zuev-3
On Mon, 8 Mar 2021 13:22:07 GMT, Alexander Zuev <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> RFR: 8182043: Access to Windows Large Icons

Alexey Ivanov-2
In reply to this post by Alexander Zuev-3
On Mon, 8 Mar 2021 13:22:07 GMT, Alexander Zuev <[hidden email]> wrote:

> Fix updated after first round of review.

src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 260:

> 258:
> 259:    /**
> 260:     * Icon for a file, directory, or folder as it would be displayed in

*Returns an icon* for a file…

src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 276:

> 274:     *
> 275:     * @param f a <code>File</code> object
> 276:     * @param size width and height of the icon in pixels

Pixels could be ambiguous now. Usually, Swing deals with user's space. That is 16×16 icon should actually be 32×32 if the scale factor of the current display is 200%.

Yes, this issue is somewhat irrelevant because the method returns multi-resolution icon. However, the terminology used must be unambiguous and clear; for consistency with other Swing API, it should be in terms of user's space coordinates, *virtual pixels*.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2875