Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8182043: Access to Windows Large Icons #2875

Closed
wants to merge 18 commits into from
Closed
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -26,6 +26,7 @@
package javax.swing.filechooser;

import java.awt.Image;
import java.awt.image.AbstractMultiResolutionImage;
import java.beans.PropertyChangeListener;
import java.io.File;
import java.io.FileNotFoundException;
@@ -225,7 +226,7 @@ public String getSystemTypeDescription(File f) {
* Icon for a file, directory, or folder as it would be displayed in
* a system file browser. Example from Windows: the "M:\" directory
* displays a CD-ROM icon.
*
* <p>
* The default implementation gets information from the ShellFolder class.
*
* @param f a <code>File</code> object
@@ -255,6 +256,54 @@ public Icon getSystemIcon(File f) {
}
}

/**
* Returns an icon for a file, directory, or folder as it would be displayed
* in a system file browser for the requested size.
* <p>
* The default implementation gets information from the
* {@code ShellFolder} class. Whenever possible, the icon
* returned will be a multi-resolution icon image,
* which will allow better scaling with different
* magnification factors.
Copy link
Member

@aivanov-jdk aivanov-jdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what are the standard terms to refer to High DPI environments and the scale factors associated with High DPI.

Copy link
Member Author

@azuev-java azuev-java May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Windows environment it is scaling size or scaling factor. On Mac it is called scaled resolution. So i would say scaling factor is an appropriate term.

* <p>
* Example: <pre>
* FileSystemView fsv = FileSystemView.getFileSystemView();
* Icon icon = fsv.getSystemIcon("application.exe", 64);
Copy link
Member

@azvegint azvegint May 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the first parameter be a File instance instead of String?
Icon icon = fsv.getSystemIcon(new File("application.exe"), 64);

Copy link
Member Author

@azuev-java azuev-java May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Yes, fixed both here and in CSR.

Copy link
Member

@mrserb mrserb May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that all possible paths can be pointed by the file object? Especially some "Windows Libraries" which are accessed by the shell folder?

Copy link
Member

@mrserb mrserb May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Later we say that this method returns null for non-existed files. is it always correct? I am not sure that the file created for the library report true for the exists() method; DId we test this usecase?

Copy link
Member Author

@azuev-java azuev-java May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not test for that in the regression test but i did tested it manually and we do return null for the non-existed files. I tested it on non-windows platform too. We still return null.

Copy link
Member Author

@azuev-java azuev-java May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is accessible from the "JFileChooser" drop-down menu. On my system, the Libraries at that drop down menu contain "GIT", "Documents", "Music". https://docs.microsoft.com/en-us/windows/client-management/windows-libraries

There are also "known folders" such as "Network" or "My computer" which do not have a real path but can be navigated in the JFileChooser and has an icon.

Ok, got it. Well, since they can be translated into the file system paths - even if these paths do not belong to physical filesystem - they are supported by the new API. Not for all of them i am able to receive the high resolution icons - like "Recent Items" for some reason only provides 32x32 and 16x16 no matter what size i am asking for. Others such as Documents, My Computer or 3D Objects do provide all resolutions available. For example here's the screenshot of all the available icons for 3D Objects
image

Copy link
Member

@mrserb mrserb May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how you got them via this method? I am not sure what parameters should be passed to it.

Copy link
Member

@aivanov-jdk aivanov-jdk May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't you answer your question already? If FileSystemView.getRoots() returns Desktop in Windows shell namespace. Otherwise, I don't know a way to get a reference to a virtual folder in Windows shell namespace which doesn't reference a file system object.

Alex's example uses "3D Objects" folder which is one of the known folders and has its own icon instead of the standard folder icon.

It's possible that I don't understand the question clearly.

Alex's fix affects WindowsPlacesBar on the left of JFileChooser in Windows LaF, the icons there look crispier in High DPI environment.

Copy link
Member Author

@azuev-java azuev-java May 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how you got them via this method? I am not sure what parameters should be passed to it.
String userdir = System.getenv("userprofile"); Icon icon = FileSystemView.getFileSystemView().getSystemIcon(new File(userdir + "\\3D Objects"), 64);

For some of the libraries getting file reference is quite easy, for some - not so much. But still doable.

Copy link
Member

@mrserb mrserb May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

* JLabel label = new JLabel(icon);
* </pre>
*
* @param f a <code>File</code> object
Copy link
Member

@aivanov-jdk aivanov-jdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param f a <code>File</code> object
* @param f a {@code File} object

* @param size width and height of the icon in virtual pixels
Copy link
Member

@mrserb mrserb May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the "virtual pixels"? I remember we refer to something similar by the point in the "user space coordinate system" Or probably we use the virtual pixels somewhere?

Copy link
Member Author

@azuev-java azuev-java May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the "virtual pixels"? I remember we refer to something similar by the point in the "user space coordinate system" Or probably we use the virtual pixels somewhere?

It is to say that the sizes are given in the same pixels as other components in the container and are subject to be rendered with different resolution based on the display scaling factor with preserving of relative sizes and proportions. The same terminology is used i.e. in SurfaceData.

Copy link
Member

@mrserb mrserb May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SurfaceData is not a public class, do we use this term somewhere in the spec? If not then it will be better to use size/points in the user space coordinate system, it is used already in the java2d.

Copy link
Member Author

@azuev-java azuev-java May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CSR is already approved and changing specification at this point will require to roll it back to draft and then going trough the approval process again which in turn risks this change not to make it in the upcoming LTS release. I would prefer to integrate it and create a follow-up bug to clarify the wording where we can discuss the matter and - if it is worth it - to submit a new CSR to amend the specification.

Copy link
Member

@aivanov-jdk aivanov-jdk May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If user coordinate system is used in similar context in other methods, we should change the wording to match it. I don't mind using a new bug to clarify virtual pixels.

The term user space (coordinate system) is used in the majority of cases.

Copy link
Member

@mrserb mrserb May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW this is why I recommended filing a CSR after the fix is fully discussed and agreed upon.

Copy link
Member Author

@azuev-java azuev-java May 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW this is why I recommended filing a CSR after the fix is fully discussed and agreed upon.

The CSR was filed almost five years ago.

* @return an icon as it would be displayed by a native file chooser
* or null if invalid parameters are passed such as pointer to a
* non-existing file.
Copy link
Contributor

@prrace prrace May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-existent not non-existing, but I think null deserves an IAE - as I had written a couple of days ago.

I see you dropped the words about null if the size is too large.
Should I interpret that as meaning one of the things I suggested - that this is just another case of a closest match ?

Copy link
Member Author

@azuev-java azuev-java May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for correction. Yes - we will return the best match no matter how big the requested size is.

Fixed IAE in case of null file.

* @see JFileChooser#getIcon
Copy link
Contributor

@prrace prrace May 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor grammar : add "an"or "a" as appropriate, ie change to :
"if an invalid parameter such a negative size or a null file reference"

* @see AbstractMultiResolutionImage
* @since 17
Copy link
Contributor

@prrace prrace May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add the IAE to the javadoc.

Copy link
Member Author

@azuev-java azuev-java May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

*/
public Icon getSystemIcon(File f, int size) {
if (f == null) {
return null;
Copy link
Contributor

@prrace prrace May 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested the a null File ought to be IAE too - this is showing up in the conversation thread but not here.

}

ShellFolder sf;

try {
sf = ShellFolder.getShellFolder(f);
} catch (FileNotFoundException e) {
return null;
}

Image img = sf.getIcon(size);

if (img != null) {
return new ImageIcon(img, sf.getFolderType());
} else {
return UIManager.getIcon(f.isDirectory() ? "FileView.directoryIcon"
: "FileView.fileIcon");
}
}

/**
* On Windows, a file can appear in multiple folders, other than its
* parent directory in the filesystem. Folder could for example be the
@@ -207,6 +207,15 @@ public Image getIcon(boolean getLargeIcon) {
return null;
}

/**
* Returns the icon of the specified size used to display this shell folder.
*
* @param size size of the icon > 0 (Valid range: 1 to 256)
Copy link
Member

@aivanov-jdk aivanov-jdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure the valid range of 1 to 256 makes sense provided the icon smaller than 16×16 is never returned (on Windows at least).

Copy link
Member Author

@azuev-java azuev-java May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, user still can request 1x1 icon - we will return the multiresolution image with minimal (1x1) icon inside that will be scaled every time the actual painting occurs. The size will define the layout of the component with the icon and can be auto-generated from the user code so i do not see why we should limit the lowest requested size - especially in the shared instance that not only specific for Windows platform.

Copy link
Member

@aivanov-jdk aivanov-jdk May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though 1×1 icon doesn't make much sense, you're right imposing a limitation is no good either.

* @return The icon of the specified size used to display this shell folder
*/
public Image getIcon(int size) {
return null;
}

// Static

@@ -82,6 +82,16 @@
@SuppressWarnings("serial") // JDK-implementation class
final class Win32ShellFolder2 extends ShellFolder {

static final int SMALL_ICON_SIZE = 16;
static final int LARGE_ICON_SIZE = 32;
static final int MIN_QUALITY_ICON = 16;
static final int MAX_QUALITY_ICON = 256;
private final static int[] ICON_RESOLUTIONS
= {16, 24, 32, 48, 64, 72, 96, 128, 256};

static final int FILE_ICON_ID = 1;
static final int FOLDER_ICON_ID = 4;

private static native void initIDs();

static {
@@ -991,14 +1001,15 @@ public String getExecutableType() {

// NOTE: this method uses COM and must be called on the 'COM thread'. See ComInvoker for the details
private static native long extractIcon(long parentIShellFolder, long relativePIDL,
boolean getLargeIcon, boolean getDefaultIcon);
int size, boolean getDefaultIcon);

// NOTE: this method uses COM and must be called on the 'COM thread'. See ComInvoker for the details
private static native boolean hiResIconAvailable(long parentIShellFolder, long relativePIDL);

// Returns an icon from the Windows system icon list in the form of an HICON
private static native long getSystemIcon(int iconID);
private static native long getIconResource(String libName, int iconID,
int cxDesired, int cyDesired,
boolean useVGAColors);
// Note: useVGAColors is ignored on XP and later
int cxDesired, int cyDesired);

// Return the bits from an HICON. This has a side effect of setting
// the imageHash variable for efficient caching / comparing.
@@ -1018,20 +1029,17 @@ private long getIShellIcon() {
return pIShellIcon;
}

private static Image makeIcon(long hIcon, boolean getLargeIcon) {
private static Image makeIcon(long hIcon) {
if (hIcon != 0L && hIcon != -1L) {
// Get the bits. This has the side effect of setting the imageHash value for this object.
final int[] iconBits = getIconBits(hIcon);
if (iconBits != null) {
// icons are always square
final int size = (int) Math.sqrt(iconBits.length);
final int baseSize = getLargeIcon ? 32 : 16;
final int iconSize = (int) Math.sqrt(iconBits.length);
final BufferedImage img =
new BufferedImage(size, size, BufferedImage.TYPE_INT_ARGB);
img.setRGB(0, 0, size, size, iconBits, 0, 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;
Copy link
Member

@aivanov-jdk aivanov-jdk Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are cases where the size of the buffered image is different from the requested size. It could affect the layout because it breaks the assumption that the returned image has the requested size but it may be larger. (Or is it no longer possible?) I think it should be wrapped into MultiResolutionIconImage in this case.

Copy link
Member Author

@azuev-java azuev-java Apr 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually in makeImage we do not use requested size, we return the bits that system returns to us. How the generated image is treated depends on the implementation of the public methods - where it matters they are wrapped in the corresponding multi resolution images so it behaves correctly in UI.

Copy link
Member

@aivanov-jdk aivanov-jdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, if it always wrapped in a multi-resolution image where it matters.

}
}
return null;
@@ -1043,11 +1051,13 @@ private static Image makeIcon(long hIcon, boolean getLargeIcon) {
*/
public Image getIcon(final boolean getLargeIcon) {
Image icon = getLargeIcon ? largeIcon : smallIcon;
int size = getLargeIcon ? LARGE_ICON_SIZE : SMALL_ICON_SIZE;
if (icon == null) {
icon =
invoke(new Callable<Image>() {
public Image call() {
Image newIcon = null;
Image newIcon2 = null;
if (isLink()) {
Win32ShellFolder2 folder = getLinkLocation(false);
if (folder != null && folder.isLibrary()) {
@@ -1072,33 +1082,39 @@ public Image call() {
newIcon = imageCache.get(Integer.valueOf(index));
if (newIcon == null) {
long hIcon = getIcon(getAbsolutePath(), getLargeIcon);
newIcon = makeIcon(hIcon, getLargeIcon);
newIcon = makeIcon(hIcon);
disposeIcon(hIcon);
if (newIcon != null) {
imageCache.put(Integer.valueOf(index), newIcon);
}
}
}
}

if (newIcon == null) {
// These are only cached per object
long hIcon = extractIcon(getParentIShellFolder(),
getRelativePIDL(), getLargeIcon, false);
// E_PENDING: loading can take time so get the default
if(hIcon <= 0) {
hIcon = extractIcon(getParentIShellFolder(),
getRelativePIDL(), getLargeIcon, true);
if(hIcon <= 0) {
if (isDirectory()) {
return getShell32Icon(4, getLargeIcon);
if (newIcon != null) {
if (isLink()) {
imageCache = getLargeIcon ? smallLinkedSystemImages
: largeLinkedSystemImages;
} else {
return getShell32Icon(1, getLargeIcon);
imageCache = getLargeIcon ? smallSystemImages : largeSystemImages;
}
newIcon2 = imageCache.get(Integer.valueOf(index));
Copy link
Member

@aivanov-jdk aivanov-jdk May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, explicit boxing is unnecessary.

Copy link
Member Author

@azuev-java azuev-java May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

if (newIcon2 == null) {
long hIcon = getIcon(getAbsolutePath(), !getLargeIcon);
newIcon2 = makeIcon(hIcon);
disposeIcon(hIcon);
}
}

if (newIcon2 != null) {
Map<Integer, Image> bothIcons = new HashMap<>(2);
bothIcons.put(getLargeIcon ? LARGE_ICON_SIZE : SMALL_ICON_SIZE, newIcon);
bothIcons.put(getLargeIcon ? SMALL_ICON_SIZE : LARGE_ICON_SIZE, newIcon2);
newIcon = new MultiResolutionIconImage(getLargeIcon ? LARGE_ICON_SIZE
: SMALL_ICON_SIZE, bothIcons);
}
}
newIcon = makeIcon(hIcon, getLargeIcon);
disposeIcon(hIcon);
}

if (hiResIconAvailable(getParentIShellFolder(), getRelativePIDL()) || newIcon == null) {
newIcon = getIcon(getLargeIcon ? LARGE_ICON_SIZE : SMALL_ICON_SIZE);
}

if (newIcon == null) {
@@ -1107,42 +1123,78 @@ public Image call() {
return newIcon;
}
});
if (getLargeIcon) {
largeIcon = icon;
} else {
smallIcon = icon;
}
}
return icon;
}

/**
* @return The icon image of specified size used to display this shell folder
*/
public Image getIcon(int size) {
return invoke(() -> {
Image newIcon = null;
if (isLink()) {
Win32ShellFolder2 folder = getLinkLocation(false);
if (folder != null && folder.isLibrary()) {
return folder.getIcon(size);
}
}
Map<Integer, Image> multiResolutionIcon = new HashMap<>();
int start = size > MAX_QUALITY_ICON ? ICON_RESOLUTIONS.length - 1 : 0;
Copy link
Member

@aivanov-jdk aivanov-jdk Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to always start at zero?
The icons of smaller size will never be used, will they?
Thus it's safe to start at the index which corresponds to the requested size if size matches, or the index such as ICON_RESOLUTIONS[index] < size && ICON_RESOLUTIONS[index + 1] > size.

Copy link
Member

@aivanov-jdk aivanov-jdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is also about the case of not fetching icons of sizes smaller than requested size.

Copy link
Member Author

@azuev-java azuev-java May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, missed that in my latest fix. Indeed there is no legitimate ways to set scaling factor to less than 100% on Windows so yes, omitting the icons that are less than expected size. As for starting the count from the correct index - to get the correct index we would have to traverse the array of possible resolutions anyways so there is no performance gain.

Copy link
Member

@mrserb mrserb May 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MRI image takes care of graphics transformation which might be the same as a scale factor of the screen for the onscreen rendering, but in general, it might be set to any value by the application when rendered to the image.

int increment = size > MAX_QUALITY_ICON ? -1 : 1;
int end = size > MAX_QUALITY_ICON ? -1 : ICON_RESOLUTIONS.length;
for (int i = start; i != end; i += increment) {
int s = ICON_RESOLUTIONS[i];
if (size < MIN_QUALITY_ICON || size > MAX_QUALITY_ICON
|| (s >= size/2 && s <= size*2)) {
long hIcon = extractIcon(getParentIShellFolder(),
getRelativePIDL(), s, false);

// E_PENDING: loading can take time so get the default
if (hIcon <= 0) {
hIcon = extractIcon(getParentIShellFolder(),
getRelativePIDL(), s, true);
if (hIcon <= 0) {
if (isDirectory()) {
return getShell32Icon(FOLDER_ICON_ID, size);
} else {
return getShell32Icon(FILE_ICON_ID, size);
}
}
}
newIcon = makeIcon(hIcon);
disposeIcon(hIcon);

multiResolutionIcon.put(s, newIcon);
if (size < MIN_QUALITY_ICON || size > MAX_QUALITY_ICON) {
break;
}
}
}
return new MultiResolutionIconImage(size, multiResolutionIcon);
});
}

/**
* Gets an icon from the Windows system icon list as an {@code Image}
*/
static Image getSystemIcon(SystemIcon iconType) {
long hIcon = getSystemIcon(iconType.getIconID());
Image icon = makeIcon(hIcon, true);
Image icon = makeIcon(hIcon);
disposeIcon(hIcon);
return icon;
}

/**
* Gets an icon from the Windows system icon list as an {@code Image}
*/
static Image getShell32Icon(int iconID, boolean getLargeIcon) {
boolean useVGAColors = true; // Will be ignored on XP and later

int size = getLargeIcon ? 32 : 16;

static Image getShell32Icon(int iconID, int size) {
Toolkit toolkit = Toolkit.getDefaultToolkit();
String shellIconBPP = (String)toolkit.getDesktopProperty("win.icon.shellIconBPP");
Copy link
Member

@aivanov-jdk aivanov-jdk Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these lines aren't used any more and should be removed too.

Copy link
Member Author

@azuev-java azuev-java May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Fixed.

if (shellIconBPP != null) {
useVGAColors = shellIconBPP.equals("4");
}

long hIcon = getIconResource("shell32.dll", iconID, size, size, useVGAColors);
long hIcon = getIconResource("shell32.dll", iconID, size, size);
Copy link
Member

@aivanov-jdk aivanov-jdk May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's outside the scope for this code review but still, should getIconResource free the loaded library? With each call, the library gets loaded but it's never freed and thus it's never unloaded even if it's not needed any more.

Copy link
Member Author

@azuev-java azuev-java May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will create a separate bug to track this - i do not know if library loading gets cached on some level and what impact on performance will we have if we release it every call. But i will check it out separately.

Copy link
Member

@aivanov-jdk aivanov-jdk May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! I just wanted to bring it up. I could've submitted the bug myself… yet I decided to ask at first.
As far as I can see JDK_LoadSystemLibrary has no caching, it just loads the library. If any icons are accessed shell32.dll will already be loaded. Probably, we never fully release it from COM. So in this case, loading and freeing will just increase and decrease the counters. Even if it's never really unloaded, we should release the resources that's not needed any more.

Copy link
Member

@aivanov-jdk aivanov-jdk May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For documentation purposes, JDK-8266948: ShellFolder2::getIconResource does not release the library loaded.

if (hIcon != 0) {
Image icon = makeIcon(hIcon, getLargeIcon);
Image icon = makeIcon(hIcon);
disposeIcon(hIcon);
return icon;
Copy link
Member

@aivanov-jdk aivanov-jdk May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall it not be wrapped into multi-resolution image if the size is different from the actual size of the icon?
Previously, it was inside makeIcon.

Copy link
Member Author

@azuev-java azuev-java May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wherever it is necessary down the line we are wrapping the result in multi-resolution and if we wrap it here too we will have multi-resolution icon that contains multi-resolution images as a resolution variant. Unfortunately AWT can't handle painting of such abomination.

Copy link
Member

@aivanov-jdk aivanov-jdk May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it isn't wrapped: if getShell32Icon is called in getIcon(final boolean getLargeIcon), the returned value is immediately returned to the caller, see lines 1157–1163 in the updated code:

                        if (hIcon <= 0) {
                            if (isDirectory()) {
                                return getShell32Icon(FOLDER_ICON_ID, size);
                            } else {
                                return getShell32Icon(FILE_ICON_ID, size);
                            }
                        }

It's not wrapped into multi-resolution icon when called from Win32ShellFolder2.get() for keys shell32Icon * and shell32LargeIcon * (lines 411/413–414).

Neither is the returned value of Win32ShellFolder2.getSystemIcon wrapped; it's also called from Win32ShellFolder2.get() when getting icons for optionPaneIcon * (lines 405/407). These icons are supposed to be large 32×32 icons, thus if the size of the icon in getSystemIcon(SystemIcon iconType) differs from 32, it should be wrapped. Otherwise, it could cause regression for JDK-8151385: [hidpi] JOptionPane-Icons only partially visible when using Windows 10 L&F.

Copy link
Member Author

@azuev-java azuev-java May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see - but still it has to be solved in the getShell32Icon method - which i'm going to do in a moment.

Copy link
Member

@aivanov-jdk aivanov-jdk May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Win32ShellFolder2.getSystemIcon is still affected, the return value should be wrapped into MR-icon if its size is not equal to LARGE_ICON_SIZE.

    static Image getSystemIcon(SystemIcon iconType) {
        long hIcon = getSystemIcon(iconType.getIconID());
        Image icon = makeIcon(hIcon);
        if (LARGE_ICON_SIZE != icon.getWidth(null)) {
            icon = new MultiResolutionIconImage(size, icon);
        }
        disposeIcon(hIcon);
        return icon;
    }

Copy link
Member Author

@azuev-java azuev-java May 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}
@@ -1325,13 +1377,17 @@ public List<KnownFolderDefinition> call() throws Exception {
}

static class MultiResolutionIconImage extends AbstractMultiResolutionImage {

final int baseSize;
final Image resolutionVariant;
final Map<Integer, Image> resolutionVariants = new HashMap<>();

public MultiResolutionIconImage(int baseSize, Map<Integer, Image> resolutionVariants) {
this.baseSize = baseSize;
this.resolutionVariants.putAll(resolutionVariants);
}

public MultiResolutionIconImage(int baseSize, Image resolutionVariant) {
public MultiResolutionIconImage(int baseSize, Image image) {
this.baseSize = baseSize;
this.resolutionVariant = resolutionVariant;
this.resolutionVariants.put(baseSize, image);
}

@Override
@@ -1346,17 +1402,34 @@ public int getHeight(ImageObserver observer) {

@Override
protected Image getBaseImage() {
return resolutionVariant;
return getResolutionVariant(baseSize, baseSize);
}

@Override
public Image getResolutionVariant(double width, double height) {
return resolutionVariant;
int dist = 0;
Image retVal = null;
// We only care about width since we don't support non-rectangular icons
int w = (int) width;
int retindex = 0;
for (Integer i : resolutionVariants.keySet()) {
if (retVal == null || dist > Math.abs(i - w)
|| (dist == Math.abs(i - w) && i > retindex)) {
retindex = i;
dist = Math.abs(i - w);
retVal = resolutionVariants.get(i);
if (i == w) {
break;
}
}
}
return retVal;
}

@Override
public List<Image> getResolutionVariants() {
return Arrays.asList(resolutionVariant);
return Collections.unmodifiableList(
new ArrayList<Image>(resolutionVariants.values()));
}
}
}