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

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

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

Semyon Sadetsky
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.

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

>
>
> 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: <Swing Dev> <AWT Dev> [10] Review request for 8182043: Access to Windows Large Icons

Alexey Ivanov
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.


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: <Swing Dev> <AWT Dev> [10] Review request for 8182043: Access to Windows Large Icons

Phil Race
If this is up for consideration for backporting - as appears to be the case - then
I think you should post an updated webrev.

-phil.

On 9/28/17, 10:41 AM, 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

Reply | Threaded
Open this post in threaded view
|

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

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

On 28/09/2017 19:49, Sergey Bylokhov wrote:
On 9/28/17 10:57, Semyon Sadetsky wrote:
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?
We should relay on the native platform always. So, the icon size returned by the native API is the correct approach. And possibility to use any other sizes is provided as well.

On windows and Linux we cannot rely on the native system because all HiDPI support is implemented on our(jdk) side, the native system does not know how this icons are used.

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.
This is not true. MRI has a basic size which uniquely determines its painted size in component coordinates. The size painted in component will be the same for all scales this is how HiDPI works in java.

The size in a component coordinates will be the same, but if HiDPI image is used the real number of pixels to be drawn will be 4 times bigger, because low-dpi images will be scaled x2 and HiDPI images will be drawn as is.

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.
The size should be equal 16x16 otherwise the component view will be distorted. But painted resolution is determined by native platform. The native platform may return icon of any size. If the size of the icon differs from 16x16 (32x32 for example) then it will be wrapped by MRI of 16x16 size.

The draw resolution cannot be calculated by the native platform for each window in java. The windows provide a set of icons for each resolution, and we should select correct one depending from the scalefactor of our window. And we should draw bigger icons when the bigger dpi is in use.

from the example above the code in WindowsFileChooserUI will use low-dpi icons on a HiDPI screen:
1316 icon = getFileChooser().getFileSystemView().getSystemIcon(f);

How we should rewrite this code using a new API to support the icons which are large than default?

If I understand correctly, the introduction of the new API does not change the behaviour in this case, does it?

The icon extracted from Windows was 16×16 and will continue to be used.
That is the icon will be scaled when painted.

To support HiDPI displays the implementation of getFileChooser().getFileSystemView().getSystemIcon(f) should be enhanced to fetch the icon at the appropriate size according to the current rendering scale. I mean in standard resolution, get 16×16 icon, for 125% scale factor, get 20×20 icon, for 150% scale factor, get icon 24×24. (As far as I know, IExtractIcon::Extract does not perform any scaling to account for HiDPI rendering. And it really can't because different displays can have different DPI settings. Thus if you request icon size of 16×16, you'll get 16×16 icon, not 32×32 even if this size is more appropriate to the current HiDPI scaling.)

Different icon sizes could be combined into MRI lazily.
To support it, we could use different APIs to extract the icons. For example, we can get the list of icon sizes for a file type, and extract only “native” size. If larger size is required for HiDPI and the icon has it, then get the larger version and use it for rendering rather than scaling the one we already have.

However, it looks to be out of the scope for this particular fix. Yet this approach will make UI look crispier at higher resolutions.


Regards,
Alexey
Reply | Threaded
Open this post in threaded view
|

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

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

On 29/09/2017 20:39, Semyon Sadetsky wrote:

>
>
> On 9/29/2017 12:29 PM, Sergey Bylokhov wrote:
>> On 9/29/17 07:34, Alexey Ivanov wrote:
>>>> Ok, so it means that we will support 1-128 pixels
>>>> natively(MAX_ICON_SIZE) and others via MRI.
>>>
>>> Why 128 pixels? Windows shell usually provides icons up to 256
>>> pixels, for example there are 256×256 icons for folders and generic
>>> file type.
>>
>> It is limitation of our implementation:
>> https://bugs.openjdk.java.net/browse/JDK-8151385
>> http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html
>>
> Sergey, it is not clear how those links are related to the icon size
> returned by Windows?
>>>
>>> Since |IExtractIcon::Extract| gives you the requested size,
>>> performing scaling if required, then MRI will never be crea
>>
>> As far as I understand the bug above, it is possible that OS returns
>> some other size.
> You've probably didn't understand what Alexey meant. The Extract call
> may return any size you request (it does scaling internally if there
> are no suitable image).

Yes, that's what I meant. IExtractIcon::Extract always returns the size
you requested, if necessary it does internal scaling.

> But the bug above is about queering the fixed size (small or long)
> which size is determined by OS shell according to the current scale.
> For those fixed sizes we use SHGetFileInfo not the Extract.

However, I'm afraid IExtractIcon::Extract does not scale the icon to the
current DPI scaling, it's the task of the programmer to request the icon
size that's required in current scaling settings to make sure the icon
is crisp.

Regards,
Alexey
>
>
> --Semyon