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

[Alexey Ivanov]

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
>