[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons
From:       Alexey Ivanov <aivanov () openjdk ! java ! net>
Date:       2020-10-08 22:43:23
Message-ID: j6-IdWKuJNRBs7fRdEqlI6Ht5VkFQxMezy0qk2mlj7g=.0d379bf1-f2dd-4d1b-8e12-849dd7579f88 () github ! com
[Download RAW message or body]

On Mon, 28 Sep 2020 15:20:33 GMT, Alexander Zuev <kizune@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


[prev in list] [next in list] [prev in thread] [next in thread]