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

[Alexander Zuev]

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.
>
> 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
>
> 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.
>
>
> 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.
>
> 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.
>
> 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.
> 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
>>>
>>
>