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

Tue Mar 31 14:51:48 UTC 2020

Hi Alexey,

   please see my answers inline.

On 31-Mar-20 12:52, Alexey Ivanov wrote:
> Hi Alexander,
>
> I haven't looked at the code thoroughly yet, I'm just replying to 
> already raised concerns. Please see inline:
>
> On 31/03/2020 01:02, Alexander Zuev wrote:
>> He Sergey,
>>
>>   please look for answers inline.
>>
>> On 30-Mar-20 21:35, Sergey Bylokhov wrote:
>>> Hi, Alexander.
>>>
>>> A few initial question before I have look to the webrev:
>>>
>>>  - Why we need a new class for only one method, why we cannot 
>>> enhance the FileSystemView
>>>    where the similar method is implemented already getSystemIcon(File)?
>> As you know this is not the first attempt to fix this issue and when 
>> i asked about "Why do we need
>> a separate class for one new method" the answer was "There is a 
>> reason, we tried different approaches
>> and this one is what we ended up with". Exact reason buried somewhere 
>> in the previous reviews.
>> Personally for me i would prefer adding the new method to some 
>> existing public class.
>> Here's the link to the latest bit of the discussion that i found and 
>> there was exactly the same question
>> raised by Alexey Ivanov, there are some reasons for creating a 
>> separate class:
>> http://mail.openjdk.java.net/pipermail/awt-dev/2019-January/014941.html
>> For some reason the discussion looks incomplete as if some part of it 
>> happened
>> outside of the alias so i can't say what was the outcome - aside of 
>> the fact that last proposed
>> implementation still had a separate class.
>
> As far as I remember there was no clear explanation as to why a new 
> class is needed, none that I'm aware of at the very least.
>
> Initially there were many other helper methods which deemed 
> unnecessary during code review.
>
> I'm for adding the new method into FileSystemView class: it seems 
> logical to me, it just extends the existing functionality.
Ok, noted. While i'm working on that let's clarify the rest of the stuff.
>
>>>  - Can we try to re-implement the places where the old method 
>>> ShellFolder.getIcon(boolean)
>>>    was used, and change it to use the new public API, just to 
>>> confirm that our new code is a
>>>    a good replacement of the old/private api. I guess we could get 
>>> rid the boolean version.
>> It is outside of the initial scope of the request but yes - i can do 
>> it. Should i do it within this fix or
>> should i create a new bug and do it there?
>
> I think we should update implementation of 
> ShellFolder.getIcon(boolean) to use the new API. This way, we'll also 
> test the new API.
That would be a little bit backwards - i mean we are making a new API 
that exposes the new method inside the ShellFolder class and now we want 
to use it in the method within the ShellFolder itself? I would
prefer to avoid such circular dependensies and match the implementation 
of ShellFolder.getIcon
to use same LOGIC as the new API so FileManager can enjoy the new icons.
>
>>>  - The current spec for SystemIcon.getSystemIcon() specify that the 
>>> icon will store the
>>>    "maximum quality icon" what does it meant?
>> It means that the maximum size of the icon allowed by the system will 
>> be used. Right now on
>> Windows (and this issue is Windows specific) the maximum icon size 
>> allowed is 256x256 pixels.
>> That is the size we will request and store in the 
>> MultiResolutionImageIcon.
>
> What if 256×256 icon is not available. Will it result in Windows 
> up-scaling the largest icon for us to 256×256 which we will down-scale 
> to the requested size?
Yes, actually - Windows does scale automatically icons of the different 
sizes to the size requested by
user if such icon does not exist in the file's resources section.
> As I read in your initial note, sizes below 24 are not down-scaled. 
> However, I think we should also make the exception for 32×32 icons 
> too: it's the standard icon size which is also somewhat optimised for 
> this size. 
Well, i did some experiments and it would look almost identical as if we 
request this icon from the system on
100% magnification but as soon as we start changing magnification it 
starts showing the atrifacts of the
upscaling (especially when scale factor is not multiple of 100% - like 
on 130% scaling it looks very bad).
Approach with asking system for 256x256 icon and allowing our UI to 
scale it to exact physical resolution
looks almost the same at 100% magnification and looks way better on 
anything else.
> The size of 48×48 could also be an exception as many applications 
> provide this icon size since Windows XP era.
>
> In general we should use the closest match to the requested size. 
> Unfortunately, Windows does not provide an easy-to-use API which can 
> give you the list of sizes available in the icon. Having the list, we 
> can dynamically request the closest match and cache it, and then up- 
> or down-scale it. This would also work well in High DPI environments 
> with multiple displays: a multi resolution image would have use the 
> right icon size. For example, 16×16 icon at 200% scale is 32×32, so 
> 32×32 icon can be used avoiding scaling up the small icon; or 32×32 
> icon at 150% scale is 48×48, this icon size is is also often available 
> directly.
The problem is that sometimes 16x16 icon looks DIFFERENT from the 32x32 
icon, you can look for it here
http://cr.openjdk.java.net/~kizune/8182043/new.png
Notice that on the top panel the first icon (size 16 folsed icon) looks 
different.
And that's why i made exception for the small icons - they sometimes are 
especially crafted by the
application authors and if we add upscaling/downscaling the file manager 
(for example) will show
application icon differently in the same view on different displays due 
to the different magnification factor.
And scaling 256x256 icon for scaling factors such as 130% or 145% looks 
MUCH better than scaling closest
resolution icon to the exact size (yes, Windows 10 allows custom scaling 
factors to be applied). So i still
think that keeping the maximum quality icon and scaling it down is a 
prefered way for all icons 32px or more.
>
>>>  - Another question is about multi-screen environment, if the 
>>> JFileChooser will be shown
>>>    on the non-HiDPI screen and then moved to the HiDPI screen which 
>>> icons we will request
>>>    from the native and which actual icons(resolution) will we draw 
>>> on each screen?(both types
>>>    of icons large/small are interesting).
>> Right now Swing is handling scaling and with my limited testing 
>> capability (i don't have access to
>> the different multi-monitor configurations for obvious reasons) i 
>> don't see any problem. Both
>> small and large sized icons got scaled together with the scaling 
>> factor change, obviously
>> quality of icons with size 32 and up is much better since they all 
>> are downscale of the 256x256
>> icon and both 16x16 and 24x24 icons are pretty pixelated with scaling 
>> factor 200% and up,
>> but that was a trade off for allowing of the custom small icons to be 
>> used where available.
>>
>> /Alex
>>
>>>
>>> On 3/30/20 4:19 am, Alexander Zuev wrote:
>>>> Hello,
>>>>
>>>>    please review my fix for the issue 8182043: Access to Windows 
>>>> Large Icons
>>>>
>>>>    Bug: https://bugs.openjdk.java.net/browse/JDK-8182043
>>>>    Webrev: http://cr.openjdk.java.net/~kizune/8182043/webrev 
>>>> <http://cr.openjdk.java.net/~kizune/8182043/webrev/>
>>>>
>>>>    Main idea is to provide a new API call to retrieve image of the 
>>>> specified size
>>>> and to make Windows implementation that for all the resolutions 
>>>> higher than 24 pixels
>>>> returns the multi resolution image icon with image inside being the 
>>>> highest quality icon
>>>> available and the size set to the size requested by the user. This 
>>>> way we will have good
>>>> scaling across the different resolution while maintaining relative 
>>>> sizes in the UI intact.
>>>> The exception made for images size of 24 and less since sometimes 
>>>> application has
>>>> different image for the small icons in its resource section which 
>>>> is optimized to
>>>> make sure that on low resolution screen this icon is not displayed 
>>>> as just scaled down
>>>> blurry little square.
>>>>
>>>> /Alex
>>>