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

Tue Sep 26 20:58:37 UTC 2017

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20170926/d44ed4a3/attachment-0001.html>