<AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

classic Classic list List threaded Threaded
48 messages Options
123
Reply | Threaded
Open this post in threaded view
|

<AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Semyon Sadetsky
Hello,

Please review fix for JDK10 (the changes involve AWT and Swing):

bug: https://bugs.openjdk.java.net/browse/JDK-8182043

webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/

The fix  opens the part of the ShellFolder API for getting system icons
which was decided to be left closed during the 8081722 enhancement
review in 9.

Also the fix extends the API by adding possibility to query file icons
of arbitrary size and implements this extension for Windows platform.

--Semyon

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Sergey Bylokhov
Hi, Semyon
I have some initial comments which are based on the two bugs:
JDK-8182043 and JDK-8156183.

getSystemIcon(File file, int size):
     - How the user will know what values/sizes should be passed, what
values are supported? It is unlikely that he will pass all values in
between 8-256?

"For any positive size value the exact file icon size is queried":
     - This should be double checked because our implementation can
return MultiResolutionIconImage if the system returns the icon which
size is different from requested.

FILE_ICON_SMALL(FILE_ICON_LARGE);
     - What these parameters mean? Is it the smallest(biggest) supported
size or is it a default size? Can it be different if different dpi are
used on the screen? For example 16(32) by default and 32(64) on HiDPI?

FILE_ICON_SMALL:
    - It seems that this value duplicate functionality of the old
getSystemIcon(File) method?


Probably it will be better to provide to the user the
set(list/mri/array/etc) of supported images, or if it is really slow the
set(list/mri/array/etc) of supported sizes, and the user will be able to
pass some meaningful sizes.


On 9/13/17 11:01, Semyon Sadetsky wrote:

> Hello,
>
> Please review fix for JDK10 (the changes involve AWT and Swing):
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8182043
>
> webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
>
> The fix  opens the part of the ShellFolder API for getting system icons
> which was decided to be left closed during the 8081722 enhancement
> review in 9.
>
> Also the fix extends the API by adding possibility to query file icons
> of arbitrary size and implements this extension for Windows platform.
>
> --Semyon
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Sergey Bylokhov
On 9/21/17 08:54, Semyon Sadetsky wrote:

> On 09/20/2017 02:41 PM, Sergey Bylokhov wrote:
>
>> Hi, Semyon
>> I have some initial comments which are based on the two bugs:
>> JDK-8182043 and JDK-8156183.
>>
>> getSystemIcon(File file, int size):
>>     - How the user will know what values/sizes should be passed, what
>> values are supported? It is unlikely that he will pass all values in
>> between 8-256?
> Supported sizes are described in the method spec, aren't they?
> This API doesn't imply any size limitation like the 8-256 you mentioned.

Does it mean that all sizes will be supported(1-Integer.MAX_INT)? I
guess it is unlikely that the the explorer.exe will contains the icons
bigger than 1024.


>>
>> "For any positive size value the exact file icon size is queried":
>>     - This should be double checked because our implementation can
>> return MultiResolutionIconImage if the system returns the icon which
>> size is different from requested.
>>
>> FILE_ICON_SMALL(FILE_ICON_LARGE);
>>     - What these parameters mean? Is it the smallest(biggest)
>> supported size or is it a default size? Can it be different if
>> different dpi are used on the screen? For example 16(32) by default
>> and 32(64) on HiDPI?
> They means what they have been meaning FileChooserUI implementation for
> the Windows L&F which operates by two fixed icon sizes, large and small.

But it is not necessary will be used by our implementation of
FileChooserUI when this API became public. For example in the
description of JDK-8156183 the user said that large icons are used in "a
media file browser" and "32x32 isn't large by the standards of
current-millennium operating systems".
But even in our FileChooserUI implementation shouldn't we use x2 icons
in case of HiDPI?
FILE_ICON_SMALL - is it a smallest supported size?

>> FILE_ICON_SMALL:
>>    - It seems that this value duplicate functionality of the old
>> getSystemIcon(File) method?
> How this can be got from the spec? It may return the same size but not
> necessarily.

Then how the old method and the new one are related? Will the old method
returns the size in between FILE_ICON_SMALL and FILE_ICON_LARGE?

>>
>>
>> Probably it will be better to provide to the user the
>> set(list/mri/array/etc) of supported images, or if it is really slow
>> the set(list/mri/array/etc) of supported sizes, and the user will be
>> able to pass some meaningful sizes.
> This is not a good idea. Extracting all available icon resolutions might
> take significant time and since icons are cached it would be waste of RAM.

It should be measured, we can try to load them lazy, or provide the list
of sizes which are supported.

>>
>>
>> On 9/13/17 11:01, Semyon Sadetsky wrote:
>>> Hello,
>>>
>>> Please review fix for JDK10 (the changes involve AWT and Swing):
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8182043
>>>
>>> webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
>>>
>>> The fix  opens the part of the ShellFolder API for getting system
>>> icons which was decided to be left closed during the 8081722
>>> enhancement review in 9.
>>>
>>> Also the fix extends the API by adding possibility to query file
>>> icons of arbitrary size and implements this extension for Windows
>>> platform.
>>>
>>> --Semyon
>>>
>>
>>
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Semyon Sadetsky
On 09/21/2017 01:52 PM, Sergey Bylokhov wrote:

> On 9/21/17 08:54, Semyon Sadetsky wrote:
>> On 09/20/2017 02:41 PM, Sergey Bylokhov wrote:
>>
>>> Hi, Semyon
>>> I have some initial comments which are based on the two bugs:
>>> JDK-8182043 and JDK-8156183.
>>>
>>> getSystemIcon(File file, int size):
>>>     - How the user will know what values/sizes should be passed,
>>> what values are supported? It is unlikely that he will pass all
>>> values in between 8-256?
>> Supported sizes are described in the method spec, aren't they?
>> This API doesn't imply any size limitation like the 8-256 you mentioned.
>
> Does it mean that all sizes will be supported(1-Integer.MAX_INT)? I
> guess it is unlikely that the the explorer.exe will contains the icons
> bigger than 1024.
This is a a cross-platform API, not a windows specific implementation.

>
>
>>>
>>> "For any positive size value the exact file icon size is queried":
>>>     - This should be double checked because our implementation can
>>> return MultiResolutionIconImage if the system returns the icon which
>>> size is different from requested.
>>>
>>> FILE_ICON_SMALL(FILE_ICON_LARGE);
>>>     - What these parameters mean? Is it the smallest(biggest)
>>> supported size or is it a default size? Can it be different if
>>> different dpi are used on the screen? For example 16(32) by default
>>> and 32(64) on HiDPI?
>> They means what they have been meaning FileChooserUI implementation
>> for the Windows L&F which operates by two fixed icon sizes, large and
>> small.
>
> But it is not necessary will be used by our implementation of
> FileChooserUI when this API became public. For example in the
> description of JDK-8156183 the user said that large icons are used in
> "a media file browser" and "32x32 isn't large by the standards of
> current-millennium operating systems".
> But even in our FileChooserUI implementation shouldn't we use x2 icons
> in case of HiDPI?
> FILE_ICON_SMALL - is it a smallest supported size?
User may use the new method to get icons at any resolution. Small/large
sizes are preserved for backward compatibility, because before this
change only those two sizes were available.

>
>
>>> FILE_ICON_SMALL:
>>>    - It seems that this value duplicate functionality of the old
>>> getSystemIcon(File) method?
>> How this can be got from the spec? It may return the same size but
>> not necessarily.
>
> Then how the old method and the new one are related? Will the old
> method returns the size in between FILE_ICON_SMALL and FILE_ICON_LARGE?
I didn't get how it would be possible?

>
>>>
>>>
>>> Probably it will be better to provide to the user the
>>> set(list/mri/array/etc) of supported images, or if it is really slow
>>> the set(list/mri/array/etc) of supported sizes, and the user will be
>>> able to pass some meaningful sizes.
>> This is not a good idea. Extracting all available icon resolutions
>> might take significant time and since icons are cached it would be
>> waste of RAM.
>
> It should be measured, we can try to load them lazy, or provide the
> list of sizes which are supported.
Sorry, I didn't get what are you proposing to measure? And how do you
propose to get the icon size without reading the image?

>
>>>
>>>
>>> On 9/13/17 11:01, Semyon Sadetsky wrote:
>>>> Hello,
>>>>
>>>> Please review fix for JDK10 (the changes involve AWT and Swing):
>>>>
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8182043
>>>>
>>>> webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
>>>>
>>>> The fix  opens the part of the ShellFolder API for getting system
>>>> icons which was decided to be left closed during the 8081722
>>>> enhancement review in 9.
>>>>
>>>> Also the fix extends the API by adding possibility to query file
>>>> icons of arbitrary size and implements this extension for Windows
>>>> platform.
>>>>
>>>> --Semyon
>>>>
>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Sergey Bylokhov
On 9/21/17 14:12, Semyon Sadetsky wrote:

> On 09/21/2017 01:52 PM, Sergey Bylokhov wrote:
>
>> On 9/21/17 08:54, Semyon Sadetsky wrote:
>>> On 09/20/2017 02:41 PM, Sergey Bylokhov wrote:
>>>
>>>> Hi, Semyon
>>>> I have some initial comments which are based on the two bugs:
>>>> JDK-8182043 and JDK-8156183.
>>>>
>>>> getSystemIcon(File file, int size):
>>>>     - How the user will know what values/sizes should be passed,
>>>> what values are supported? It is unlikely that he will pass all
>>>> values in between 8-256?
>>> Supported sizes are described in the method spec, aren't they?
>>> This API doesn't imply any size limitation like the 8-256 you mentioned.
>>
>> Does it mean that all sizes will be supported(1-Integer.MAX_INT)? I
>> guess it is unlikely that the the explorer.exe will contains the icons
>> bigger than 1024.
> This is a a cross-platform API, not a windows specific implementation.

This was an example, and the question was how the user will know what
icons are supported by some specific file.

>>
>>
>>>>
>>>> "For any positive size value the exact file icon size is queried":
>>>>     - This should be double checked because our implementation can
>>>> return MultiResolutionIconImage if the system returns the icon which
>>>> size is different from requested.
>>>>
>>>> FILE_ICON_SMALL(FILE_ICON_LARGE);
>>>>     - What these parameters mean? Is it the smallest(biggest)
>>>> supported size or is it a default size? Can it be different if
>>>> different dpi are used on the screen? For example 16(32) by default
>>>> and 32(64) on HiDPI?
>>> They means what they have been meaning FileChooserUI implementation
>>> for the Windows L&F which operates by two fixed icon sizes, large and
>>> small.
>>
>> But it is not necessary will be used by our implementation of
>> FileChooserUI when this API became public. For example in the
>> description of JDK-8156183 the user said that large icons are used in
>> "a media file browser" and "32x32 isn't large by the standards of
>> current-millennium operating systems".
>> But even in our FileChooserUI implementation shouldn't we use x2 icons
>> in case of HiDPI?
>> FILE_ICON_SMALL - is it a smallest supported size?
> User may use the new method to get icons at any resolution. Small/large
> sizes are preserved for backward compatibility, because before this
> change only those two sizes were available.

Backward compatibility to what? There was know public API for this.
It is still unclear what is a "the small or large file icon variant" means.

>>
>>
>>>> FILE_ICON_SMALL:
>>>>    - It seems that this value duplicate functionality of the old
>>>> getSystemIcon(File) method?
>>> How this can be got from the spec? It may return the same size but
>>> not necessarily.
>>
>> Then how the old method and the new one are related? Will the old
>> method returns the size in between FILE_ICON_SMALL and FILE_ICON_LARGE?
> I didn't get how it would be possible?

Possible what? It is unclear how the two methods are related to each other.

>>
>>>>
>>>>
>>>> Probably it will be better to provide to the user the
>>>> set(list/mri/array/etc) of supported images, or if it is really slow
>>>> the set(list/mri/array/etc) of supported sizes, and the user will be
>>>> able to pass some meaningful sizes.
>>> This is not a good idea. Extracting all available icon resolutions
>>> might take significant time and since icons are cached it would be
>>> waste of RAM.
>>
>> It should be measured, we can try to load them lazy, or provide the
>> list of sizes which are supported.
> Sorry, I didn't get what are you proposing to measure? And how do you
> propose to get the icon size without reading the image?
>>
>>>>
>>>>
>>>> On 9/13/17 11:01, Semyon Sadetsky wrote:
>>>>> Hello,
>>>>>
>>>>> Please review fix for JDK10 (the changes involve AWT and Swing):
>>>>>
>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8182043
>>>>>
>>>>> webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
>>>>>
>>>>> The fix  opens the part of the ShellFolder API for getting system
>>>>> icons which was decided to be left closed during the 8081722
>>>>> enhancement review in 9.
>>>>>
>>>>> Also the fix extends the API by adding possibility to query file
>>>>> icons of arbitrary size and implements this extension for Windows
>>>>> platform.
>>>>>
>>>>> --Semyon
>>>>>
>>>>
>>>>
>>>
>>
>>
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Alexey Ivanov
Hi Sergey,

On 22/09/2017 04:21, Sergey Bylokhov wrote:

> On 9/21/17 14:12, Semyon Sadetsky wrote:
>> On 09/21/2017 01:52 PM, Sergey Bylokhov wrote:
>>
>>> On 9/21/17 08:54, Semyon Sadetsky wrote:
>>>> On 09/20/2017 02:41 PM, Sergey Bylokhov wrote:
>>>>
>>>>> Hi, Semyon
>>>>> I have some initial comments which are based on the two bugs:
>>>>> JDK-8182043 and JDK-8156183.
>>>>>
>>>>> getSystemIcon(File file, int size):
>>>>>     - How the user will know what values/sizes should be passed,
>>>>> what values are supported? It is unlikely that he will pass all
>>>>> values in between 8-256?
>>>> Supported sizes are described in the method spec, aren't they?
>>>> This API doesn't imply any size limitation like the 8-256 you
>>>> mentioned.
>>>
>>> Does it mean that all sizes will be supported(1-Integer.MAX_INT)? I
>>> guess it is unlikely that the the explorer.exe will contains the
>>> icons bigger than 1024.
>> This is a a cross-platform API, not a windows specific implementation.
>
> This was an example, and the question was how the user will know what
> icons are supported by some specific file.

There's no way of knowing in advance.
Explorer does not restrict the size of icons (now), it's up to
developers of a particular file handler to provide icons. Usually,
there's only one icon with size larger than 255.

If there's the icon of the requested size, Explorer will give it to you,
otherwise it will scale the closest available to the requested size.

Windows documentation suggests the following sizes:
https://msdn.microsoft.com/en-us/library/windows/desktop/dn742485(v=vs.85).aspx#size_requirements


As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows
API to retrieve the recommended size for small and large icon size
rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the
icons must be larger.


Regards,
Alexey

>
>
>>>
>>>
>>>>>
>>>>> "For any positive size value the exact file icon size is queried":
>>>>>     - This should be double checked because our implementation can
>>>>> return MultiResolutionIconImage if the system returns the icon
>>>>> which size is different from requested.
>>>>>
>>>>> FILE_ICON_SMALL(FILE_ICON_LARGE);
>>>>>     - What these parameters mean? Is it the smallest(biggest)
>>>>> supported size or is it a default size? Can it be different if
>>>>> different dpi are used on the screen? For example 16(32) by
>>>>> default and 32(64) on HiDPI?
>>>> They means what they have been meaning FileChooserUI implementation
>>>> for the Windows L&F which operates by two fixed icon sizes, large
>>>> and small.
>>>
>>> But it is not necessary will be used by our implementation of
>>> FileChooserUI when this API became public. For example in the
>>> description of JDK-8156183 the user said that large icons are used
>>> in "a media file browser" and "32x32 isn't large by the standards of
>>> current-millennium operating systems".
>>> But even in our FileChooserUI implementation shouldn't we use x2
>>> icons in case of HiDPI?
>>> FILE_ICON_SMALL - is it a smallest supported size?
>> User may use the new method to get icons at any resolution.
>> Small/large sizes are preserved for backward compatibility, because
>> before this change only those two sizes were available.
>
> Backward compatibility to what? There was know public API for this.
> It is still unclear what is a "the small or large file icon variant"
> means.
>
>>>
>>>
>>>>> FILE_ICON_SMALL:
>>>>>    - It seems that this value duplicate functionality of the old
>>>>> getSystemIcon(File) method?
>>>> How this can be got from the spec? It may return the same size but
>>>> not necessarily.
>>>
>>> Then how the old method and the new one are related? Will the old
>>> method returns the size in between FILE_ICON_SMALL and FILE_ICON_LARGE?
>> I didn't get how it would be possible?
>
> Possible what? It is unclear how the two methods are related to each
> other.
>
>>>
>>>>>
>>>>>
>>>>> Probably it will be better to provide to the user the
>>>>> set(list/mri/array/etc) of supported images, or if it is really
>>>>> slow the set(list/mri/array/etc) of supported sizes, and the user
>>>>> will be able to pass some meaningful sizes.
>>>> This is not a good idea. Extracting all available icon resolutions
>>>> might take significant time and since icons are cached it would be
>>>> waste of RAM.
>>>
>>> It should be measured, we can try to load them lazy, or provide the
>>> list of sizes which are supported.
>> Sorry, I didn't get what are you proposing to measure? And how do you
>> propose to get the icon size without reading the image?
>>>
>>>>>
>>>>>
>>>>> On 9/13/17 11:01, Semyon Sadetsky wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review fix for JDK10 (the changes involve AWT and Swing):
>>>>>>
>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8182043
>>>>>>
>>>>>> webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
>>>>>>
>>>>>> The fix  opens the part of the ShellFolder API for getting system
>>>>>> icons which was decided to be left closed during the 8081722
>>>>>> enhancement review in 9.
>>>>>>
>>>>>> Also the fix extends the API by adding possibility to query file
>>>>>> icons of arbitrary size and implements this extension for Windows
>>>>>> platform.
>>>>>>
>>>>>> --Semyon
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Semyon Sadetsky
Hi Alexey,

Thank you for your exact clarification.

On 09/22/2017 04:22 AM, Alexey Ivanov wrote:

> Hi Sergey,
>
> On 22/09/2017 04:21, Sergey Bylokhov wrote:
>> On 9/21/17 14:12, Semyon Sadetsky wrote:
>>> On 09/21/2017 01:52 PM, Sergey Bylokhov wrote:
>>>
>>>> On 9/21/17 08:54, Semyon Sadetsky wrote:
>>>>> On 09/20/2017 02:41 PM, Sergey Bylokhov wrote:
>>>>>
>>>>>> Hi, Semyon
>>>>>> I have some initial comments which are based on the two bugs:
>>>>>> JDK-8182043 and JDK-8156183.
>>>>>>
>>>>>> getSystemIcon(File file, int size):
>>>>>>     - How the user will know what values/sizes should be passed,
>>>>>> what values are supported? It is unlikely that he will pass all
>>>>>> values in between 8-256?
>>>>> Supported sizes are described in the method spec, aren't they?
>>>>> This API doesn't imply any size limitation like the 8-256 you
>>>>> mentioned.
>>>>
>>>> Does it mean that all sizes will be supported(1-Integer.MAX_INT)? I
>>>> guess it is unlikely that the the explorer.exe will contains the
>>>> icons bigger than 1024.
>>> This is a a cross-platform API, not a windows specific implementation.
>>
>> This was an example, and the question was how the user will know what
>> icons are supported by some specific file.
>
> There's no way of knowing in advance.
> Explorer does not restrict the size of icons (now), it's up to
> developers of a particular file handler to provide icons. Usually,
> there's only one icon with size larger than 255.
>
> If there's the icon of the requested size, Explorer will give it to
> you, otherwise it will scale the closest available to the requested size.
This I think a general approach for all platforms. Since the icons size
may be set to arbitrarily value in the shell the shell should have a way
to scale to it.

>
> Windows documentation suggests the following sizes:
> https://msdn.microsoft.com/en-us/library/windows/desktop/dn742485(v=vs.85).aspx#size_requirements 
>
>
>
> As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows
> API to retrieve the recommended size for small and large icon size
> rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the
> icons must be larger.
I also found this as most suitable approach for the moment.
Later this may be changed, for example, if Swing JFC is re-factored to
support shell determined icon sizes at HiDPI.

--Semyon

>
>
> Regards,
> Alexey
>
>>
>>
>>>>
>>>>
>>>>>>
>>>>>> "For any positive size value the exact file icon size is queried":
>>>>>>     - This should be double checked because our implementation
>>>>>> can return MultiResolutionIconImage if the system returns the
>>>>>> icon which size is different from requested.
>>>>>>
>>>>>> FILE_ICON_SMALL(FILE_ICON_LARGE);
>>>>>>     - What these parameters mean? Is it the smallest(biggest)
>>>>>> supported size or is it a default size? Can it be different if
>>>>>> different dpi are used on the screen? For example 16(32) by
>>>>>> default and 32(64) on HiDPI?
>>>>> They means what they have been meaning FileChooserUI
>>>>> implementation for the Windows L&F which operates by two fixed
>>>>> icon sizes, large and small.
>>>>
>>>> But it is not necessary will be used by our implementation of
>>>> FileChooserUI when this API became public. For example in the
>>>> description of JDK-8156183 the user said that large icons are used
>>>> in "a media file browser" and "32x32 isn't large by the standards
>>>> of current-millennium operating systems".
>>>> But even in our FileChooserUI implementation shouldn't we use x2
>>>> icons in case of HiDPI?
>>>> FILE_ICON_SMALL - is it a smallest supported size?
>>> User may use the new method to get icons at any resolution.
>>> Small/large sizes are preserved for backward compatibility, because
>>> before this change only those two sizes were available.
>>
>> Backward compatibility to what? There was know public API for this.
>> It is still unclear what is a "the small or large file icon variant"
>> means.
>>
>>>>
>>>>
>>>>>> FILE_ICON_SMALL:
>>>>>>    - It seems that this value duplicate functionality of the old
>>>>>> getSystemIcon(File) method?
>>>>> How this can be got from the spec? It may return the same size but
>>>>> not necessarily.
>>>>
>>>> Then how the old method and the new one are related? Will the old
>>>> method returns the size in between FILE_ICON_SMALL and
>>>> FILE_ICON_LARGE?
>>> I didn't get how it would be possible?
>>
>> Possible what? It is unclear how the two methods are related to each
>> other.
>>
>>>>
>>>>>>
>>>>>>
>>>>>> Probably it will be better to provide to the user the
>>>>>> set(list/mri/array/etc) of supported images, or if it is really
>>>>>> slow the set(list/mri/array/etc) of supported sizes, and the user
>>>>>> will be able to pass some meaningful sizes.
>>>>> This is not a good idea. Extracting all available icon resolutions
>>>>> might take significant time and since icons are cached it would be
>>>>> waste of RAM.
>>>>
>>>> It should be measured, we can try to load them lazy, or provide the
>>>> list of sizes which are supported.
>>> Sorry, I didn't get what are you proposing to measure? And how do
>>> you propose to get the icon size without reading the image?
>>>>
>>>>>>
>>>>>>
>>>>>> On 9/13/17 11:01, Semyon Sadetsky wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Please review fix for JDK10 (the changes involve AWT and Swing):
>>>>>>>
>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8182043
>>>>>>>
>>>>>>> webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
>>>>>>>
>>>>>>> The fix  opens the part of the ShellFolder API for getting
>>>>>>> system icons which was decided to be left closed during the
>>>>>>> 8081722 enhancement review in 9.
>>>>>>>
>>>>>>> Also the fix extends the API by adding possibility to query file
>>>>>>> icons of arbitrary size and implements this extension for
>>>>>>> Windows platform.
>>>>>>>
>>>>>>> --Semyon
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Alexey Ivanov
Hi Semyon,

On 22/09/2017 17:13, Semyon Sadetsky wrote:

> Hi Alexey,
>
> Thank you for your exact clarification.
>
> On 09/22/2017 04:22 AM, Alexey Ivanov wrote:
>> <SNIP>
>>
>> As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows
>> API to retrieve the recommended size for small and large icon size
>> rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the
>> icons must be larger.
> I also found this as most suitable approach for the moment.
> Later this may be changed, for example, if Swing JFC is re-factored to
> support shell determined icon sizes at HiDPI.

Swing UI scales to accommodate HiDPI settings. If fonts are larger then
icons should be larger too. Otherwise icons are too small compared to
surrounding text.

Anyway it could be postponed to a later fix.

Does it make sense to declare the standard sizes of 16×16 and 32×32 as
constants at least in Java sources? This way, it would be easier to find
the places in code where a change is necessary.


Regards,
Alexey

>
>
> --Semyon
>>
>>
>> Regards,
>> Alexey
>>
>>>
>>>> <SNIP>
>>>>>
>>>>>>> On 9/13/17 11:01, Semyon Sadetsky wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> Please review fix for JDK10 (the changes involve AWT and Swing):
>>>>>>>>
>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8182043
>>>>>>>>
>>>>>>>> webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
>>>>>>>>
>>>>>>>> The fix  opens the part of the ShellFolder API for getting
>>>>>>>> system icons which was decided to be left closed during the
>>>>>>>> 8081722 enhancement review in 9.
>>>>>>>>
>>>>>>>> Also the fix extends the API by adding possibility to query
>>>>>>>> file icons of arbitrary size and implements this extension for
>>>>>>>> Windows platform.
>>>>>>>>
>>>>>>>> --Semyon
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Semyon Sadetsky
Hi Alexey,

On 09/22/2017 09:43 AM, Alexey Ivanov wrote:

> Hi Semyon,
>
> On 22/09/2017 17:13, Semyon Sadetsky wrote:
>> Hi Alexey,
>>
>> Thank you for your exact clarification.
>>
>> On 09/22/2017 04:22 AM, Alexey Ivanov wrote:
>>> <SNIP>
>>>
>>> As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using
>>> Windows API to retrieve the recommended size for small and large
>>> icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in
>>> effect, the icons must be larger.
>> I also found this as most suitable approach for the moment.
>> Later this may be changed, for example, if Swing JFC is re-factored
>> to support shell determined icon sizes at HiDPI.
>
> Swing UI scales to accommodate HiDPI settings. If fonts are larger
> then icons should be larger too. Otherwise icons are too small
> compared to surrounding text.
>
> Anyway it could be postponed to a later fix.
>
> Does it make sense to declare the standard sizes of 16×16 and 32×32 as
> constants at least in Java sources? This way, it would be easier to
> find the places in code where a change is necessary.
This topic requires more investigations. At first, we need to keep the
API cross-platform and this requires comparing all supported platforms
in details. At the second, even for the existing windows implementation
there is an ambiguity in icons sizes received form the OS shell. Windows
platform has number of predefined constants to query icon sizes (small,
large, extra large, jumbo...) but their actual size may differ depending
on the shell preferences.
Those icon sizes may be changed by Windows registry settings and may
depend on the hi-res scale. I did several experiments and found that
depending on the way of desktop scaling  in Windows 10 (it has two ways
the new one and the old) at the same scale 2x the returned large icon,
for example,  may be 32 or 64 pixels in size (this was the root cause of
the 8151385  bug).
I would postpone digging in this direction because we are not planning
to update Swing JFC dialog for better HiDPI view in the nearest future.
Also,we don't have statistics how users may use the API. Since that, the
most flexible API that leaves to the user the decision about icon size
to query seems more preferable.

--Semyon

>
>
> Regards,
> Alexey
>
>>
>>
>> --Semyon
>>>
>>>
>>> Regards,
>>> Alexey
>>>
>>>>
>>>>> <SNIP>
>>>>>>
>>>>>>>> On 9/13/17 11:01, Semyon Sadetsky wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> Please review fix for JDK10 (the changes involve AWT and Swing):
>>>>>>>>>
>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8182043
>>>>>>>>>
>>>>>>>>> webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
>>>>>>>>>
>>>>>>>>> The fix  opens the part of the ShellFolder API for getting
>>>>>>>>> system icons which was decided to be left closed during the
>>>>>>>>> 8081722 enhancement review in 9.
>>>>>>>>>
>>>>>>>>> Also the fix extends the API by adding possibility to query
>>>>>>>>> file icons of arbitrary size and implements this extension for
>>>>>>>>> Windows platform.
>>>>>>>>>
>>>>>>>>> --Semyon
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Alexey Ivanov
Hi Semyon,

On 22/09/2017 18:29, Semyon Sadetsky wrote:

> Hi Alexey,
>
> On 09/22/2017 09:43 AM, Alexey Ivanov wrote:
>> Hi Semyon,
>>
>> On 22/09/2017 17:13, Semyon Sadetsky wrote:
>>> Hi Alexey,
>>>
>>> Thank you for your exact clarification.
>>>
>>> On 09/22/2017 04:22 AM, Alexey Ivanov wrote:
>>>> <SNIP>
>>>>
>>>> As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using
>>>> Windows API to retrieve the recommended size for small and large
>>>> icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in
>>>> effect, the icons must be larger.
>>> I also found this as most suitable approach for the moment.
>>> Later this may be changed, for example, if Swing JFC is re-factored
>>> to support shell determined icon sizes at HiDPI.
>>
>> Swing UI scales to accommodate HiDPI settings. If fonts are larger
>> then icons should be larger too. Otherwise icons are too small
>> compared to surrounding text.
>>
>> Anyway it could be postponed to a later fix.
>>
>> Does it make sense to declare the standard sizes of 16×16 and 32×32
>> as constants at least in Java sources? This way, it would be easier
>> to find the places in code where a change is necessary.
> This topic requires more investigations. At first, we need to keep the
> API cross-platform and this requires comparing all supported platforms
> in details. At the second, even for the existing windows
> implementation there is an ambiguity in icons sizes received form the
> OS shell. Windows platform has number of predefined constants to query
> icon sizes (small, large, extra large, jumbo...) but their actual size
> may differ depending on the shell preferences.
> Those icon sizes may be changed by Windows registry settings and may
> depend on the hi-res scale. I did several experiments and found that
> depending on the way of desktop scaling  in Windows 10 (it has two
> ways the new one and the old) at the same scale 2x the returned large
> icon, for example,  may be 32 or 64 pixels in size (this was the root
> cause of the 8151385  bug).
> I would postpone digging in this direction because we are not planning
> to update Swing JFC dialog for better HiDPI view in the nearest
> future. Also,we don't have statistics how users may use the API. Since
> that, the most flexible API that leaves to the user the decision about
> icon size to query seems more preferable.

I totally agree with your points. And the new API provides means for app
developer to choose better-suited size for icons.

What about using constants, private ones, for the two standard sizes
instead of using “magic” numbers?
Other than that, the fix looks good to me.


Regards,
Alexey

>
>
> --Semyon
>>
>>
>> Regards,
>> Alexey
>>
>>>
>>>
>>> --Semyon
>>>>
>>>>
>>>> Regards,
>>>> Alexey
>>>>
>>>>>
>>>>>> <SNIP>
>>>>>>>
>>>>>>>>> On 9/13/17 11:01, Semyon Sadetsky wrote:
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> Please review fix for JDK10 (the changes involve AWT and Swing):
>>>>>>>>>>
>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8182043
>>>>>>>>>>
>>>>>>>>>> webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
>>>>>>>>>>
>>>>>>>>>> The fix  opens the part of the ShellFolder API for getting
>>>>>>>>>> system icons which was decided to be left closed during the
>>>>>>>>>> 8081722 enhancement review in 9.
>>>>>>>>>>
>>>>>>>>>> Also the fix extends the API by adding possibility to query
>>>>>>>>>> file icons of arbitrary size and implements this extension
>>>>>>>>>> for Windows platform.
>>>>>>>>>>
>>>>>>>>>> --Semyon
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Semyon Sadetsky

Hi Alexey,

On 09/22/2017 10:53 AM, Alexey Ivanov wrote:
Hi Semyon,

On 22/09/2017 18:29, Semyon Sadetsky wrote:
Hi Alexey,

On 09/22/2017 09:43 AM, Alexey Ivanov wrote:
Hi Semyon,

On 22/09/2017 17:13, Semyon Sadetsky wrote:
Hi Alexey,

Thank you for your exact clarification.

On 09/22/2017 04:22 AM, Alexey Ivanov wrote:
<SNIP>

As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows API to retrieve the recommended size for small and large icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the icons must be larger.
I also found this as most suitable approach for the moment.
Later this may be changed, for example, if Swing JFC is re-factored to support shell determined icon sizes at HiDPI.

Swing UI scales to accommodate HiDPI settings. If fonts are larger then icons should be larger too. Otherwise icons are too small compared to surrounding text.

Anyway it could be postponed to a later fix.

Does it make sense to declare the standard sizes of 16×16 and 32×32 as constants at least in Java sources? This way, it would be easier to find the places in code where a change is necessary.
This topic requires more investigations. At first, we need to keep the API cross-platform and this requires comparing all supported platforms in details. At the second, even for the existing windows implementation there is an ambiguity in icons sizes received form the OS shell. Windows platform has number of predefined constants to query icon sizes (small, large, extra large, jumbo...) but their actual size may differ depending on the shell preferences.
Those icon sizes may be changed by Windows registry settings and may depend on the hi-res scale. I did several experiments and found that depending on the way of desktop scaling  in Windows 10 (it has two ways the new one and the old) at the same scale 2x the returned large icon, for example,  may be 32 or 64 pixels in size (this was the root cause of the 8151385  bug).
I would postpone digging in this direction because we are not planning to update Swing JFC dialog for better HiDPI view in the nearest future. Also,we don't have statistics how users may use the API. Since that, the most flexible API that leaves to the user the decision about icon size to query seems more preferable.

I totally agree with your points. And the new API provides means for app developer to choose better-suited size for icons.

What about using constants, private ones, for the two standard sizes instead of using “magic” numbers?
Which constants do you mean? The next for large and small sizes?

public static final int FILE_ICON_SMALL = -1;
public static final int FILE_ICON_LARGE = -2;
they cannot be private because they are part of the new API that is requested in the bug. The bug asks enabling the query for the large icon that ShellFolder had been providing before. The bug itself is not related to HiDPI. The possibility to get arbitrary icon sizes was added because after 9 this may be in demand for HiDPI UIs.

--Semyon

Other than that, the fix looks good to me.


Regards,
Alexey



--Semyon


Regards,
Alexey



--Semyon


Regards,
Alexey


<SNIP>

On 9/13/17 11:01, Semyon Sadetsky wrote:
Hello,

Please review fix for JDK10 (the changes involve AWT and Swing):

bug: https://bugs.openjdk.java.net/browse/JDK-8182043

webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/

The fix  opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9.

Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform.

--Semyon















Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Semyon Sadetsky

Hi Alexey,

On 09/22/2017 01:01 PM, Alexey Ivanov wrote:
Hi Semyon,

On 22/09/2017 20:06, Semyon Sadetsky wrote:

Hi Alexey,

On 09/22/2017 10:53 AM, Alexey Ivanov wrote:
Hi Semyon,

On 22/09/2017 18:29, Semyon Sadetsky wrote:
Hi Alexey,

On 09/22/2017 09:43 AM, Alexey Ivanov wrote:
Hi Semyon,

On 22/09/2017 17:13, Semyon Sadetsky wrote:
Hi Alexey,

Thank you for your exact clarification.

On 09/22/2017 04:22 AM, Alexey Ivanov wrote:
<SNIP>

As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows API to retrieve the recommended size for small and large icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the icons must be larger.
I also found this as most suitable approach for the moment.
Later this may be changed, for example, if Swing JFC is re-factored to support shell determined icon sizes at HiDPI.

Swing UI scales to accommodate HiDPI settings. If fonts are larger then icons should be larger too. Otherwise icons are too small compared to surrounding text.

Anyway it could be postponed to a later fix.

Does it make sense to declare the standard sizes of 16×16 and 32×32 as constants at least in Java sources? This way, it would be easier to find the places in code where a change is necessary.
This topic requires more investigations. At first, we need to keep the API cross-platform and this requires comparing all supported platforms in details. At the second, even for the existing windows implementation there is an ambiguity in icons sizes received form the OS shell. Windows platform has number of predefined constants to query icon sizes (small, large, extra large, jumbo...) but their actual size may differ depending on the shell preferences.
Those icon sizes may be changed by Windows registry settings and may depend on the hi-res scale. I did several experiments and found that depending on the way of desktop scaling  in Windows 10 (it has two ways the new one and the old) at the same scale 2x the returned large icon, for example,  may be 32 or 64 pixels in size (this was the root cause of the 8151385  bug).
I would postpone digging in this direction because we are not planning to update Swing JFC dialog for better HiDPI view in the nearest future. Also,we don't have statistics how users may use the API. Since that, the most flexible API that leaves to the user the decision about icon size to query seems more preferable.

I totally agree with your points. And the new API provides means for app developer to choose better-suited size for icons.

What about using constants, private ones, for the two standard sizes instead of using “magic” numbers?
Which constants do you mean? The next for large and small sizes?

public static final int FILE_ICON_SMALL = -1;
public static final int FILE_ICON_LARGE = -2;
they cannot be private because they are part of the new API that is requested in the bug. The bug asks enabling the query for the large icon that ShellFolder had been providing before. The bug itself is not related to HiDPI. The possibility to get arbitrary icon sizes was added because after 9 this may be in demand for HiDPI UIs.

I'm talking about these two numbers in Win32ShellFolder2.java as an example:

   public Image getIcon(final boolean getLargeIcon) {
       int size = getLargeIcon ? 32 : 16;

Does it make sense to declare constants for the size of 16 and 32. So that the places where they're used are more easily identified if someone decides to update the code later for supporting system icon size.
I updated the fix.

http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/

--Semyon


Regards,
Alexey


--Semyon

Other than that, the fix looks good to me.


Regards,
Alexey



--Semyon


Regards,
Alexey



--Semyon


Regards,
Alexey


<SNIP>

On 9/13/17 11:01, Semyon Sadetsky wrote:
Hello,

Please review fix for JDK10 (the changes involve AWT and Swing):

bug: https://bugs.openjdk.java.net/browse/JDK-8182043

webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/

The fix  opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9.

Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform.

--Semyon

















Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Sergey Bylokhov
In reply to this post by Alexey Ivanov
On 9/22/17 04:22, Alexey Ivanov wrote:

> There's no way of knowing in advance.
> Explorer does not restrict the size of icons (now), it's up to
> developers of a particular file handler to provide icons. Usually,
> there's only one icon with size larger than 255.
>
> If there's the icon of the requested size, Explorer will give it to you,
> otherwise it will scale the closest available to the requested size.
>
> Windows documentation suggests the following sizes:
> https://msdn.microsoft.com/en-us/library/windows/desktop/dn742485(v=vs.85).aspx#size_requirements 

Ok, so it means that we will support 1-128 pixels
natively(MAX_ICON_SIZE) and others via MRI.

>
> As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows
> API to retrieve the recommended size for small and large icon size
> rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the
> icons must be larger.

But this depends from the the DPI of the screen, we cannot just request
default FILE_ICON_SMALL/FILE_ICON_LARGE. If these constants will be
added then we should use something like this to get correct icons:

Icon icon = getSystemIcon(file, FILE_ICON_SMALL);
Icon hicon = getSystemIcon(file, icon.getIconWidth()*screenScale);
or
Icon icon = getSystemIcon(file);
Icon hicon = getSystemIcon(file, icon.getIconWidth()*screenScale);
Or we can do:
Icon hicon = getSystemIcon(file, FILE_ICON_LARGE);

This means that on HiDPI screen the FILE_ICON_LARGE works in similar way
as FILE_ICON_SMALL on non-HiDPI screen, and the meaning of the
FILE_ICON_SMALL on HiDPI is unclear, because it is half of the correct size.

For example one of the consumer of this new API is WindowsFileView.
How the code below should be changed to work on a different screens, and
request the proper icon?
WindowsFileChooserUI.java
1316 icon = getFileChooser().getFileSystemView().getSystemIcon(f);


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Alexey Ivanov
In reply to this post by Semyon Sadetsky
Hi Semyon,

ShellFolder2.cpp
944             pIcon->Extract(szBuf, index, &hIcon, 0, sz);
I think 0 should really be NULL.
The result of the call is ignored now. Is it intentional?

Win32ShellFolder2.java
1010     private static Image makeIcon(long hIcon, int bsize) {
bsize was called baseSize previously, and it conveyed the meaning better, didn't it?

1043                 if(hIcon <= 0) {
1044                     if (isDirectory()) {
1045                         return getShell32Icon(4, size);
1046                     } else {
1047                         return getShell32Icon(1, size);
1048                     }
I guess I understand what hides behind 4 and 1: generic folder and generic file icon ids. Would declaring these numbers as constants improve readability?

getIcon(int size) and getIcon(boolean getLargeIcon) are somewhat similar. The latter provides additional caching. Can it be simplified to re-use portions of new getIcon(int size)?


Regards,
Alexey

On 22/09/2017 22:05, Semyon Sadetsky wrote:

Hi Alexey,

On 09/22/2017 01:01 PM, Alexey Ivanov wrote:
Hi Semyon,

On 22/09/2017 20:06, Semyon Sadetsky wrote:

Hi Alexey,

On 09/22/2017 10:53 AM, Alexey Ivanov wrote:
Hi Semyon,

On 22/09/2017 18:29, Semyon Sadetsky wrote:
Hi Alexey,

On 09/22/2017 09:43 AM, Alexey Ivanov wrote:
Hi Semyon,

On 22/09/2017 17:13, Semyon Sadetsky wrote:
Hi Alexey,

Thank you for your exact clarification.

On 09/22/2017 04:22 AM, Alexey Ivanov wrote:
<SNIP>

As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows API to retrieve the recommended size for small and large icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the icons must be larger.
I also found this as most suitable approach for the moment.
Later this may be changed, for example, if Swing JFC is re-factored to support shell determined icon sizes at HiDPI.

Swing UI scales to accommodate HiDPI settings. If fonts are larger then icons should be larger too. Otherwise icons are too small compared to surrounding text.

Anyway it could be postponed to a later fix.

Does it make sense to declare the standard sizes of 16×16 and 32×32 as constants at least in Java sources? This way, it would be easier to find the places in code where a change is necessary.
This topic requires more investigations. At first, we need to keep the API cross-platform and this requires comparing all supported platforms in details. At the second, even for the existing windows implementation there is an ambiguity in icons sizes received form the OS shell. Windows platform has number of predefined constants to query icon sizes (small, large, extra large, jumbo...) but their actual size may differ depending on the shell preferences.
Those icon sizes may be changed by Windows registry settings and may depend on the hi-res scale. I did several experiments and found that depending on the way of desktop scaling  in Windows 10 (it has two ways the new one and the old) at the same scale 2x the returned large icon, for example,  may be 32 or 64 pixels in size (this was the root cause of the 8151385  bug).
I would postpone digging in this direction because we are not planning to update Swing JFC dialog for better HiDPI view in the nearest future. Also,we don't have statistics how users may use the API. Since that, the most flexible API that leaves to the user the decision about icon size to query seems more preferable.

I totally agree with your points. And the new API provides means for app developer to choose better-suited size for icons.

What about using constants, private ones, for the two standard sizes instead of using “magic” numbers?
Which constants do you mean? The next for large and small sizes?

public static final int FILE_ICON_SMALL = -1;
public static final int FILE_ICON_LARGE = -2;
they cannot be private because they are part of the new API that is requested in the bug. The bug asks enabling the query for the large icon that ShellFolder had been providing before. The bug itself is not related to HiDPI. The possibility to get arbitrary icon sizes was added because after 9 this may be in demand for HiDPI UIs.

I'm talking about these two numbers in Win32ShellFolder2.java as an example:

   public Image getIcon(final boolean getLargeIcon) {
       int size = getLargeIcon ? 32 : 16;

Does it make sense to declare constants for the size of 16 and 32. So that the places where they're used are more easily identified if someone decides to update the code later for supporting system icon size.
I updated the fix.

http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/

--Semyon


Regards,
Alexey


--Semyon

Other than that, the fix looks good to me.


Regards,
Alexey



--Semyon


Regards,
Alexey



--Semyon


Regards,
Alexey


<SNIP>

On 9/13/17 11:01, Semyon Sadetsky wrote:
Hello,

Please review fix for JDK10 (the changes involve AWT and Swing):

bug: https://bugs.openjdk.java.net/browse/JDK-8182043

webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/

The fix  opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9.

Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform.

--Semyon


















Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Semyon Sadetsky
Hi Alexey,

On 9/26/2017 12:29 PM, Alexey Ivanov wrote:
Hi Semyon,

ShellFolder2.cpp
944             pIcon->Extract(szBuf, index, &hIcon, 0, sz);
I think 0 should really be NULL.
Ok, but both represent the null pointer in Win32.

The result of the call is ignored now. Is it intentional?
Yes, it has been working the same way before the fix. The function returns the Icon reference which is 0 in case of OS API call error.


Win32ShellFolder2.java
1010     private static Image makeIcon(long hIcon, int bsize) {
bsize was called baseSize previously, and it conveyed the meaning better, didn't it?

1043                 if(hIcon <= 0) {
1044                     if (isDirectory()) {
1045                         return getShell32Icon(4, size);
1046                     } else {
1047                         return getShell32Icon(1, size);
1048                     }
I guess I understand what hides behind 4 and 1: generic folder and generic file icon ids. Would declaring these numbers as constants improve readability?
Ok.

getIcon(int size) and getIcon(boolean getLargeIcon) are somewhat similar. The latter provides additional caching. Can it be simplified to re-use portions of new getIcon(int size)?
It has additional difference because of query for a fixed icon size from another API call.  It's better to leave it as it is.

http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.02/

--Semyon


Regards,
Alexey

On 22/09/2017 22:05, Semyon Sadetsky wrote:

Hi Alexey,

On 09/22/2017 01:01 PM, Alexey Ivanov wrote:
Hi Semyon,

On 22/09/2017 20:06, Semyon Sadetsky wrote:

Hi Alexey,

On 09/22/2017 10:53 AM, Alexey Ivanov wrote:
Hi Semyon,

On 22/09/2017 18:29, Semyon Sadetsky wrote:
Hi Alexey,

On 09/22/2017 09:43 AM, Alexey Ivanov wrote:
Hi Semyon,

On 22/09/2017 17:13, Semyon Sadetsky wrote:
Hi Alexey,

Thank you for your exact clarification.

On 09/22/2017 04:22 AM, Alexey Ivanov wrote:
<SNIP>

As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows API to retrieve the recommended size for small and large icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the icons must be larger.
I also found this as most suitable approach for the moment.
Later this may be changed, for example, if Swing JFC is re-factored to support shell determined icon sizes at HiDPI.

Swing UI scales to accommodate HiDPI settings. If fonts are larger then icons should be larger too. Otherwise icons are too small compared to surrounding text.

Anyway it could be postponed to a later fix.

Does it make sense to declare the standard sizes of 16×16 and 32×32 as constants at least in Java sources? This way, it would be easier to find the places in code where a change is necessary.
This topic requires more investigations. At first, we need to keep the API cross-platform and this requires comparing all supported platforms in details. At the second, even for the existing windows implementation there is an ambiguity in icons sizes received form the OS shell. Windows platform has number of predefined constants to query icon sizes (small, large, extra large, jumbo...) but their actual size may differ depending on the shell preferences.
Those icon sizes may be changed by Windows registry settings and may depend on the hi-res scale. I did several experiments and found that depending on the way of desktop scaling  in Windows 10 (it has two ways the new one and the old) at the same scale 2x the returned large icon, for example,  may be 32 or 64 pixels in size (this was the root cause of the 8151385  bug).
I would postpone digging in this direction because we are not planning to update Swing JFC dialog for better HiDPI view in the nearest future. Also,we don't have statistics how users may use the API. Since that, the most flexible API that leaves to the user the decision about icon size to query seems more preferable.

I totally agree with your points. And the new API provides means for app developer to choose better-suited size for icons.

What about using constants, private ones, for the two standard sizes instead of using “magic” numbers?
Which constants do you mean? The next for large and small sizes?

public static final int FILE_ICON_SMALL = -1;
public static final int FILE_ICON_LARGE = -2;
they cannot be private because they are part of the new API that is requested in the bug. The bug asks enabling the query for the large icon that ShellFolder had been providing before. The bug itself is not related to HiDPI. The possibility to get arbitrary icon sizes was added because after 9 this may be in demand for HiDPI UIs.

I'm talking about these two numbers in Win32ShellFolder2.java as an example:

   public Image getIcon(final boolean getLargeIcon) {
       int size = getLargeIcon ? 32 : 16;

Does it make sense to declare constants for the size of 16 and 32. So that the places where they're used are more easily identified if someone decides to update the code later for supporting system icon size.
I updated the fix.

http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/

--Semyon


Regards,
Alexey


--Semyon

Other than that, the fix looks good to me.


Regards,
Alexey



--Semyon


Regards,
Alexey



--Semyon


Regards,
Alexey


<SNIP>

On 9/13/17 11:01, Semyon Sadetsky wrote:
Hello,

Please review fix for JDK10 (the changes involve AWT and Swing):

bug: https://bugs.openjdk.java.net/browse/JDK-8182043

webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/

The fix  opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9.

Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform.

--Semyon



















Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Semyon Sadetsky
In reply to this post by Sergey Bylokhov
Hi Sergey,

On 9/25/2017 1:44 PM, Sergey Bylokhov wrote:

> On 9/22/17 04:22, Alexey Ivanov wrote:
>> There's no way of knowing in advance.
>> Explorer does not restrict the size of icons (now), it's up to
>> developers of a particular file handler to provide icons. Usually,
>> there's only one icon with size larger than 255.
>>
>> If there's the icon of the requested size, Explorer will give it to
>> you, otherwise it will scale the closest available to the requested
>> size.
>>
>> Windows documentation suggests the following sizes:
>> https://msdn.microsoft.com/en-us/library/windows/desktop/dn742485(v=vs.85).aspx#size_requirements 
>
>
> Ok, so it means that we will support 1-128 pixels
> natively(MAX_ICON_SIZE) and others via MRI.
We will support all sizes natively not only 1-128. Windows does the scaling.

>
>>
>> As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows
>> API to retrieve the recommended size for small and large icon size
>> rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the
>> icons must be larger.
>
> But this depends from the the DPI of the screen, we cannot just
> request default FILE_ICON_SMALL/FILE_ICON_LARGE. If these constants
> will be added then we should use something like this to get correct
> icons:
>
> Icon icon = getSystemIcon(file, FILE_ICON_SMALL);
> Icon hicon = getSystemIcon(file, icon.getIconWidth()*screenScale);
> or
> Icon icon = getSystemIcon(file);
> Icon hicon = getSystemIcon(file, icon.getIconWidth()*screenScale);
> Or we can do:
> Icon hicon = getSystemIcon(file, FILE_ICON_LARGE);
>
> This means that on HiDPI screen the FILE_ICON_LARGE works in similar
> way as FILE_ICON_SMALL on non-HiDPI screen, and the meaning of the
> FILE_ICON_SMALL on HiDPI is unclear, because it is half of the correct
> size.
Small and large don't have any special meanings for HiDPI. They are some
conditional sizes established by the native platform for the current
screen resolution.
Why is it half of the correct size? It is the same size as for non-HiDPI
and that is the correct size because otherwise java UI component that is
HiDPI unaware would paint icons 2 times bigger in size than it is
required. But the resolution of small/large icons may differs in case of
HiDPI because it is determined by the size of the images returned by the
native platform by the small/large icon queries.
>
> For example one of the consumer of this new API is WindowsFileView.
> How the code below should be changed to work on a different screens,
> and request the proper icon?
> WindowsFileChooserUI.java
> 1316 icon = getFileChooser().getFileSystemView().getSystemIcon(f);
Why it should be changed? The code is requesting the proper icon.

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Sergey Bylokhov
On 9/26/17 14:37, Semyon Sadetsky wrote:
>> This means that on HiDPI screen the FILE_ICON_LARGE works in similar
>> way as FILE_ICON_SMALL on non-HiDPI screen, and the meaning of the
>> FILE_ICON_SMALL on HiDPI is unclear, because it is half of the correct
>> size.
> Small and large don't have any special meanings for HiDPI. They are some
> conditional sizes established by the native platform for the current
> screen resolution.

The question what is the current screens resolution when you have two
screens?

> Why is it half of the correct size? It is the same size as for non-HiDPI
> and that is the correct size because otherwise java UI component that is
> HiDPI unaware would paint icons 2 times bigger in size than it is
> required. But the resolution of small/large icons may differs in case of
> HiDPI because it is determined by the size of the images returned by the
> native platform by the small/large icon queries.

It is half of the correct size because on HiDPI it is better to use
hidpi icons instead of lowdpi. Will the HiDPI unaware apps draw x2 icons
correctly or not depends from our implementation. If we will return the
MRI then it will be drawn in correct size even if the bigger(HiDPI)
image will be used.

>>
>> For example one of the consumer of this new API is WindowsFileView.
>> How the code below should be changed to work on a different screens,
>> and request the proper icon?
>> WindowsFileChooserUI.java
>> 1316 icon = getFileChooser().getFileSystemView().getSystemIcon(f);
> Why it should be changed? The code is requesting the proper icon.

Because this method returns an icon of 16x16 pixels, which will be
rescaled to 32x32 pixels in paint operation.


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Alexey Ivanov
In reply to this post by Semyon Sadetsky
Hi Semyon,

On 26/09/2017 21:58, Semyon Sadetsky wrote:
Hi Alexey,

On 9/26/2017 12:29 PM, Alexey Ivanov wrote:
Hi Semyon,

ShellFolder2.cpp
944             pIcon->Extract(szBuf, index, &hIcon, 0, sz);
I think 0 should really be NULL.
Ok, but both represent the null pointer in Win32.

Yes, I know. Yet NULL makes it clear that it's a null-pointer rather than an index, size…
It's still zero in the latest review.


The result of the call is ignored now. Is it intentional?
Yes, it has been working the same way before the fix. The function returns the Icon reference which is 0 in case of OS API call error.


Win32ShellFolder2.java
1010     private static Image makeIcon(long hIcon, int bsize) {
bsize was called baseSize previously, and it conveyed the meaning better, didn't it?

1043                 if(hIcon <= 0) {
1044                     if (isDirectory()) {
1045                         return getShell32Icon(4, size);
1046                     } else {
1047                         return getShell32Icon(1, size);
1048                     }
I guess I understand what hides behind 4 and 1: generic folder and generic file icon ids. Would declaring these numbers as constants improve readability?
Ok.

getIcon(int size) and getIcon(boolean getLargeIcon) are somewhat similar. The latter provides additional caching. Can it be simplified to re-use portions of new getIcon(int size)?
It has additional difference because of query for a fixed icon size from another API call.  It's better to leave it as it is.

Okay.


Regards,
Alexey


http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.02/

--Semyon


Regards,
Alexey

On 22/09/2017 22:05, Semyon Sadetsky wrote:

<SNIP>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Semyon Sadetsky
Hi Alexey,

On 9/28/2017 7:28 AM, Alexey Ivanov wrote:
I think 0 should really be NULL.
Ok, but both represent the null pointer in Win32.

Yes, I know. Yet NULL makes it clear that it's a null-pointer rather than an index, size…
It's still zero in the latest review.
Agh... I will fix it inline when push if you don't mind.

--Semyon

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> <Swing Dev> [10] Review request for 8182043: Access to Windows Large Icons

Alexey Ivanov
Sure!

--
Alexey

On 28/09/2017 18:41, Semyon Sadetsky wrote:
Hi Alexey,

On 9/28/2017 7:28 AM, Alexey Ivanov wrote:
I think 0 should really be NULL.
Ok, but both represent the null pointer in Win32.

Yes, I know. Yet NULL makes it clear that it's a null-pointer rather than an index, size…
It's still zero in the latest review.
Agh... I will fix it inline when push if you don't mind.

--Semyon


123