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.

Sign up for GitHub

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

Jump to bottom

8182043: Access to Windows Large Icons #2875

Closed
wants to merge 18 commits into from
Closed

8182043: Access to Windows Large Icons #2875

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
19a0efe
8182043: Access to Windows Large Icons
azuev-java Mar 8, 2021
6607b61
Small fixes
azuev-java Apr 30, 2021
4a36062
Update src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp
azuev-java Apr 30, 2021
5b2c941
Merge remote-tracking branch 'origin/master' into JDK-8182043
azuev-java May 7, 2021
237d407
Small fixes
azuev-java May 7, 2021
a481b29
Slightly changing javadoc wording
azuev-java May 11, 2021
10bae9b
Make getShell32Icon method return multi-resolition image in case of
azuev-java May 11, 2021
4cd5a50
Slight change of wording in javadoc
azuev-java May 14, 2021
911bc70
Example in JavaDoc fixed
azuev-java May 17, 2021
5922469
Fixed documentation based on CSR review feedback
azuev-java May 18, 2021
09c7f8d
Empty <p> tag before @implSpec causes warning during javadoc generation
azuev-java May 20, 2021
548dcef
Specification of getSystemIcon reworded to get rid of the non-public
azuev-java May 20, 2021
5628578
Chenged implementation specification based on CSR review
azuev-java May 21, 2021
d679dc0
Changed API to support non-square icons and updated documentation
azuev-java May 25, 2021
b3ca9da
null file now properly causes IllegalArgumentException
azuev-java May 25, 2021
4835302
Grammar fix in method documentation
azuev-java May 26, 2021
9e7bf90
Fixed small errors
azuev-java May 26, 2021
0652197
Test comment fixed.
azuev-java May 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,17 +265,22 @@ public Icon getSystemIcon(File f) {
* Icon icon = fsv.getSystemIcon(new File("application.exe"), 64);
* JLabel label = new JLabel(icon);
* </pre>
* @implSpec The default implementation gets information from the
* {@code ShellFolder} class. Whenever possible, the icon
* returned is a multi-resolution icon image,
*
* @implSpec The default implementation is platform specific
* and while we will do our best to find an icon of a specific size the exact
* match can not be guaranteed. Wherever supported by the underlying
* implementation, the icon returned is a multi-resolution icon image,
* which allows better support for High DPI environments
* with different scaling factors.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@mrserb mrserb May 20, 2021 •
edited
Loading

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.

Copy link
Member Author

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.

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

*
* @param f a {@code File} object
* @param size width and height of the icon in virtual pixels
* @param f a {@code File} object for which the icon will be retrieved
* @param size width and height of the icon in user coordinate system.
* This API only supports square icons with the edge length
* equals or greater than 1. Maximum size allowed is defined by the
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a weird way to say size >=1.

We should throw IAE for <=0 .

I also think we need to allow for rectangular icons, not bake it in to the spec in such strong terms that we don't.
I suppose it could be revised if a platform that needs it comes along but ...

Still not sure about failing if you exceed the maximum size.

Why ? Why not just return the largest possible - it is the same thing as other cases, we'll return the best match.

If that is what you already mean, then say it more clearly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, in order to allow rectangular items API needs to be slightly modified and tests and documentation should be amended accordingly. I did that, please take a look, once it is settled i will update and finalize CSR.

* underlying implementation.
* @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.
* non-existing file or a size outside of supported range.
Copy link
Contributor

Choose a reason for hiding this comment

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

pointer -> reference

* @see JFileChooser#getIcon
Copy link
Contributor

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