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

[Alexey Ivanov]

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.

I think we should limit the number of icons that we extract as you did 
in webrev.01.

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.

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.


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.

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.


To provide better support for High DPI environments in JFileChooser, 
getIcon(boolean getLargeIcon) should also return a set of icons.

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.


As for High DPI environments and memory footprint, getting icon sizes 
according to the display scaling looks like an ideal solution. But it 
seems too tricky. The MultiResolutionIconImage should store all the 
details of the file or the shell folder to be able to fetch a different 
size of DPI changes. Eventually, keeping lots of objects to achieve that 
could have larger footprint than storing a set of icons all the time.

Overall, the current approach of fetching a set of icons to cater for 
the most common scaling factors looks sensible.


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.
>
>
> *Win32ShellFolder2.java*
> 80     private final static int[] ICON_RESOLUTIONS
> 81             = {8, 16, 24, 32, 48, 64, 72, 96, 128, 256};
> Shall we drop 8 from the list? The size of 8×8 is non-standard for 
> Windows, is it provided by any Windows Shell interfaces?
>
>
> *Win32ShellFolder2.java*
> The trickery with newIcon2 in getIcon(final boolean getLargeIcon) is 
> for getting both 16×16 and 32×32 and returning them as 
> MultiResolutionImage, isn't it?
>
> I guess the best results would be if we get a list of 16×16, 24×24, 
> and 32×32 icons when small icon is requested, and 32×32, 48×48, and 
> 64×64 when large icon is requested, which would cover scaling factors 
> of 100%, 150%, and 200%.
>
> I don't think we'll ever need small icon when large one is requested, 
> and if I read the code correctly this is what would happen). However, 
> JFileChooser does not seem to allow for large icons in its file view, 
> thus adding the larger icon makes the rendering crispier in High DPI 
> environments.
>
> 1154                 if (size > 512 || size < 4) {
> 1155                     return newIcon;
> 1156                 }
> I can understand that there are no useful resolutions if size of 512 
> or larger is requested. Should it be 256 or rather 
> ICON_RESOLUTIONS[ICON_RESOLUTIONS.length - 1]?
>
> Javadoc states the valid range ends at 256.
>
> As for the minimum size… In my opinion, icon size less than 8 is not 
> viable; on the other hand we can't make any assumptions. So if the 
> requested size is less than 4, then we'll return only 8×8 icon (or 
> 16×16, if 8 is dropped from the list). Do I get it right?
>
>
> *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.
>
>
> With this new version, I noticed that JFileChooser takes longer to 
> appear and then longer to populate the file list.
>
> I'm still playing around with the build but I wanted to provide my 
> feedback.
>
>
> Regards,
> Alexey
>
> On 29/06/2020 15:27, Alexander Zuev wrote:
>> Alexey, Sergey,
>>
>>   here's the latest version of the fix:
>>
>> http://cr.openjdk.java.net/~kizune/8182043/webrev.02/
>>
>> It adds one more native method - hiResIconAvailable
>> that queries if system will be able to provide high resolution icons 
>> for a given file.
>> Now i have tested three approaches with the FileManager:
>> The old one - at magnification 150% can be seen here:
>> http://cr.openjdk.java.net/~kizune/8182043/example/fchooser_old_150.PNG
>> I changed the behavior by making the icon loaded by the FileChooser a 
>> MultiResolutionIcon
>> carrying both small and large icons at the same time.
>> The result looks much better - here's the same view with the 
>> intermediate fix:
>> http://cr.openjdk.java.net/~kizune/8182043/example/fchooser_inter_150.PNG 
>>
>> But then i added the new native method and in FileChooser i'm getting 
>> the multiResolutionIcon
>> for all the files for which it is possible. The result looks much 
>> crisper:
>> http://cr.openjdk.java.net/~kizune/8182043/example/fchooser_new_150.PNG
>> 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?
>>
>> /Alex
>>
>>
>> On 22-Jun-20 11:27, Alexander Zuev wrote:
>>> Hi Alexey,
>>>
>>>   sorry for late responce, after fixing that initial error with the 
>>> wrong
>>> icon sizes pulled i found a lot of corner cases that needed some 
>>> additional digging.
>>>
>>> On 18-Jun-20 20:56, Alexey Ivanov wrote:
>>>> On 18/06/2020 02:30, Sergey Bylokhov wrote:
>>>>> On 6/17/20 5:50 pm, Sergey Bylokhov wrote:
>>>>>
>>>>>> I guess it is too early to compare behavior before and after the 
>>>>>> fix since the fix has a few bugs.
>>>>>>   - It does not fetch several sizes, as it was expected from the 
>>>>>> code, but load only one native size
>>>>>> and map it to the different resolutions.
>>>>
>>>> Yes, I re-read your message and found this mistake.
>>>> I fixed it locally.
>>>>
>>>> Then I noticed JFileChooser does not really use the new method 
>>>> which returns MRI, so this explains why JFileChooser never adjusts 
>>>> the icons according the screen DPI.
>>>>
>>>> I modified Win32ShellFolder2.getIcon(boolean) to use getIcon(size) 
>>>> instead of getIcon(getAbsolutePath(), getLargeIcon), and now the 
>>>> file icons are updated when JFileChooser window is moved from 
>>>> standard DPI monitor to a High DPI one (150%) and back. I noticed 
>>>> one artefact though: the customised icons from disk drives 
>>>> disappeared, some file icons look clipped, on either monitor.
>>> I also noticed this behavior, some icons retrieved with the 
>>> extractIcon method got clipped or improperly scaled.
>>> I tried to dig deeper and found that it happens when 
>>> pIcon->GetIconLocation function returns "*"
>>> as location of the resource file. I haven't found exact reason for 
>>> that but when this is the case then
>>> getIconBits function always retrieves 32x32 icon no matter what icon 
>>> size we requested.
>>> We then scale it down to 16x16 and the result is unpleasant.
>>> I'm trying to find another way of retrieving the custom icon but for 
>>> now i would say that
>>> for the small icons the approach of getIcon function which uses 
>>> SHGetFileInfo holds better results.
>>>
>>> So far the approach that seems to be working is to check if we have 
>>> a high resolution
>>> icon available by requesting icon location. If icon location is not 
>>> known we can get both
>>> low and high resolution icons from SHGetFileInfo and store them in 
>>> the MRI. If high res icon available
>>> then fetch all the standard resolution icons and also return them as 
>>> MRI.
>>>
>>> /Alex
>>
>