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 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -260,17 +260,17 @@ 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
* Example: <pre>
* FileSystemView fsv = FileSystemView.getFileSystemView();
* Icon icon = fsv.getSystemIcon(new File("application.exe"), 64);
* JLabel label = new JLabel(icon);
* </pre>
* <p>
* @implSpec The default implementation gets information from the
* {@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.

This comment has been minimized.

@mrserb

mrserb May 20, 2021
Member

Is the above text correct on all platforms? If it is not always MRI then how the user should use the icon? instanceof+cast? BTW an example does not show how to solve the bug itself, on how to access the "large icons".

Need to clarify: the implSpec is a part of the specification so can we point the non public "ShellFolder" class?

This comment has been minimized.

@azuev-java

azuev-java May 20, 2021
Author Member

implSpec marks that the paragraph below describes the details and logic of the default implementation and not the API specification. This tag also says that it can be changed in overriding or extending methods so it is Ok to specify non-public class to help describe the implementation specifics.

As for the correctness on all platforms - that's the end goal of this new method and i believe it should be implemented this way everywhere where technically possible. But exact implementation on all platforms except Windows is outside of the scope of this exact changeset.

This comment has been minimized.

@mrserb

mrserb May 20, 2021
Member

The @implSpec is part of the specification, it is different from the @implNote, no?
https://bugs.openjdk.java.net/browse/JDK-8266541?focusedCommentId=14419988&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14419988

If we will specify this method in a way that will require support on all platforms we will get tck-red immediately after this push.

This comment has been minimized.

@azuev-java

azuev-java May 20, 2021
Author Member

The @implSpec is part of the specification, it is different from the @implNote, no?
https://bugs.openjdk.java.net/browse/JDK-8266541?focusedCommentId=14419988&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14419988

If we will specify this method in a way that will require support on all platforms we will get tck-red immediately after this push.

Not exactly. The implNote is a note for future maintainers or people who will extend the functionality of the method. There will be no tck-red because the method is working and we did noted that we are taking into consideration the icon size and whenever technical possible we should return the multiresolution icon. So, for example, on Linux code
` FileSystemView fsv = FileSystemView.getFileSystemView();

    Icon icon = fsv.getSystemIcon(new File("."));
    Icon icon2 = fsv.getSystemIcon(new File("."), 16);
    System.out.println("icon = " + icon);
    System.out.println("icon2 = " + icon2);

`
will get icon and icon2 as the same single-resolution icon - but that will change when underlying implementation will be fixed. Right now it is not technical possible to return multi-resolution icon - we do not do it on Linux. Implementing the underlaying code for different system, as i said, is outside of the scope of this change.

This comment has been minimized.

@mrserb

mrserb May 20, 2021
Member

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

This comment has been minimized.

@mrserb

mrserb May 20, 2021
Member

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?

This comment has been minimized.

@azuev-java

azuev-java May 20, 2021
Author Member

No they will have the same size. That's why the broad wording is used that we take a requested size into consideration but we will return the best possible icon we can get and we do not guarantee that the icon size will change the outcome. Even on Windows if we request icon if sizes 1, 2, 3 and 4 the icon will be basically the same - minimal quality icon available.

This comment has been minimized.

@azuev-java

azuev-java May 20, 2021
Author Member

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

implSpec may describe the behavior of the default implementation and if it means referring the non-public API to clarify the behavior of this method i do not see any issue here.

This comment has been minimized.

@mrserb

mrserb May 20, 2021
Member

But it is still part of the specification unlike implnote/apinote, and we cannot use non-public classes there, since other JavaSE implementations may not have this class. see discussion on the link above.

This comment has been minimized.

@azuev-java

azuev-java May 20, 2021
Author Member

But it is still part of the specification unlike implnote/apinote

I think you can suggest usage of the implNote here - i am going from the initial description of the reason implSpec in the JEP saying that implementation and logic of it may vary between different Java SE implementations and even between different platforms so i am going with the original reasoning for implSpec tag existence. If you disagree, please file the separate issue for spec amendment once this PR is integrated. Or we can discuss it and i file follow-up bug - whatever you prefer, but i honestly think it is not a blocker and that this technical issue linger in this state for way too long.

This comment has been minimized.

@prrace

prrace May 20, 2021
Contributor

We absolutely should NOT reference a non-API class in the public javadoc, no matter whether
it is an implNote or implSpec.
Additionally, if you add or remove an implNote or implSpec or update it for something much more than a typo you will need to revise the CSR.

Really I would need to see what the actual delta ends up being to be sure for this case.

This comment has been minimized.

@azuev-java

azuev-java May 20, 2021
Author Member

Really I would need to see what the actual delta ends up being to be sure for this case.

I have updated the method documentation. Could you please take a look before i finalized the CSR again? I am really trying to push this functionality into 17 and there's not much time left. Thanks.

* <p>
* Example: <pre>
* FileSystemView fsv = FileSystemView.getFileSystemView();
* Icon icon = fsv.getSystemIcon(new File("application.exe"), 64);
* JLabel label = new JLabel(icon);
* </pre>
*
* @param f a {@code File} object
* @param size width and height of the icon in virtual pixels

This comment has been minimized.

@mrserb

mrserb May 20, 2021
Member Outdated

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?

This comment has been minimized.

@azuev-java

azuev-java May 20, 2021
Author Member Outdated

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.

This comment has been minimized.

@mrserb

mrserb May 20, 2021
Member Outdated

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.

This comment has been minimized.

@azuev-java

azuev-java May 20, 2021
Author Member Outdated

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.

This comment has been minimized.

@aivanov-jdk

aivanov-jdk May 20, 2021
Member Outdated

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.

This comment has been minimized.

@mrserb

mrserb May 20, 2021
Member Outdated

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

This comment has been minimized.

@azuev-java

azuev-java May 20, 2021
Author Member Outdated

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.

@@ -279,6 +279,7 @@ public Icon getSystemIcon(File f) {
* non-existing file.

This comment has been minimized.

@prrace

prrace May 25, 2021
Contributor Outdated

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 ?

This comment has been minimized.

@azuev-java

azuev-java May 25, 2021
Author Member Outdated

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

This comment has been minimized.

@prrace

prrace May 26, 2021
Contributor

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
* @see FileSystemView#getSystemIcon(File)
* @since 17

This comment has been minimized.

@prrace

prrace May 25, 2021
Contributor

You need to add the IAE to the javadoc.

This comment has been minimized.

@azuev-java

azuev-java May 25, 2021
Author Member

Done.

*/
public Icon getSystemIcon(File f, int size) {