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

Conversation

@azuev-java
Copy link
Member

@azuev-java azuev-java commented Mar 8, 2021

Fix updated after first round of review.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875
$ git checkout pull/2875

Update a local copy of the PR:
$ git checkout pull/2875
$ git pull https://git.openjdk.java.net/jdk pull/2875/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2875

View PR using the GUI difftool:
$ git pr show -t 2875

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/2875.diff

Fix updated based on the previous review.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 8, 2021

👋 Welcome back kizune! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Mar 8, 2021
@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Mar 8, 2021

Continuation of review at #380

@openjdk
Copy link

@openjdk openjdk bot commented Mar 8, 2021

@azuev-java The following labels will be automatically applied to this pull request:

  • 2d
  • awt
  • swing

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Mar 8, 2021

/csr needed

@openjdk openjdk bot added the csr label Mar 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 8, 2021

@azuev-java this pull request will not be integrated until the CSR request JDK-8188238 for issue JDK-8182043 has been approved.

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 10, 2021

Just to continue the thread from the old review, did you have a chance to look at the possibility of loading all existed icons embedded in the file? It was discussed here:
#380 (comment)

@azuev-java
Copy link
Member Author

@azuev-java azuev-java commented Mar 10, 2021

Just to continue the thread from the old review, did you have a chance to look at the possibility of loading all existed icons embedded in the file? It was discussed here:
#380 (comment)

Yes, i did. There were 3 shortcomings: 1. It works in about 10% of the cases in all other cases it reports only one icon available (32x32) even if there are other resolution icons. 2. It is quite slow, especially on network drives. 3. Does not work at all in files that are within AppData\Roaming folder - returning list is just empty.

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.

newIcon2 = imageCache.get(Integer.valueOf(index));
if (newIcon2 == null) {
long hIcon = getIcon(getAbsolutePath(), !getLargeIcon);
newIcon2 = makeIcon(hIcon, getLargeIcon ? SMALL_ICON_SIZE
: LARGE_ICON_SIZE);
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);
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.

I still propose to refactor this code to make it clearer. The code always returns two icons: small + large or large + small — combined in MultiResolutionIconImage.

You can get smallIcon and largeIcon and then wrap them into MultiResolutionIconImage in the correct order and store each icon in the cache.

My opinion hasn't changed here.

I still think there's not much value in getting smaller icon size in addition to larger one. Or does it provide an alternative image for the case where the system scaling is 200% and the window is moved to a monitor with scaling of 100%?

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.

Getting smaller icon is relevant in the case of the scaling. I do not think refactoring image caches from icons to multiresolution images will make code much cleaner - at the end we will have to extract images from the multiresolution image to repack them into different multiresolution image because the base size of the image will not be the same and it does matter in how they will be scaled on the different screens. And we still need to extract both images because sometimes small resolution image looks not like the large resolution one and for a reason - so small resolution image is not blurred by the small detail on the large icon when scaling on the low resolution screen.

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.

No, I can't see how it's relevant. If the scale factor is 100% and the requested icon size is 16, then 16×16 is used; if the window is moved to a monitor with scale factor 200%, then 32×32 icon is used for rendering which is already fetched and available in the multi-resolution image — perfect, there's the benefit.

But when is it possible that the scale factor is less than 100%?

If JFileChooser requests a large icon that is 32×32, then it will be used for rendering when the scale factor is 100%. When the scale factor is 200%, the icon of 64×64 is required but it's not available, instead there's 16×16 icon. For this icon to be used, the scale factor needs to be 50%.

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.

Ok, changed so only the icons that are higher or same size as requested will be in the resulting set (unless size requested exceeds the maximum allowed quality, in this case only maximum quality variant will be provided).

disposeIcon(hIcon);
return icon;
}

/**
* Gets an icon from the Windows system icon list as an {@code Image}
*/
static Image getShell32Icon(int iconID, boolean getLargeIcon) {
static Image getShell32Icon(int iconID, int size) {
boolean useVGAColors = true; // Will be ignored on XP and later
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.

I cannot see where useVGAColors is used. Since the supported OS are later than XP, it can be removed, can't it?

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.

I think so. Fixed.

throw new RuntimeException("icon is null!!!");
}

if (icon.getIconWidth() != size) {
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 check that the icon is a multi-resolution image with the expected sizes?
We know for sure explorer.exe contains the entire set of icons.

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.

Since the test always expects a multi-resolution image, does it make sense to assert icon instanceof MultiResolutionImage at the very 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.

Ok, added testing for multiresolution icon.

* a system file browser for the requested size.
* <p>
* The default implementation gets information from the
* <code>ShellFolder</code> class. Whenever possible, the icon
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.

Do we continue using <code> elements? I thought that {@code} is the preferred way to markup class names.

Copy link
Member Author

@azuev-java azuev-java Apr 30, 2021

Choose a reason for hiding this comment

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

Fixed.

bothIcons.put(getLargeIcon? LARGE_ICON_SIZE : SMALL_ICON_SIZE, newIcon);
bothIcons.put(getLargeIcon? SMALL_ICON_SIZE : LARGE_ICON_SIZE, newIcon2);
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.

Suggested change
bothIcons.put(getLargeIcon? LARGE_ICON_SIZE : SMALL_ICON_SIZE, newIcon);
bothIcons.put(getLargeIcon? SMALL_ICON_SIZE : LARGE_ICON_SIZE, newIcon2);
bothIcons.put(getLargeIcon ? LARGE_ICON_SIZE : SMALL_ICON_SIZE, newIcon);
bothIcons.put(getLargeIcon ? SMALL_ICON_SIZE : LARGE_ICON_SIZE, newIcon2);

}
}
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 start = size > MAX_QUALITY_ICON ? ICON_RESOLUTIONS.length - 1 : 0;
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) {
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.

Suggested change
for (int i = start; i != end; i+=increment) {
for (int i = start; i != end; i += increment) {

// 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()) {
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.

Suggested change
for (Integer i: resolutionVariants.keySet()) {
for (Integer i : resolutionVariants.keySet()) {

@@ -255,6 +256,54 @@ public Icon getSystemIcon(File f) {
}
}

/**
* Icon for a file, directory, or folder as it would be displayed in
Copy link
Member

@aivanov-jdk aivanov-jdk Apr 2, 2021

Choose a reason for hiding this comment

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

Returns an icon for a file…

* </pre>
*
* @param f a <code>File</code> object
* @param size width and height of the icon in pixels
Copy link
Member

@aivanov-jdk aivanov-jdk Apr 2, 2021

Choose a reason for hiding this comment

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

Pixels could be ambiguous now. Usually, Swing deals with user's space. That is 16×16 icon should actually be 32×32 if the scale factor of the current display is 200%.

Yes, this issue is somewhat irrelevant because the method returns multi-resolution icon. However, the terminology used must be unambiguous and clear; for consistency with other Swing API, it should be in terms of user's space coordinates, virtual pixels.

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.

Ok, fixed.

azuev-java and others added 2 commits Apr 30, 2021
Remived size parameter from makeIcon
Removed useVGAColors usage as irrelevant since it is ignored on all
supported platforms now
Select one icon at a time.

Co-authored-by: Alexey Ivanov <70774172+aivanov-jdk@users.noreply.github.com>
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.

* 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

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

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

/**
* 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.

* which will allow better scaling with different
* magnification factors.
* scaling factors.
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.

“…which will allow better support for High DPI environments with different scaling factors.” — does it look better?
“scaling with … scaling factors” doesn't sound good to me.

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.

Yes, much better. I updated the wording and once the PR is approved i will fix it in CSR too.

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.

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.


long hIcon = getIconResource("shell32.dll", iconID, size, size, useVGAColors);
static Image getShell32Icon(int iconID, int size) {
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.

return FALSE;
}
HRESULT hres;
IExtractIconW* pIcon;
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.

pIcon->Release() must be called before returning as done in extractIcon.

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.

Fixed.

azuev-java added 2 commits May 11, 2021
Removing unnecessary boxing of int
Releasing the pIcon in native code properly
requested icon size and actual icon size mismatch.
@@ -263,8 +263,8 @@ public Icon getSystemIcon(File f) {
* 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
* scaling factors.
* which will allow better support for High DPI environments
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.

I'm not sure but probably “…which allows better…” is more grammatical, as well as in the line above: “the icon returned is a multi-resolution”.

Phil or Joe could correct this.

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.

Agree.


long hIcon = getIconResource("shell32.dll", iconID, size, size, useVGAColors);
static Image getShell32Icon(int iconID, int size) {
long hIcon = getIconResource("shell32.dll", iconID, size, size);
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.

Fixed Win32ShellFolder2.getSystemIcon scaling issue
* <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!


if (icon.getImage() instanceof MultiResolutionImage) {
MultiResolutionImage mri = (MultiResolutionImage) icon.getImage();
if (mri.getResolutionVariant(size, size) == null) {
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.

This is to describe one of my questions above, is this instanceof+cast cannot be improved? Why we cannot always wrap the data in the MRI and if we have only one icon return the MRI with one resolution?

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.

This is to describe one of my questions above, is this instanceof+cast cannot be improved? Why we cannot always wrap the data in the MRI and if we have only one icon return the MRI with one resolution?

No because in specification we say that we do not guarantee the the icon returned will be MRI. We know how it works on Windows so we test it this way.

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.

Here I am not talking about specification, I am talking about the usage of it, is the instanceof+cast is helpful? It does not look good, why we cannot always return MRI?

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.

Here I am not talking about specification, I am talking about the usage of it, is the instanceof+cast is helpful?

It is necessary until all the consequences of always returning of MRI is extensively tested on all the supported platforms and it does not cause any serious regression or visual degradations. So for now i would not enforce MRI usage in this method which means use base image class instead and do the cast later.

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.

I did not get how you will be able to force use MRI later since this method is implemented as a return icon. So this choice should be made before integration.

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.

I did not get how you will be able to force use MRI later since this method is implemented as a return icon. So this choice should be made before integration.

I am not going to force MRI usage - i am assuming that there might be an implementation that does not do it hence i am going to go to the lowest common denominator - Icon.

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 in the code above you do not use the icon, you use an ImageIcon. So to get the resolution variant the user need to do something like:

    Icon icon = fsv.getSystemIcon(file, size);
    if (icon instanceof ImageIcon) {
        ImageIcon imageIcon = (ImageIcon) icon;
        if (icon.getImage() instanceof MultiResolutionImage) {
            MultiResolutionImage mri = (MultiResolutionImage) icon.getImage();
            .... = mri.getResolutionVariant(size, size);
        }
    }

Probably some other variants were discussed already and we cannot do something better?

* {@code ShellFolder} class. Whenever possible, the icon
* returned is a multi-resolution icon image,
* which allows better support for High DPI environments
* with different scaling factors.
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.

My point was that the implspec is a normative specification and we cannot refer to non-public classes in that documentation.

* <p>
* Example: <pre>
* FileSystemView fsv = FileSystemView.getFileSystemView();
* Icon icon = fsv.getSystemIcon("application.exe", 64);
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?

/*
* @test
* @bug 8182043
* @requires os.family == "windows"
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.

Other than using "explorer" what prevents us to run this test on other platforms?

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.

Other than using "explorer" what prevents us to run this test on other platforms?

Because right now it makes no sense? The functionality we are testing is implemented on Windows only. Once we extend it to other platforms we might change the test.

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.

But this is shared code, which has a specification it should work everywhere, so even if on Linux and macOS it is not implemented in the best way it should return something.

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.

But this is shared code, which has a specification it should work everywhere, so even if on Linux and macOS it is not implemented in the best way it should return something.

The stuff that is returned by the common code is already tested in UIManager tests. This issue is windows specific. This implementation at this moment of time is windows specific. Once we extend implementation - and i mean real implementation with support for multiple resolution icons - this test will be updated to reflect it. This is not a specification conformance test, it is implementation logic test.

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.

How it could be tested by the UIManager tests since this is a new method added in this class? I guess it should work just out of the box, no? What is the reason to not enable it, there are some issues?

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.

I suggest running it now and do not wait while the new tck tests will be created and run, so we will not get a tck-red right before release.

* {@code ShellFolder} class. Whenever possible, the icon
* returned is a multi-resolution icon image,
* which allows better support for High DPI environments
* with different scaling factors.
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.

As for the example on Linux, how it will work for different sizes?
Icon icon1 = fsv.getSystemIcon(new File("."), 16);
Icon icon2 = fsv.getSystemIcon(new File("."), 32);
Will the resulted icons have proper size 16 and 32?

* with different scaling factors.
*
* @param f a {@code File} object
* @param size width and height of the icon in virtual pixe