Hi Alexander,
I haven't looked at the code thoroughly yet, I'm just replying to
already raised concerns. Please see inline:
On 31/03/2020 01:02, Alexander Zuev wrote:
He Sergey,
please look for answers inline.
On 30-Mar-20 21:35, Sergey Bylokhov wrote:
Hi, Alexander.
A few initial question before I have look to the webrev:
- Why we need a new class for only one method, why we cannot enhance
the FileSystemView
where the similar method is implemented already getSystemIcon(File)?
As you know this is not the first attempt to fix this issue and when i
asked about "Why do we need
a separate class for one new method" the answer was "There is a
reason, we tried different approaches
and this one is what we ended up with". Exact reason buried somewhere
in the previous reviews.
Personally for me i would prefer adding the new method to some
existing public class.
Here's the link to the latest bit of the discussion that i found and
there was exactly the same question
raised by Alexey Ivanov, there are some reasons for creating a
separate class:
http://mail.openjdk.java.net/pipermail/awt-dev/2019-January/014941.html
For some reason the discussion looks incomplete as if some part of it
happened
outside of the alias so i can't say what was the outcome - aside of
the fact that last proposed
implementation still had a separate class.
As far as I remember there was no clear explanation as to why a new
class is needed, none that I'm aware of at the very least.
Initially there were many other helper methods which deemed unnecessary
during code review.
I'm for adding the new method into FileSystemView class: it seems
logical to me, it just extends the existing functionality.
- Can we try to re-implement the places where the old method
ShellFolder.getIcon(boolean)
was used, and change it to use the new public API, just to confirm
that our new code is a
a good replacement of the old/private api. I guess we could get
rid the boolean version.
It is outside of the initial scope of the request but yes - i can do
it. Should i do it within this fix or
should i create a new bug and do it there?
I think we should update implementation of ShellFolder.getIcon(boolean)
to use the new API. This way, we'll also test the new API.
- The current spec for SystemIcon.getSystemIcon() specify that the
icon will store the
"maximum quality icon" what does it meant?
It means that the maximum size of the icon allowed by the system will
be used. Right now on
Windows (and this issue is Windows specific) the maximum icon size
allowed is 256x256 pixels.
That is the size we will request and store in the
MultiResolutionImageIcon.
What if 256×256 icon is not available. Will it result in Windows
up-scaling the largest icon for us to 256×256 which we will down-scale
to the requested size?
As I read in your initial note, sizes below 24 are not down-scaled.
However, I think we should also make the exception for 32×32 icons too:
it's the standard icon size which is also somewhat optimised for this
size. The size of 48×48 could also be an exception as many applications
provide this icon size since Windows XP era.
In general we should use the closest match to the requested size.
Unfortunately, Windows does not provide an easy-to-use API which can
give you the list of sizes available in the icon. Having the list, we
can dynamically request the closest match and cache it, and then up- or
down-scale it. This would also work well in High DPI environments with
multiple displays: a multi resolution image would have use the right
icon size. For example, 16×16 icon at 200% scale is 32×32, so 32×32 icon
can be used avoiding scaling up the small icon; or 32×32 icon at 150%
scale is 48×48, this icon size is is also often available directly.
Regards,
Alexey
- Another question is about multi-screen environment, if the
JFileChooser will be shown
on the non-HiDPI screen and then moved to the HiDPI screen which
icons we will request
from the native and which actual icons(resolution) will we draw on
each screen?(both types
of icons large/small are interesting).
Right now Swing is handling scaling and with my limited testing
capability (i don't have access to
the different multi-monitor configurations for obvious reasons) i
don't see any problem. Both
small and large sized icons got scaled together with the scaling
factor change, obviously
quality of icons with size 32 and up is much better since they all are
downscale of the 256x256
icon and both 16x16 and 24x24 icons are pretty pixelated with scaling
factor 200% and up,
but that was a trade off for allowing of the custom small icons to be
used where available.
/Alex
On 3/30/20 4:19 am, Alexander Zuev wrote:
Hello,
please review my fix for the issue 8182043: Access to Windows
Large Icons
Bug: https://bugs.openjdk.java.net/browse/JDK-8182043
Webrev: http://cr.openjdk.java.net/~kizune/8182043/webrev
<http://cr.openjdk.java.net/~kizune/8182043/webrev/>
Main idea is to provide a new API call to retrieve image of the
specified size
and to make Windows implementation that for all the resolutions
higher than 24 pixels
returns the multi resolution image icon with image inside being the
highest quality icon
available and the size set to the size requested by the user. This
way we will have good
scaling across the different resolution while maintaining relative
sizes in the UI intact.
The exception made for images size of 24 and less since sometimes
application has
different image for the small icons in its resource section which is
optimized to
make sure that on low resolution screen this icon is not displayed
as just scaled down
blurry little square.
/Alex
