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

[Alexey Ivanov]

Hi Alex,

Sorry for my delayed reply.


*FileSystemView.java*

263     * The default implementation gets information from the 
ShellFolder class.
<p> is missing at the start to create a new paragraph in javadoc.
Shall ShellFolder be wrapped in {@code }?

264     * Whenever possible the icon returned will be a multi-resolution 
icon
265     * image, which will will allow better scaling with different
266     * magnification factors.
I think there should be a comma after “possible”.
I'm unsure whether the comma is necessary before “which”, likely yes.
There's double “will” after “which”.
Shall we use Present tense instead of Future in this sentence?

268     * Example: <pre>
Also <p> is missing.

275     * @param size width and height of the icon in pixels to be scaled
Is “to be scaled” necessary here?
It is the requested size of the icon. To render the icon at the 
requested size, it may be scaled (up or down).


*Win32ShellFolder2.java*

I guess you meant to use the new constant MAX_QUALITY_ICON in the 
following lines:
1136             int start = size > 256 ? ICON_RESOLUTIONS.length - 1 : 0;
1137             int increment = size > 256 ? -1 : 1;
1138             int end = size > 256 ? -1 : ICON_RESOLUTIONS.length;

Shall this be updated to size < 16 since 8 has been dropped from the list?
1141                 if (size < 8 || size > 256 || (s >= size/2 && s <= 
size*2)) {
And this one too:
1161                     if (size < 8 || size > 256) {

A new constant MIN_QUALITY_ICON=16?


Will List/ArrayList more compact storage? The Image has its own size.
1377         final Map<Integer, Image> resolutionVariants = new HashMap<>();

Shall it be wrapped into unmodifiableList?
1427             return new ArrayList<Image>(resolutionVariants.values());


Please also see comments inline:

On 21/07/2020 22:33, Alexander Zuev wrote:
> Hi Alexey,
>
>   first of all - the new and improved webrev is available at 
> http://cr.openjdk.java.net/~kizune/8182043/webrev.03/
>
>   I returned the limitation of icon sizes in the result which should 
> enhance the performance, also i fixed some corner cases and improved 
> docuentation. More details inline.
>
> /Alex
>
> On 7/15/2020 4:46 PM, Alexey Ivanov wrote:
>> Hi Alexander,
>>
>> I've run some tests and this version works good. However, I noticed 
>> the new version is slower than before, it could be due to the fact 
>> that the entire set of icons is extracted from the native. On 
>> average, JFileChooser is 100ms slower to show on the screen.
> That should be dealt with in the new patch.
>>
>> I think we should limit the number of icons that we extract as you 
>> did in webrev.01.
> Done.

Yes, it helped, JFileChooser gets displayed a little bit faster.

>> If I request the icon of size 16:
>> FileSystemView.getFileSystemView().getSystemIcon(new File("C:\"), 16);
>> the resulting icon contains icon sizes 16, 24, 32, 48, 64, 72, 96, 
>> 128, 256.
>>
>> Hardly the icon of size 96 and above will ever be used: 96px is 6 
>> times 16px. Getting 48px-sized icon corresponds to 300% scaling, it 
>> may be reasonable, yet still unlikely to be used.
>>
>> Then if I request the icon of size 48:
>> FileSystemView.getFileSystemView().getSystemIcon(new File("C:\"), 48);
>> the resulting icon contains the same set of sizes. But the sizes 
>> smaller than the base size will never be used. I believe scaling 
>> factors less than 100% are not supported.
> I still tend to leave it on - it does not hurt performance much and

Well, in the 3rd version the set of icons changes as the requested size 
changes from 16 to 48. That's good.

>> So we should decide up to which resolution we get the icons. Getting 
>> icons up to 200% makes sense.
>>
>> What about the lower limit? if the requested size matches one from 
>> our list, nothing to decide. What if size of 33 is requested? Scaling 
>> up 32×32 icon will likely give a better result than scaling down 48×48.
> That's why in my code i do keep lower resolution images and when asked 
> about the resolution variant i'm returning closest resolution to one 
> asked so in your case if i got asked for 33 pixel icon i will present 
> the 32 pixel one.

It's a good point and a nice trick.

However, I still think we can drop the smaller size if the requested 
size matches one the sizes that we get from the system. Anyway, I am 
fine with the current implementation.

>> Will such an implementation be good enough? This would resolve this 
>> RFE: provide access to Windows large icon.
>>
>>
>> Then another problem which we're trying to solve: make JFileChooser 
>> support High DPI environments. In majority of cases, JFileChooser 
>> will use another method to extract the icon: 
>> Win32ShellFolder2.getIcon(boolean). This function uses 
>> SHGFI_SMALLICON or SHGFI_LARGEICON parameters to SHGetFileInfo, and 
>> therefore the size is limited to 16×16 and 32×32. It will return the 
>> icon of the currently configured size for the shell icon, small and 
>> large correspondingly; however, this parameter cannot be easily 
>> changed. On the other hand, the shell icon size takes DPI into 
>> account, and the size of the returned icon may differ from 16 and 32.
>>
>> This situation where the size was different from the requested one 
>> was handled by wrapping the icon into MultiResolutionIconImage:
>>
>> Win32ShellFolder2.makeIcon(long hIcon, int size)
>> -                return size == baseSize
>> -                        ? img
>> -                        : new MultiResolutionIconImage(baseSize, img);
>> +                        new BufferedImage(iconSize, iconSize, 
>> BufferedImage.TYPE_INT_ARGB);
>> +                img.setRGB(0, 0, iconSize, iconSize, iconBits, 0, 
>> iconSize);
>> +                return img;
>>
>> The new code works well for getIcon(int size) which wraps a set of 
>> icons into MultiResolutionIconImage.
>>
>> I think the calls to getIcon(boolean getLargeIcon) should still wrap 
>> the result into MultiResolutionIconImage if the size differs from the 
>> requested one.
> Right now the icon got wrapped in the multiresolutionimage when more 
> than one variant is available.
> I can change it to wrap it every time even if the bigger or smaller 
> variant is not available but i do not see
> how it can benefit the file manager itself.

Yes, that's correct. The problem comes up in the case where the 
requested size differs from the returned size. For example, in the 
situation that you describe below.

I could affect the layout because it breaks the assumption that the 
returned image has the requested size but it may be larger.

>> The code at
>> 1158                 if (newIcon.getWidth(null) == s) {
>> 1159                     multiResolutionIcon.put(s, newIcon);
>> 1160                 }
>> handles the same situation. Yet here, we'll throw away the icon if 
>> its size is not equal to the requested one. I wonder if this 
>> condition is ever false, however.
> I removed this condition for i found a corner case when it is 100% 
> false - the dynamically generated icons in AppData/Roaming folder if 
> promary monitor is set to any zoom factor.
>
>> To provide better support for High DPI environments in JFileChooser, 
>> getIcon(boolean getLargeIcon) should also return a set of icons.
> It does right now. And result is better than it was before.

Yes, that's correct. I'm not challenging this, it's definitely an 
improvement.

Yet under under a separate bug id, we can update the implementation of 
getIcon(boolean getLargeIcon) which JFileChooser uses so that it returns 
a larger set of icons for better High DPI support. We can also unify the 
way the icons are requested from the system: at this time, there are two 
similar functions which use different APIs.


>> This also applies to Win32ShellFolder2.getSystemIcon(SystemIcon 
>> iconType) which returns standard icons for message boxes, or 
>> JOptionPane.
>>
>>
>>> But there's one catch: new way of retrieving icons does not get the 
>>> custom drive
>>> icon (can be seen at disk C: - the windows logo identifying the 
>>> windows system drive
>>> is no longer present). I am questioning myself, what is better: to 
>>> have better icon quality
>>> and missing custom disk logo or to have custom disk logo and 
>>> mediocre icon quality? 
>>
>> Which is weird, but I also noticed it.
>>
>> I wonder why different APIs in Windows give different icons: 
>> SHGetFileInfo which is used in getIcon(boolean getLargeIcon), and 
>> IExtractIcon::GetIconLocation and IExtractIcon::Extract which are 
>> used in getIcon(int size) that calls extractIcon(). The former 
>> function, SHGetFileInfo, can also decorate icons with link (shortcut) 
>> overlay whereas the latter does not seem to provide such an option.
> There is a way to try to get overlay for the icon but it also works 
> inconsistently.

It's not only for the system drive. The SD card has its own icon, some 
external drives use its own icon — they were displayed before but not 
any more. So it looks as if different APIs in Windows return different 
icons.

Whatever the problem is, I am fine with the current approach, it works 
in the majority of cases. This problem can be resolved later.


There are a couple comments below:

>> <SNIP>
>>
>> Regards,
>> Alexey
>>
>> On 09/07/2020 00:21, Alexey Ivanov wrote:
>>> Hi Alexander,
>>>
>>> Sorry for my belated review.
>>>
>>> *FileSystemView.java*
>>> I couldn't build JDK because of the warning about empty <p> in 
>>> javadoc for getSystemIcon(File f, int size).
>>>
>>> <p> is not needed before <pre>. Yet <p> should be added after each 
>>> empty line in the javadoc to start a new paragraph, otherwise all 
>>> the text is presented one long description.
>>>
>>> 276     * @param size width and height of the icon in pixels to be 
>>> scaled
>>> 277     *             (valid range: 1 to 256)
>>>
>>> I'd drop "to be scaled"; width and height are the base size of the 
>>> icon for the case.
>>>
>>>
>>> *ShellFolder.java*
>>> 205      * @param size size of the icon > 0(Valid range: 1 to 256)
>>>
>>> I recommend adding space before the opening parenthesis and use 
>>> lower-case in the parentheses as in FileSystemView.java.

In the 3rd version, the documentation in FileSystemView.java does not 
mention the valid range any more. Is it intended?

>>> <SNIP>
>>>
>>> *ShellFolder2.cpp*
>>> 981             hres = pIcon->Extract(szBuf, index, &hIcon, 
>>> &hIconLow, (16 << 16) + size);
>>> 982             if (size < 24) {
>>>
>>> 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.

This is still in the 3rd version. Each time we use this function, we 
request two icons but we never use both of them.

The code could be
             if (size < 24) {
                 size = 16;
             }
             hres = pIcon->Extract(szBuf, index, &hIcon, NULL, size);

>>> <SNIP>
-- 
Regards,
Alexey