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

Alexey Ivanov alexey.ivanov at oracle.com
Tue Sep 26 19:29:06 UTC 2017


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

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


More information about the awt-dev mailing list