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
Changes from all commits
19a0efe
6607b61
4a36062
5b2c941
237d407
a481b29
10bae9b
4cd5a50
911bc70
5922469
09c7f8d
548dcef
5628578
d679dc0
b3ca9da
4835302
9e7bf90
0652197
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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,67 @@ 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> | ||
* Example: <pre> | ||
* FileSystemView fsv = FileSystemView.getFileSystemView(); | ||
* Icon icon = fsv.getSystemIcon(new File("application.exe"), 64, 64); | ||
* JLabel label = new JLabel(icon); | ||
* </pre> | ||
* | ||
* @implSpec The available icons may be platform specific and so the | ||
* available sizes determined by the platform. Therefore an exact match | ||
* for the requested size may not be possible. | ||
* | ||
* The icon returned may be a multi-resolution icon image, | ||
* which allows better support for High DPI environments | ||
* with different scaling factors. | ||
* | ||
* @param f a {@code File} object for which the icon will be retrieved | ||
* @param width width of the icon in user coordinate system. | ||
* @param height height of the icon in user coordinate system. | ||
* @return an icon as it would be displayed by a native file chooser | ||
* or null for a non-existent or inaccessible file. | ||
* @throws IllegalArgumentException if an invalid parameter such | ||
* as a negative size or a null file reference is passed. | ||
* @see JFileChooser#getIcon | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor grammar : add "an"or "a" as appropriate, ie change to : |
||
* @see AbstractMultiResolutionImage | ||
* @see FileSystemView#getSystemIcon(File) | ||
* @since 17 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to add the IAE to the javadoc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
*/ | ||
public Icon getSystemIcon(File f, int width, int height) { | ||
if (height < 1 || width < 1) { | ||
throw new IllegalArgumentException("Icon size can not be below 1"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (minor) spacing. <1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo. Fixed. |
||
} | ||
|
||
if (f == null) { | ||
throw new IllegalArgumentException("The file reference should not be null"); | ||
} | ||
|
||
if(!f.exists()) { | ||
return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(width, height); | ||
|
||
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 | ||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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();
`
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.