Hi Alexey,
first of all - the new and improved webrev is available at
http://cr.openjdk.java.net/~kizune/8182043/webrev.03/
I returned the limitation of icon sizes in the result which should
enhance the performance, also i fixed some corner cases and improved
docuentation. More details inline.
/Alex
On 7/15/2020 4:46 PM, Alexey Ivanov wrote:
Hi Alexander,
I've run some tests and this version works good. However, I noticed
the new version is slower than before, it could be due to the fact
that the entire set of icons is extracted from the native. On average,
JFileChooser is 100ms slower to show on the screen.
That should be dealt with in the new patch.
I think we should limit the number of icons that we extract as you did
in webrev.01.
Done.
If I request the icon of size 16:
FileSystemView.getFileSystemView().getSystemIcon(new File("C:\"), 16);
the resulting icon contains icon sizes 16, 24, 32, 48, 64, 72, 96,
128, 256.
Hardly the icon of size 96 and above will ever be used: 96px is 6
times 16px. Getting 48px-sized icon corresponds to 300% scaling, it
may be reasonable, yet still unlikely to be used.
Then if I request the icon of size 48:
FileSystemView.getFileSystemView().getSystemIcon(new File("C:\"), 48);
the resulting icon contains the same set of sizes. But the sizes
smaller than the base size will never be used. I believe scaling
factors less than 100% are not supported.
I still tend to leave it on - it does not hurt performance much and
So we should decide up to which resolution we get the icons. Getting
icons up to 200% makes sense.
What about the lower limit? if the requested size matches one from our
list, nothing to decide. What if size of 33 is requested? Scaling up
32×32 icon will likely give a better result than scaling down 48×48.
That's why in my code i do keep lower resolution images and when asked
about the resolution variant i'm returning closest resolution to one
asked so in your case if i got asked for 33 pixel icon i will present
the 32 pixel one.
Will such an implementation be good enough? This would resolve this
RFE: provide access to Windows large icon.
Then another problem which we're trying to solve: make JFileChooser
support High DPI environments. In majority of cases, JFileChooser will
use another method to extract the icon:
Win32ShellFolder2.getIcon(boolean). This function uses SHGFI_SMALLICON
or SHGFI_LARGEICON parameters to SHGetFileInfo, and therefore the size
is limited to 16×16 and 32×32. It will return the icon of the
currently configured size for the shell icon, small and large
correspondingly; however, this parameter cannot be easily changed. On
the other hand, the shell icon size takes DPI into account, and the
size of the returned icon may differ from 16 and 32.
This situation where the size was different from the requested one was
handled by wrapping the icon into MultiResolutionIconImage:
Win32ShellFolder2.makeIcon(long hIcon, int size)
- return size == baseSize
- ? img
- : new MultiResolutionIconImage(baseSize, img);
+ new BufferedImage(iconSize, iconSize,
BufferedImage.TYPE_INT_ARGB);
+ img.setRGB(0, 0, iconSize, iconSize, iconBits, 0,
iconSize);
+ return img;
The new code works well for getIcon(int size) which wraps a set of
icons into MultiResolutionIconImage.
I think the calls to getIcon(boolean getLargeIcon) should still wrap
the result into MultiResolutionIconImage if the size differs from the
requested one.
Right now the icon got wrapped in the multiresolutionimage when more
than one variant is available.
I can change it to wrap it every time even if the bigger or smaller
variant is not available but i do not see
how it can benefit the file manager itself.
The code at
1158 if (newIcon.getWidth(null) == s) {
1159 multiResolutionIcon.put(s, newIcon);
1160 }
handles the same situation. Yet here, we'll throw away the icon if its
size is not equal to the requested one. I wonder if this condition is
ever false, however.
I removed this condition for i found a corner case when it is 100% false
- the dynamically generated icons in AppData/Roaming folder if promary
monitor is set to any zoom factor.
To provide better support for High DPI environments in JFileChooser,
getIcon(boolean getLargeIcon) should also return a set of icons.
It does right now. And result is better than it was before.
This also applies to Win32ShellFolder2.getSystemIcon(SystemIcon
iconType) which returns standard icons for message boxes, or JOptionPane.
But there's one catch: new way of retrieving icons does not get the
custom drive
icon (can be seen at disk C: - the windows logo identifying the
windows system drive
is no longer present). I am questioning myself, what is better: to
have better icon quality
and missing custom disk logo or to have custom disk logo and mediocre
icon quality?
Which is weird, but I also noticed it.
I wonder why different APIs in Windows give different icons:
SHGetFileInfo which is used in getIcon(boolean getLargeIcon), and
IExtractIcon::GetIconLocation and IExtractIcon::Extract which are used
in getIcon(int size) that calls extractIcon(). The former function,
SHGetFileInfo, can also decorate icons with link (shortcut) overlay
whereas the latter does not seem to provide such an option.
There is a way to try to get overlay for the icon but it also works
inconsistently.
As for High DPI environments and memory footprint, getting icon sizes
according to the display scaling looks like an ideal solution. But it
seems too tricky. The MultiResolutionIconImage should store all the
details of the file or the shell folder to be able to fetch a
different size of DPI changes. Eventually, keeping lots of objects to
achieve that could have larger footprint than storing a set of icons
all the time.
Overall, the current approach of fetching a set of icons to cater for
the most common scaling factors looks sensible.
Regards,
Alexey
On 09/07/2020 00:21, Alexey Ivanov wrote:
Hi Alexander,
Sorry for my belated review.
*FileSystemView.java*
I couldn't build JDK because of the warning about empty <p> in
javadoc for getSystemIcon(File f, int size).
<p> is not needed before <pre>. Yet <p> should be added after each
empty line in the javadoc to start a new paragraph, otherwise all the
text is presented one long description.
276 * @param size width and height of the icon in pixels to be
scaled
277 * (valid range: 1 to 256)
I'd drop "to be scaled"; width and height are the base size of the
icon for the case.
*ShellFolder.java*
205 * @param size size of the icon > 0(Valid range: 1 to 256)
I recommend adding space before the opening parenthesis and use
lower-case in the parentheses as in FileSystemView.java.
*Win32ShellFolder2.java*
80 private final static int[] ICON_RESOLUTIONS
81 = {8, 16, 24, 32, 48, 64, 72, 96, 128, 256};
Shall we drop 8 from the list? The size of 8×8 is non-standard for
Windows, is it provided by any Windows Shell interfaces?
*Win32ShellFolder2.java*
The trickery with newIcon2 in getIcon(final boolean getLargeIcon) is
for getting both 16×16 and 32×32 and returning them as
MultiResolutionImage, isn't it?
I guess the best results would be if we get a list of 16×16, 24×24,
and 32×32 icons when small icon is requested, and 32×32, 48×48, and
64×64 when large icon is requested, which would cover scaling factors
of 100%, 150%, and 200%.
I don't think we'll ever need small icon when large one is requested,
and if I read the code correctly this is what would happen). However,
JFileChooser does not seem to allow for large icons in its file view,
thus adding the larger icon makes the rendering crispier in High DPI
environments.
1154 if (size > 512 || size < 4) {
1155 return newIcon;
1156 }
I can understand that there are no useful resolutions if size of 512
or larger is requested. Should it be 256 or rather
ICON_RESOLUTIONS[ICON_RESOLUTIONS.length - 1]?
Javadoc states the valid range ends at 256.
As for the minimum size… In my opinion, icon size less than 8 is not
viable; on the other hand we can't make any assumptions. So if the
requested size is less than 4, then we'll return only 8×8 icon (or
16×16, if 8 is dropped from the list). Do I get it right?
*ShellFolder2.cpp*
981 hres = pIcon->Extract(szBuf, index, &hIcon,
&hIconLow, (16 << 16) + size);
982 if (size < 24) {
I wonder why you extract two icons, you never use both. This adds a
delay: only one of the extracted icons is ever used.
If the idea is to limit the size by 16, then min(16, size) passed as
the icon size would do the trick.
With this new version, I noticed that JFileChooser takes longer to
appear and then longer to populate the file list.
I'm still playing around with the build but I wanted to provide my
feedback.
Regards,
Alexey
On 29/06/2020 15:27, Alexander Zuev wrote:
Alexey, Sergey,
here's the latest version of the fix:
http://cr.openjdk.java.net/~kizune/8182043/webrev.02/
It adds one more native method - hiResIconAvailable
that queries if system will be able to provide high resolution icons
for a given file.
Now i have tested three approaches with the FileManager:
The old one - at magnification 150% can be seen here:
http://cr.openjdk.java.net/~kizune/8182043/example/fchooser_old_150.PNG
I changed the behavior by making the icon loaded by the FileChooser
a MultiResolutionIcon
carrying both small and large icons at the same time.
The result looks much better - here's the same view with the
intermediate fix:
http://cr.openjdk.java.net/~kizune/8182043/example/fchooser_inter_150.PNG
But then i added the new native method and in FileChooser i'm
getting the multiResolutionIcon
for all the files for which it is possible. The result looks much
crisper:
http://cr.openjdk.java.net/~kizune/8182043/example/fchooser_new_150.PNG
But there's one catch: new way of retrieving icons does not get the
custom drive
icon (can be seen at disk C: - the windows logo identifying the
windows system drive
is no longer present). I am questioning myself, what is better: to
have better icon quality
and missing custom disk logo or to have custom disk logo and
mediocre icon quality?
/Alex
On 22-Jun-20 11:27, Alexander Zuev wrote:
Hi Alexey,
sorry for late responce, after fixing that initial error with the
wrong
icon sizes pulled i found a lot of corner cases that needed some
additional digging.
On 18-Jun-20 20:56, Alexey Ivanov wrote:
On 18/06/2020 02:30, Sergey Bylokhov wrote:
On 6/17/20 5:50 pm, Sergey Bylokhov wrote:
I guess it is too early to compare behavior before and after the
fix since the fix has a few bugs.
- It does not fetch several sizes, as it was expected from the
code, but load only one native size
and map it to the different resolutions.
Yes, I re-read your message and found this mistake.
I fixed it locally.
Then I noticed JFileChooser does not really use the new method
which returns MRI, so this explains why JFileChooser never adjusts
the icons according the screen DPI.
I modified Win32ShellFolder2.getIcon(boolean) to use getIcon(size)
instead of getIcon(getAbsolutePath(), getLargeIcon), and now the
file icons are updated when JFileChooser window is moved from
standard DPI monitor to a High DPI one (150%) and back. I noticed
one artefact though: the customised icons from disk drives
disappeared, some file icons look clipped, on either monitor.
I also noticed this behavior, some icons retrieved with the
extractIcon method got clipped or improperly scaled.
I tried to dig deeper and found that it happens when
pIcon->GetIconLocation function returns "*"
as location of the resource file. I haven't found exact reason for
that but when this is the case then
getIconBits function always retrieves 32x32 icon no matter what
icon size we requested.
We then scale it down to 16x16 and the result is unpleasant.
I'm trying to find another way of retrieving the custom icon but
for now i would say that
for the small icons the approach of getIcon function which uses
SHGetFileInfo holds better results.
So far the approach that seems to be working is to check if we have
a high resolution
icon available by requesting icon location. If icon location is not
known we can get both
low and high resolution icons from SHGetFileInfo and store them in
the MRI. If high res icon available
then fetch all the standard resolution icons and also return them
as MRI.
/Alex
