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 #380

Closed

Conversation

azuev-java
Copy link
Member

@azuev-java azuev-java commented Sep 28, 2020

Moving review from Mercurial. See https://mail.openjdk.java.net/pipermail/awt-dev/2020-August/016078.html for previous iteration.


Progress

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

Issue

Download

$ git fetch https://git.openjdk.java.net/jdk pull/380/head:pull/380
$ git checkout pull/380

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 28, 2020

👋 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
Copy link

openjdk bot commented Sep 28, 2020

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

@openjdk openjdk bot added 2d client-libs-dev@openjdk.org swing client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org labels Sep 28, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Sep 28, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 28, 2020

Webrevs

@victordyakov
Copy link

victordyakov commented Oct 2, 2020

@mrserb Can you review this?

@victordyakov
Copy link

victordyakov commented Oct 2, 2020

@aivanov-jdk Can you review this?

@mrserb
Copy link
Member

mrserb commented Oct 2, 2020

I still suggest to try the approach recommended here:
https://mail.openjdk.java.net/pipermail/awt-dev/2020-August/016074.html

On 29.05.2020 14:35, Alexey Ivanov wrote:

The ultimate goal of this API could be to provide an MRI which stores all the available icon sizes and loads dynamically the size that's most appropriate to the current display settings. Raymond Chen has a blog post about the format of the icon resources [1]. If we do that, we will handle all the scaling ourselves and will be able to define our own algorithm for choosing the closest match. As explained in LookupIconIdFromDirectoryEx [2], LoadIcon and LoadImage “use this function to search the specified resource data for the icon… that best fits the current display device.”> [1] https://devblogs.microsoft.com/oldnewthing/20120720-00/?p=7083
[2] https://docs.microsoft.com/en-ie/windows/win32/api/winuser/nf-winuser-lookupiconidfromdirectoryex

If an approach above works then we could request the icon of the exact existed size, and if that size is unknown we could request some predefined size like 16,24. etc

Some additional comment was sent here:
https://mail.openjdk.java.net/pipermail/awt-dev/2020-August/016081.html

@aivanov-jdk
Copy link
Member

aivanov-jdk commented Oct 6, 2020

I still suggest to try the approach recommended here:
https://mail.openjdk.java.net/pipermail/awt-dev/2020-August/016074.html

If an approach above works then we could request the icon of the exact existed size, and if that size is unknown we could request some predefined size like 16,24. etc

I agree that using LookupIconIdFromDirectoryEx would allow us to load only the sizes that are available in the icon resource rather than loading a pre-defined set of sizes, some of which may be missing and thus will be scaled by Windows.

On the other hand, I think the current approach is good enough. It provides access to larger set of icons and improves JFileChooser support for High DPI environments. However, there's still room for improvement and for unification of the way the icons are requested from the system: at this time, there are two similar functions which use different APIs.

I'd like to comment on this part from previous discussion:
https://mail.openjdk.java.net/pipermail/awt-dev/2020-August/016080.html

On 8/30/2020 12:41 PM, Sergey Bylokhov wrote:

On 30.08.2020 11:49, Alexander Zuev wrote:

I did tried that approach and haven't found any benefits over the one
i used in the fix -
it is neither faster nor more accurate.

How it could be less accurate if it allows us to read the icon of the
exact size, instead of some predefined size in our code
(16, 24, 32, 48, 64, 72, 96, 128, 256)?
As far as I understand If the file has only an icon of size 100,100 we
will never return this one icon, but instead will build the list of icons.

For once i haven't seen any resource that contains a 100x100 icon, but
yes - in this case we will ask OS to interpolate icon into sizes we know
of and will return a set of sizes.

I'd rather agree that no icon contains 100×100 size only. Yet it's pretty common that some sizes are not available in
the icon resource. So the assumption that all of these sizes are available is wrong.

As for less accurate - i mean that in most cases (in all cases i observed
actually) OS doing much better job interpolating icons than we do.

Yet if the requested size differs from the above set, the icon will be scaled twice: by the OS and then by Java. In such a case, the result of scaling the original icon will likely be better than scaling an already scaled icon.

Plus it only works when we can get the location of the file containing
this resource and we will still have to implement a fallback
method when system says that path to resource file can not be resolved.

And I am still wondering why it happens. I can imagine a situation where an icon for a file is generated on the fly by the IExtractIcon, yet it's pretty uncommon.

The usage of different APIs can be the reason why drives lost their customised icons.

As for the additional comment:

Some additional comment was sent here:
https://mail.openjdk.java.net/pipermail/awt-dev/2020-August/016081.html

It talks about tray icons. It depends on the way the icon is extracted, and to support High DPI environment, the icon must be an MRI. So I'd say the comment is not strictly relevant to the current discussion. However, this new implementation may improve the display of tray icons too if this new API is used to get the icon.

hres = pIcon->Extract(szBuf, index, &hIcon, &hIconLow, (16 << 16) + size);
if (size < 24) {
fn_DestroyIcon((HICON)hIcon);
hIcon = hIconLow;
} else {
fn_DestroyIcon((HICON)hIconLow);
Copy link
Member

@aivanov-jdk aivanov-jdk Oct 8, 2020

Choose a reason for hiding this comment

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

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.

Each time we use this function, we request two icons but we never use both of them.

Suggested change
hres = pIcon->Extract(szBuf, index, &hIcon, &hIconLow, (16 << 16) + size);
if (size < 24) {
fn_DestroyIcon((HICON)hIcon);
hIcon = hIconLow;
} else {
fn_DestroyIcon((HICON)hIconLow);
if (size < 24) {
size = 16;
}
hres = pIcon->Extract(szBuf, index, &hIcon, NULL, size);

And hIconLow can be removed.

* Icon for a file, directory, or folder as it would be displayed in
* a system file browser for the requested size.
*
* The default implementation gets information from the
Copy link
Member

@aivanov-jdk aivanov-jdk Oct 8, 2020

Choose a reason for hiding this comment

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

Suggested change
* The default implementation gets information from the
* <p>The default implementation gets information from the

* a system file browser for the requested size.
*
* The default implementation gets information from the
* <code>ShellFolder</code> class. Whenever possible the icon
Copy link
Member

@aivanov-jdk aivanov-jdk Oct 8, 2020

Choose a reason for hiding this comment

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

Suggested change
* <code>ShellFolder</code> class. Whenever possible the icon
* <code>ShellFolder</code> class. Whenever possible, the icon

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

* which will allow better scaling with different
* magnification factors.
*
* Example: <pre>
Copy link
Member

@aivanov-jdk aivanov-jdk Oct 8, 2020

Choose a reason for hiding this comment

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

Suggested change
* Example: <pre>
* <p>Example: <pre>

/**
* 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 Oct 8, 2020

Choose a reason for hiding this comment

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

Suggested change
* @param size size of the icon > 0(Valid range: 1 to 256)
* @param size size of the icon > 0 (Valid range: 1 to 256)

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 Oct 8, 2020

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.

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 Oct 8, 2020

Choose a reason for hiding this comment

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

I propose to refactor this code into a separate method which always fetches small and large icon and puts into a corresponding cache.

However, 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%?

disposeIcon(hIcon);

multiResolutionIcon.put(s, newIcon);
if (size < 8 || size > 256) {
Copy link
Member

@aivanov-jdk aivanov-jdk Oct 8, 2020

Choose a reason for hiding this comment

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

Suggested change
if (size < 8 || size > 256) {
if (size < MIN_QUALITY_ICON || size > MAX_QUALITY_ICON) {

@@ -408,7 +410,8 @@ public Object get(String key) {
try {
int i = Integer.parseInt(name);
if (i >= 0) {
return Win32ShellFolder2.getShell32Icon(i, key.startsWith("shell32LargeIcon "));
return Win32ShellFolder2.getShell32Icon(i,
key.startsWith("shell32LargeIcon ")?LARGE_ICON_SIZE : SMALL_ICON_SIZE);
Copy link
Member

@aivanov-jdk aivanov-jdk Oct 8, 2020

Choose a reason for hiding this comment

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

Suggested change
key.startsWith("shell32LargeIcon ")?LARGE_ICON_SIZE : SMALL_ICON_SIZE);
key.startsWith("shell32LargeIcon ") ? LARGE_ICON_SIZE : SMALL_ICON_SIZE);

icon.getIconWidth() + " when requested " + size);
}
}
}
Copy link
Member

@aivanov-jdk aivanov-jdk Oct 8, 2020

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?

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 6, 2020

@azuev-java This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 4, 2020

@azuev-java This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2d client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org rfr Pull request is ready for review swing client-libs-dev@openjdk.org
4 participants