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
