Re: RFR: 8182043: Access to Windows Large Icons [v16]
On Fri, 7 May 2021 17:53:49 GMT, Alexander Zuev wrote: >> This comment is also about the case of not fetching icons of sizes smaller >> than requested size. > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Thu, 27 May 2021 16:38:02 GMT, Alexander Zuev wrote:
>> Is this requirement is so important? can we return an MRI(same as on
>> Windows) which will have just one resolution? Otherwise what the user should
>> do if the requested size was 32x32 but returned image will be 21x21? Paint
>> the small icon or rescale it by the application?
>
>> Is this requirement is so important? can we return an MRI(same as on
>> Windows) which will have just one resolution?
>
> We might - when the implementation will be done on other platforms. Probably
> it will be done by me, probably - by someone else. Right now we return
> whatever we have so on Linux it is UIManager default icons for file or a
> folder (which is exactly what any file manager on Linux shows for any file
> and this is exactly what we promised in the method description). In the
> future it can change but for now it is all we can guarantee.
This is my point I think we should update this implementation to always return
MRI of the requested size, otherwise, the code example of this will look like
this:
Icon icon = fsv.getSystemIcon(file, width, height);
if (icon.getIconWidth() != width && icon.getIconHeight() != height) {
return scaleTheIconInTheSameWayAsBeforeTheFix(icon, width, height);
} else if (icon instanceof ImageIcon) {
ImageIcon imageIcon = (ImageIcon) icon;
if (icon.getImage() instanceof MultiResolutionImage) {
MultiResolutionImage mri = (MultiResolutionImage) icon.getImage();
return mri.getResolutionVariant(width, height);
} else {
return imageIcon;
}
} else {
return icon;
}
I pretty sure we can do better than the code above.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Thu, 27 May 2021 16:16:19 GMT, Sergey Bylokhov wrote: > Is this requirement is so important? can we return an MRI(same as on Windows) > which will have just one resolution? We might - when the implementation will be done on other platforms. Probably it will be done by me, probably - by someone else. Right now we return whatever we have so on Linux it is UIManager default icons for file or a folder (which is exactly what any file manager on Linux shows for any file and this is exactly what we promised in the method description). In the future it can change but for now it is all we can guarantee. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Wed, 26 May 2021 21:39:19 GMT, Alexander Zuev wrote:
>> test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 87:
>>
>>> 85: }
>>> 86:
>>> 87: if (implComplete && icon.getIconWidth() != size) {
>>
>> Why the size of the returned images might be different? The spec state the
>> size parameter in the getSystemIcon is the size of the icon in the userspace.
>
> The spec says that we are taking into consideration requested sizes but
> depending on the platform implementation exact match can not be guaranteed.
> On Windows it can so i'm checking for that.
Is this requirement is so important? can we return an MRI(same as on Windows)
which will have just one resolution? Otherwise what the user should do if the
requested size was 32x32 but returned image will be 21x21? Paint the small icon
or rescale it by the application?
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v16]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Test comment fixed. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/9e7bf901..06521978 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=15 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=14-15 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Wed, 26 May 2021 21:06:49 GMT, Sergey Bylokhov wrote: > Did somebody run the test on Linux and macOS, Does our stub implementation > there confirms the specification? I did run fill tier4 and visually compared the JFileChooser appearance on both Linux and Mac OS X - this change introduced no visual degradation to them and the test passed. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Wed, 26 May 2021 16:52:38 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with two additional > commits since the last revision: > > - Fixed small errors >Adjustments in the test > - Grammar fix in method documentation > Why does it need to do this ? I'm testing the implementation here and i know that on Windows for these well known files and folders it is possible to get icons of all the sizes mentioned in the test and we will return the correct icon size - so that's what i'm testing it this way. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Thu, 27 May 2021 00:18:13 GMT, Phil Race wrote: >> Alexander Zuev has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Fixed small errors >>Adjustments in the test >> - Grammar fix in method documentation > > test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 28: > >> 26: * @bug 8182043 >> 27: * @summary Access to Windows Large Icons >> 28: * sun.awt.shell.ShellFolder > > What's the comment here mean ? That's a leftover from the first iteration of the test. I guess i have to remove it. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Wed, 26 May 2021 16:52:38 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with two additional > commits since the last revision: > > - Fixed small errors >Adjustments in the test > - Grammar fix in method documentation test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 28: > 26: * @bug 8182043 > 27: * @summary Access to Windows Large Icons > 28: * sun.awt.shell.ShellFolder What's the comment here mean ? - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Wed, 26 May 2021 21:37:44 GMT, Alexander Zuev wrote:
>> test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 77:
>>
>>> 75: int[] sizes = new int[] {16, 32, 48, 64, 128};
>>> 76: for (int size : sizes) {
>>> 77: ImageIcon icon = (ImageIcon) fsv.getSystemIcon(file, size,
>>> size);
>>
>> Is this cast allowed without instanceof check?
>
> On Windows - yes. When implementation is complete on other platforms test
> will have to be updated depending on the way it will be implemented there.
Hmm .. I focused too much on the spec it seems. The API returns Icon and the
test should not
"know" or depend on it being anything more specific.
Why does it need to do this ?
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Wed, 26 May 2021 21:12:16 GMT, Sergey Bylokhov wrote:
>> Alexander Zuev has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Fixed small errors
>>Adjustments in the test
>> - Grammar fix in method documentation
>
> test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 77:
>
>> 75: int[] sizes = new int[] {16, 32, 48, 64, 128};
>> 76: for (int size : sizes) {
>> 77: ImageIcon icon = (ImageIcon) fsv.getSystemIcon(file, size,
>> size);
>
> Is this cast allowed without instanceof check?
On Windows - yes. When implementation is complete on other platforms test will
have to be updated depending on the way it will be implemented there.
> test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 87:
>
>> 85: }
>> 86:
>> 87: if (implComplete && icon.getIconWidth() != size) {
>
> Why the size of the returned images might be different? The spec state the
> size parameter in the getSystemIcon is the size of the icon in the userspace.
The spec says that we are taking into consideration requested sizes but
depending on the platform implementation exact match can not be guaranteed. On
Windows it can so i'm checking for that.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Wed, 26 May 2021 16:52:38 GMT, Alexander Zuev wrote:
>> Fix updated after first round of review.
>
> Alexander Zuev has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Fixed small errors
>Adjustments in the test
> - Grammar fix in method documentation
test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 77:
> 75: int[] sizes = new int[] {16, 32, 48, 64, 128};
> 76: for (int size : sizes) {
> 77: ImageIcon icon = (ImageIcon) fsv.getSystemIcon(file, size,
> size);
Is this cast allowed without instanceof check?
test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 87:
> 85: }
> 86:
> 87: if (implComplete && icon.getIconWidth() != size) {
Why the size of the returned images might be different? The spec state the size
parameter in the getSystemIcon is the size of the icon in the userspace.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 21 May 2021 19:37:19 GMT, Alexander Zuev wrote:
>> 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.
>
>> 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.
Great!
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Wed, 26 May 2021 16:52:38 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with two additional > commits since the last revision: > > - Fixed small errors >Adjustments in the test > - Grammar fix in method documentation Did somebody run the test on Linux and macOS, Does our stub implementation there confirms the specification? - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v14]
On Wed, 26 May 2021 17:40:44 GMT, Phil Race wrote:
>> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
>> line 300:
>>
>>> 298:
>>> 299: if(!f.exists()) {
>>> 300: return null;
>>
>> Shall it throw `FileNotFoundException` or `IllegalArgumentException` if the
>> file doesn't exist?
>> It could more convenient to return `null` rather than catch an exception.
>>
>> The space is missing between if and the opening parenthesis.
>
> It definitely should not be IAE. But FNFE is a reasonable idea.
> However it changes the usage since it is a checked exception.
> I'm on the fence and could go either way.
The older similar method, `getSystemIcon(File f)`, returns `null` if the file
doesn't exist. It could make sense to preserve the behaviour from this point of
view and not to make the user of the API handle FNFE; thus the new method could
easily be used instead.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v14]
On Wed, 26 May 2021 15:15:42 GMT, Alexey Ivanov wrote:
>> Alexander Zuev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> null file now properly causes IllegalArgumentException
>> Small fixed in JavaDoc
>
> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
> line 300:
>
>> 298:
>> 299: if(!f.exists()) {
>> 300: return null;
>
> Shall it throw `FileNotFoundException` or `IllegalArgumentException` if the
> file doesn't exist?
> It could more convenient to return `null` rather than catch an exception.
>
> The space is missing between if and the opening parenthesis.
It definitely should not be IAE. But FNFE is a reasonable idea.
However it changes the usage since it is a checked exception.
I'm on the fence and could go either way.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
On Wed, 26 May 2021 16:52:38 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with two additional > commits since the last revision: > > - Fixed small errors >Adjustments in the test > - Grammar fix in method documentation Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v15]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with two additional commits since the last revision: - Fixed small errors Adjustments in the test - Grammar fix in method documentation - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/b3ca9da6..9e7bf901 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=13-14 Stats: 17 lines in 2 files changed: 3 ins; 6 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v14]
On Wed, 26 May 2021 16:03:02 GMT, Alexander Zuev wrote:
>> test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 67:
>>
>>> 65: if (!gotException) {
>>> 66: throw new RuntimeException("Negative size icon should throw
>>> invalid argument exception");
>>> 67: }
>>
>> A suggestion: throw the exception inside try-block and ignore the expected
>> exception, then `gotException` can be dropped.
>> Suggestion:
>>
>> try {
>> fsv.getSystemIcon(new File("."), -1, 16);
>> throw new RuntimeException("Negative size icon should throw
>> invalid argument exception");
>> } catch (IllegalArgumentException iae) {
>> // expected
>> }
>>
>>
>> Shall the test also exercise 0 as an invalid parameter? Shall the test also
>> pass an invalid height?
>
> Fixed.
I do not think such detailed testing is required. After all it is not a full
spec conformance test.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v14]
On Wed, 26 May 2021 15:07:30 GMT, Alexey Ivanov wrote:
>> Alexander Zuev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> null file now properly causes IllegalArgumentException
>> Small fixed in JavaDoc
>
> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
> line 296:
>
>> 294:
>> 295: if (f == null){
>> 296: throw new IllegalArgumentException("File reference should
>> be specified");
>
> Shall the exception message be more specific: "The file reference should not
> be null"?
Ficed.
> test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 58:
>
>> 56:
>> 57: static void negativeTests() {
>> 58: ImageIcon icon;
>
> Can it be icon? This test doesn't use the fact that the returned object is
> `ImageIcon`.
Fixed.
> test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 67:
>
>> 65: if (!gotException) {
>> 66: throw new RuntimeException("Negative size icon should throw
>> invalid argument exception");
>> 67: }
>
> A suggestion: throw the exception inside try-block and ignore the expected
> exception, then `gotException` can be dropped.
> Suggestion:
>
> try {
> fsv.getSystemIcon(new File("."), -1, 16);
> throw new RuntimeException("Negative size icon should throw
> invalid argument exception");
> } catch (IllegalArgumentException iae) {
> // expected
> }
>
>
> Shall the test also exercise 0 as an invalid parameter? Shall the test also
> pass an invalid height?
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v14]
On Tue, 25 May 2021 23:36:43 GMT, Alexander Zuev wrote:
>> Fix updated after first round of review.
>
> Alexander Zuev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> null file now properly causes IllegalArgumentException
> Small fixed in JavaDoc
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
296:
> 294:
> 295: if (f == null){
> 296: throw new IllegalArgumentException("File reference should be
> specified");
Shall the exception message be more specific: "The file reference should not be
null"?
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
300:
> 298:
> 299: if(!f.exists()) {
> 300: return null;
Shall it throw `FileNotFoundException` or `IllegalArgumentException` if the
file doesn't exist?
It could more convenient to return `null` rather than catch an exception.
The space is missing between if and the opening parenthesis.
test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 58:
> 56:
> 57: static void negativeTests() {
> 58: ImageIcon icon;
Can it be icon? This test doesn't use the fact that the returned object is
`ImageIcon`.
test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 67:
> 65: if (!gotException) {
> 66: throw new RuntimeException("Negative size icon should throw
> invalid argument exception");
> 67: }
A suggestion: throw the exception inside try-block and ignore the expected
exception, then `gotException` can be dropped.
Suggestion:
try {
fsv.getSystemIcon(new File("."), -1, 16);
throw new RuntimeException("Negative size icon should throw invalid
argument exception");
} catch (IllegalArgumentException iae) {
// expected
}
Shall the test also exercise 0 as an invalid parameter? Shall the test also
pass an invalid height?
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v14]
On Tue, 25 May 2021 23:36:43 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > null file now properly causes IllegalArgumentException > Small fixed in JavaDoc Approving but fix the grammar! src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 282: > 280: * @return an icon as it would be displayed by a native file chooser > 281: * or null if invalid parameters are passed such as reference to a > 282: * non-existent file. grammar : "a reference" src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 285: > 283: * @throws IllegalArgumentException if invalid parameter such > 284: * as negative size or null file reference is passed. > 285: * @see JFileChooser#getIcon minor grammar : add "an"or "a" as appropriate, ie change to : "if an invalid parameter such a negative size or a null file reference" - Marked as reviewed by prr (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v13]
On Tue, 25 May 2021 23:17:39 GMT, Phil Race wrote:
>> Alexander Zuev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Changed API to support non-square icons and updated documentation
>> accordingly
>> Fixed test to support updated API
>> Added negative test cases
>
> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
> line 282:
>
>> 280: * @return an icon as it would be displayed by a native file chooser
>> 281: * or null if invalid parameters are passed such as reference to a
>> 282: * non-existing file.
>
> 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 ?
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.
> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
> line 286:
>
>> 284: * @see AbstractMultiResolutionImage
>> 285: * @see FileSystemView#getSystemIcon(File)
>> 286: * @since 17
>
> You need to add the IAE to the javadoc.
Done.
> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
> line 290:
>
>> 288: public Icon getSystemIcon(File f, int width, int height) {
>> 289: if (height <1 || width < 1) {
>> 290: throw new IllegalArgumentException("Icon size can not be
>> below 1");
>
> (minor) spacing. <1
Typo. Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v14]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: null file now properly causes IllegalArgumentException Small fixed in JavaDoc - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/d679dc01..b3ca9da6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=12-13 Stats: 9 lines in 1 file changed: 6 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v13]
On Tue, 25 May 2021 22:01:36 GMT, Alexander Zuev wrote:
>> Fix updated after first round of review.
>
> Alexander Zuev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Changed API to support non-square icons and updated documentation
> accordingly
> Fixed test to support updated API
> Added negative test cases
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
282:
> 280: * @return an icon as it would be displayed by a native file chooser
> 281: * or null if invalid parameters are passed such as reference to a
> 282: * non-existing file.
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 ?
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
286:
> 284: * @see AbstractMultiResolutionImage
> 285: * @see FileSystemView#getSystemIcon(File)
> 286: * @since 17
You need to add the IAE to the javadoc.
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
290:
> 288: public Icon getSystemIcon(File f, int width, int height) {
> 289: if (height <1 || width < 1) {
> 290: throw new IllegalArgumentException("Icon size can not be
> below 1");
(minor) spacing. <1
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
294:
> 292:
> 293: if (f == null || !f.exists()) {
> 294: return null;
I suggested the a null File ought to be IAE too - this is showing up in the
conversation thread but not here.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v12]
On Fri, 21 May 2021 22:07:30 GMT, Phil Race wrote: >> Alexander Zuev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Chenged implementation specification based on CSR review > > src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java > line 280: > >> 278: * @param size width and height of the icon in user coordinate >> system. >> 279: * This API only supports square icons with the edge length >> 280: * equals or greater than 1. Maximum size allowed is defined by the > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v13]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Changed API to support non-square icons and updated documentation accordingly Fixed test to support updated API Added negative test cases - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/56285783..d679dc01 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=11-12 Stats: 59 lines in 4 files changed: 34 ins; 3 del; 22 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v12]
On Fri, 21 May 2021 21:41:44 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Chenged implementation specification based on CSR review > > if the aspect ratio is consistent as I expect is typical we should support > > that > > Ok, i see your point. I can do that for sure, it is just a change of API for > now, no implementation will be affected since Windows does not support > non-square icons for files and passage about exact size can not be guaranteed > will save us from "i requested 128x20 and you returned 128x128" complaints. > One will get what platform is able to serve. However, i do not see why we > should limit the maximum requested size. Did I say that? I think I said something more like the opposite. The spec you wrote seems to suggest that if the user exceeds some unknown size that you aren't telling them, you'll return null, leaving them in a guessing game as to what is the maximum. That can't be right. So either you provide another API telling them the maximum size, or, if they exceed the maximum size you specify that the API will return the best fit, just like in other cases. The question I raised was around what "size" means. If you meant the API to return the size to which the image will *scale* when drawn, you need to make this very clear to the developer, or since you don't have an API telling them the max available, they'll respond by asking for ridiculous sizes to get the best quality image. So what do you intend ? > > > I can't agree that IAE is wrong for a negative size. > > How about non-existing file or file? Also IAE? First .. "inaccessible" should not be distinguishable from "non-existing". If I ask for a file that the system has hidden from me for whatever reason. Now since this is something where the app may not know that for platform reasons, I think a file that cannot be "found" should not throw IAE, just return null. Passing in null directly tho' probably *should* be IAE because what are they doing that for ?? src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 280: > 278: * @param size width and height of the icon in user coordinate system. > 279: * This API only supports square icons with the edge length > 280: * equals or greater than 1. Maximum size allowed is defined by the 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. src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line 284: > 282: * @return an icon as it would be displayed by a native file chooser > 283: * or null if invalid parameters are passed such as pointer to a > 284: * non-existing file or a size outside of supported range. pointer -> reference - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v11]
On Fri, 21 May 2021 21:24:10 GMT, Phil Race wrote: > if the aspect ratio is consistent as I expect is typical we should support > that Ok, i see your point. I can do that for sure, it is just a change of API for now, no implementation will be affected since Windows does not support non-square icons for files and passage about exact size can not be guaranteed will save us from "i requested 128x20 and you returned 128x128" complaints. One will get what platform is able to serve. However, i do not see why we should limit the maximum requested size. > I can't agree that IAE is wrong for a negative size. How about non-existing file or file? Also IAE? - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v12]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Chenged implementation specification based on CSR review - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/548dcef1..56285783 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=10-11 Stats: 5 lines in 1 file changed: 1 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v11]
On Thu, 20 May 2021 22:25:05 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Specification of getSystemIcon reworded to get rid of the non-public > class reference and to better describe some cornor cases I was reviewing the CSR so comments in the CSR are the obvious place for that. We can continue it here if you like 1) Will you be updating to explain how we map to (theoretical) non-square icons ? > As for the square icons - yes, we imply that the greater dimension is > specified so in case of 256x128 icon > the 256x256 icon will be returned with the 256x128 bitmap inside and the > 256x256 reported size. FWIW I think now I understand that you mean we will always return a square icon, that may be an issue. Imagine if the OS has icons that are 256x128 or 128x256 and we need to display a bunch of them side by side and tow by row you will find it impossible to layout well. Surely we should return the same if we can It is *theoretically* possible that the platform will do something bananas like return 128x256 then 256x256 then 384x256 alternative images in which case we'd need logic to handle that as sensibly as possible, acc. to the way MRI handles cuch cases, but if the aspect ratio is consistent as I expect is typical we should support that 2) I can't agree that IAE is wrong for a negative size. The argument that a developer may make a mistake is not sufficient. Virtually every Image related API we have throws an exception (usually IAE) if you pass <= 0 and folks get that right all the time. Allowing it here would be anomalous. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v11]
On Fri, 21 May 2021 20:10:55 GMT, Phil Race wrote: > Please note that I added a number of comments in the CSR itself. > I had suggestions about what to do in many of the cases but there's a couple > of cases where > I could not either divine what was intended or know the best way to phrase it. I am responding to CSR comments right now but honestly i would prefer to do a review in one place - preferably here. For some reason i do not receive any notifications from JBS about new comments in CSR so i had to monitor it manually by refreshing the page. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v11]
On Thu, 20 May 2021 22:25:05 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Specification of getSystemIcon reworded to get rid of the non-public > class reference and to better describe some cornor cases Please note that I added a number of comments in the CSR itself. I had suggestions about what to do in many of the cases but there's a couple of cases where I could not either divine what was intended or know the best way to phrase it. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 21 May 2021 19:11:45 GMT, Alexey Ivanov wrote:
>> But how you got them via this method? I am not sure what parameters should
>> be passed to it.
>
> 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.
> 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.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 21 May 2021 18:16:36 GMT, Sergey Bylokhov wrote: >>> 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 >>  > > But how you got them via this method? I am not sure what parameters should be > passed to it. 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Fri, 21 May 2021 01:11:38 GMT, Sergey Bylokhov wrote: >>> 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. > > 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? 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 21 May 2021 06:32:02 GMT, Alexander Zuev wrote: >> 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. > >> 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 >  But how you got them via this method? I am not sure what parameters should be passed to it. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 21 May 2021 05:42:46 GMT, Sergey Bylokhov wrote: > 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  - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 21 May 2021 04:15:41 GMT, Alexander Zuev wrote: >> Maybe we can test this case by just reusing this method where the >> "sf.getIcon(largeicon)" previously used in the WindowsPlacesBar class? That >> old method has the special code for "folder.isLibrary()" so I assume it >> supported the Libraries. And if we will start to use the new method then all >> old tests for the JFileChooser will start to test it. > > What is the library a collection folder such as Recent Items or Documents? 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 21 May 2021 03:21:19 GMT, Sergey Bylokhov wrote: >> The JFileChooser supports the libraries since it allows navigation inside >> them. It is done via ShellFolder which extends the file class. The >> FileSystemView feeds the JFileChooser by the data, so it may returns >> "non-file" objects. >> For example the FileSystemView.getRoots() on WIndows will return "new >> Win32ShellFolder2(DESKTOP)". >> >> Usually, when the FileSystemView gets a file object it checks that the file >> is "instance ShellFolder". >> For example, in the newly added method the line "sf = >> ShellFolder.getShellFolder(f);" will check that the file is ShellFolder and >> return it as-is without checking of file existence, and really that path may >> not exist. but if it is a plain file it will check that. >> >> This is why I have asked about virtual folders, do we test them or we can >> consider them as "not existed"? > > Maybe we can test this case by just reusing this method where the > "sf.getIcon(largeicon)" previously used in the WindowsPlacesBar class? That > old method has the special code for "folder.isLibrary()" so I assume it > supported the Libraries. And if we will start to use the new method then all > old tests for the JFileChooser will start to test it. What is the library a collection folder such as Recent Items or Documents? - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 21 May 2021 02:45:15 GMT, Sergey Bylokhov wrote: >> No, libraries are not supported. >> >> I see no contradiction here: `JFileChooser` uses Windows Shell API to >> enumerate objects and navigate the shell namespace. But it does not return >> non-file objects, does it? >> >> The new method accepts `File` object which implies it's a file system object >> rather than an arbitrary object in Windows Shell namespace which cannot be >> represented in Java. > > The JFileChooser supports the libraries since it allows navigation inside > them. It is done via ShellFolder which extends the file class. The > FileSystemView feeds the JFileChooser by the data, so it may returns > "non-file" objects. > For example the FileSystemView.getRoots() on WIndows will return "new > Win32ShellFolder2(DESKTOP)". > > Usually, when the FileSystemView gets a file object it checks that the file > is "instance ShellFolder". > For example, in the newly added method the line "sf = > ShellFolder.getShellFolder(f);" will check that the file is ShellFolder and > return it as-is without checking of file existence, and really that path may > not exist. but if it is a plain file it will check that. > > This is why I have asked about virtual folders, do we test them or we can > consider them as "not existed"? Maybe we can test this case by just reusing this method where the "sf.getIcon(largeicon)" previously used in the WindowsPlacesBar class? That old method has the special code for "folder.isLibrary()" so I assume it supported the Libraries. And if we will start to use the new method then all old tests for the JFileChooser will start to test it. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Thu, 20 May 2021 18:51:54 GMT, Alexey Ivanov wrote: >> So the libraries are not supported? I doubt since JFileChooser should >> support them, and supports of it is one of the reasons why we use shell >> folder to iterate the folders and do not use the javaio. > > No, libraries are not supported. > > I see no contradiction here: `JFileChooser` uses Windows Shell API to > enumerate objects and navigate the shell namespace. But it does not return > non-file objects, does it? > > The new method accepts `File` object which implies it's a file system object > rather than an arbitrary object in Windows Shell namespace which cannot be > represented in Java. The JFileChooser supports the libraries since it allows navigation inside them. It is done via ShellFolder which extends the file class. The FileSystemView feeds the JFileChooser by the data, so it may returns "non-file" objects. For example the FileSystemView.getRoots() on WIndows will return "new Win32ShellFolder2(DESKTOP)". Usually, when the FileSystemView gets a file object it checks that the file is "instance ShellFolder". For example, in the newly added method the line "sf = ShellFolder.getShellFolder(f);" will check that the file is ShellFolder and return it as-is without checking of file existence, and really that path may not exist. but if it is a plain file it will check that. This is why I have asked about virtual folders, do we test them or we can consider them as "not existed"? - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 23:03:02 GMT, Alexander Zuev wrote: >> 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. > >> 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. 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? - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 22:59:43 GMT, Alexander Zuev wrote:
>> 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 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.
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?
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 22:38:48 GMT, Sergey Bylokhov wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 22:37:08 GMT, Sergey Bylokhov wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 18:49:53 GMT, Alexander Zuev wrote: >> test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 27: >> >>> 25: * @test >>> 26: * @bug 8182043 >>> 27: * @requires os.family == "windows" >> >> Other than using "explorer" what prevents us to run this test on other >> platforms? > >> 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 19:25:38 GMT, Alexander Zuev wrote: >> 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? > >> 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 19:35:57 GMT, Phil Race wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v11]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Specification of getSystemIcon reworded to get rid of the non-public class reference and to better describe some cornor cases - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/09c7f8d7..548dcef1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=09-10 Stats: 11 lines in 1 file changed: 5 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 19:22:50 GMT, Alexander Zuev wrote: >> 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. > >> 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 17:29:55 GMT, Sergey Bylokhov wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 17:33:08 GMT, Sergey Bylokhov wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 16:09:14 GMT, Sergey Bylokhov wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Thu, 20 May 2021 17:37:20 GMT, Sergey Bylokhov wrote: >>> Are we sure that all possible paths can be pointed by the file object? >> >> We specify that we return the icon for a file. If path can not be resolved >> in the file object we can not return the icon for it. > > So the libraries are not supported? I doubt since JFileChooser should support > them, and supports of it is one of the reasons why we use shell folder to > iterate the folders and do not use the javaio. No, libraries are not supported. I see no contradiction here: `JFileChooser` uses Windows Shell API to enumerate objects and navigate the shell namespace. But it does not return non-file objects, does it? The new method accepts `File` object which implies it's a file system object rather than an arbitrary object in Windows Shell namespace which cannot be represented in Java. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Thu, 20 May 2021 16:45:35 GMT, Alexander Zuev wrote: >> 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. > >> Are we sure that all possible paths can be pointed by the file object? > > We specify that we return the icon for a file. If path can not be resolved in > the file object we can not return the icon for it. So the libraries are not supported? I doubt since JFileChooser should support them, and supports of it is one of the reasons why we use shell folder to iterate the folders and do not use the javaio. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 17:25:33 GMT, Alexander Zuev wrote:
>> test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 72:
>>
>>> 70: if (icon.getImage() instanceof MultiResolutionImage) {
>>> 71: MultiResolutionImage mri = (MultiResolutionImage)
>>> icon.getImage();
>>> 72: if (mri.getResolutionVariant(size, size) == null) {
>>
>> 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?
>
>> 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.
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?
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 16:58:36 GMT, Alexander Zuev wrote: >> 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. > >> 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 16:14:42 GMT, Sergey Bylokhov wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 15:57:04 GMT, Sergey Bylokhov wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 16:53:06 GMT, Alexander Zuev wrote:
>> 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?
>
> 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.
> 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.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 16:13:41 GMT, Sergey Bylokhov wrote:
>> My point was that the implspec is a normative specification and we cannot
>> refer to non-public classes in that documentation.
>
> 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?
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.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Thu, 20 May 2021 16:43:05 GMT, Alexander Zuev wrote: >> 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? > > 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. > Are we sure that all possible paths can be pointed by the file object? We specify that we return the icon for a file. If path can not be resolved in the file object we can not return the icon for it. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Thu, 20 May 2021 16:06:37 GMT, Sergey Bylokhov wrote: >> 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? > > 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? 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 07:30:00 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Empty tag before @implSpec causes warning during javadoc generation test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 27: > 25: * @test > 26: * @bug 8182043 > 27: * @requires os.family == "windows" Other than using "explorer" what prevents us to run this test on other platforms? - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 16:01:32 GMT, Sergey Bylokhov wrote:
>>> 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.
>
> My point was that the implspec is a normative specification and we cannot
> refer to non-public classes in that documentation.
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?
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 14:55:06 GMT, Alexey Ivanov wrote: >> 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. > > 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. BTW this is why I recommended filing a CSR after the fix is fully discussed and agreed upon. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 07:30:00 GMT, Alexander Zuev wrote:
>> Fix updated after first round of review.
>
> Alexander Zuev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Empty tag before @implSpec causes warning during javadoc generation
test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 72:
> 70: if (icon.getImage() instanceof MultiResolutionImage) {
> 71: MultiResolutionImage mri = (MultiResolutionImage)
> icon.getImage();
> 72: if (mri.getResolutionVariant(size, size) == null) {
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?
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 10:36:39 GMT, Alexander Zuev wrote:
>> 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.
>
>> 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.
The implspec is a specification and we cannot refer to non-public classes in
that documentation.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Thu, 20 May 2021 07:40:35 GMT, Sergey Bylokhov wrote: >> Good catch! Yes, fixed both here and in CSR. > > 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? 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? - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 08:26:07 GMT, Alexander Zuev wrote: >> 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. > > 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. 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*. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 08:25:25 GMT, Sergey Bylokhov wrote:
> 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.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 08:13:21 GMT, Alexander Zuev wrote: >> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java >> line 272: >> >>> 270: * returned is a multi-resolution icon image, >>> 271: * which allows better support for High DPI environments >>> 272: * with different scaling factors. >> >> 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? > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 08:18:02 GMT, Sergey Bylokhov wrote: >>> 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. > > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 08:07:48 GMT, Alexander Zuev wrote:
>> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
>> line 275:
>>
>>> 273: *
>>> 274: * @param f a {@code File} object
>>> 275: * @param size width and height of the icon in virtual pixels
>>
>> 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?
>
>> 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.
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.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 07:47:02 GMT, Sergey Bylokhov wrote: >> Alexander Zuev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Empty tag before @implSpec causes warning during javadoc generation > > src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java > line 272: > >> 270: * returned is a multi-resolution icon image, >> 271: * which allows better support for High DPI environments >> 272: * with different scaling factors. > > 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? 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 07:46:28 GMT, Sergey Bylokhov wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
On Thu, 20 May 2021 07:30:00 GMT, Alexander Zuev wrote:
>> Fix updated after first round of review.
>
> Alexander Zuev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Empty tag before @implSpec causes warning during javadoc generation
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
272:
> 270: * returned is a multi-resolution icon image,
> 271: * which allows better support for High DPI environments
> 272: * with different scaling factors.
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?
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
275:
> 273: *
> 274: * @param f a {@code File} object
> 275: * @param size width and height of the icon in virtual pixels
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?
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Mon, 17 May 2021 05:08:13 GMT, Alexander Zuev wrote:
>> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
>> line 271:
>>
>>> 269: * Example:
>>> 270: * FileSystemView fsv = FileSystemView.getFileSystemView();
>>> 271: * Icon icon = fsv.getSystemIcon("application.exe", 64);
>>
>> Shouldn't the first parameter be a File instance instead of String?
>> `Icon icon = fsv.getSystemIcon(new File("application.exe"), 64);`
>
> Good catch! Yes, fixed both here and in CSR.
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?
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v10]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Empty tag before @implSpec causes warning during javadoc generation - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/59224696..09c7f8d7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=08-09 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v9]
On Tue, 18 May 2021 00:29:21 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Fixed documentation based on CSR review feedback Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v9]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Fixed documentation based on CSR review feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/911bc70b..59224696 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=07-08 Stats: 14 lines in 1 file changed: 7 ins; 6 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v8]
On Mon, 17 May 2021 05:13:06 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Example in JavaDoc fixed Marked as reviewed by azvegint (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Sun, 16 May 2021 18:49:24 GMT, Alexander Zvegintsev
wrote:
>> Alexander Zuev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Slight change of wording in javadoc
>> Fixed Win32ShellFolder2.getSystemIcon scaling issue
>
> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
> line 271:
>
>> 269: * Example:
>> 270: * FileSystemView fsv = FileSystemView.getFileSystemView();
>> 271: * Icon icon = fsv.getSystemIcon("application.exe", 64);
>
> Shouldn't the first parameter be a File instance instead of String?
> `Icon icon = fsv.getSystemIcon(new File("application.exe"), 64);`
Good catch! Yes, fixed both here and in CSR.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v8]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Example in JavaDoc fixed - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/4cd5a508..911bc70b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=06-07 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 14 May 2021 19:46:03 GMT, Alexander Zuev wrote:
>> Fix updated after first round of review.
>
> Alexander Zuev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Slight change of wording in javadoc
> Fixed Win32ShellFolder2.getSystemIcon scaling issue
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
271:
> 269: * Example:
> 270: * FileSystemView fsv = FileSystemView.getFileSystemView();
> 271: * Icon icon = fsv.getSystemIcon("application.exe", 64);
Shouldn't the first parameter be a File instance instead of String?
`Icon icon = fsv.getSystemIcon(new File("application.exe"), 64);`
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
On Fri, 14 May 2021 19:46:03 GMT, Alexander Zuev wrote: >> Fix updated after first round of review. > > Alexander Zuev has updated the pull request incrementally with one additional > commit since the last revision: > > Slight change of wording in javadoc > Fixed Win32ShellFolder2.getSystemIcon scaling issue Marked as reviewed by aivanov (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v7]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Slight change of wording in javadoc Fixed Win32ShellFolder2.getSystemIcon scaling issue - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/10bae9be..4cd5a508 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=05-06 Stats: 5 lines in 2 files changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Fri, 14 May 2021 13:41:31 GMT, Alexey Ivanov wrote:
>> I see - but still it has to be solved in the getShell32Icon method - which
>> i'm going to do in a moment.
>
> `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;
> }
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v6]
On Fri, 14 May 2021 13:30:13 GMT, Alexey Ivanov wrote:
>> Alexander Zuev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Make getShell32Icon method return multi-resolition image in case of
>> requested icon size and actual icon size mismatch.
>
> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
> line 266:
>
>> 264: * {@code ShellFolder} class. Whenever possible, the icon
>> 265: * returned will be a multi-resolution icon image,
>> 266: * which will allow better support for High DPI environments
>
> 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.
Agree.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 21:34:59 GMT, Alexander Zuev wrote:
>> 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](https://bugs.openjdk.java.net/browse/JDK-8151385): _[hidpi]
>> JOptionPane-Icons only partially visible when using Windows 10 L&F_.
>
> I see - but still it has to be solved in the getShell32Icon method - which
> i'm going to do in a moment.
`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;
}
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v6]
On Tue, 11 May 2021 21:41:15 GMT, Alexander Zuev wrote:
>> Fix updated after first round of review.
>
> Alexander Zuev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Make getShell32Icon method return multi-resolition image in case of
> requested icon size and actual icon size mismatch.
Changes requested by aivanov (Reviewer).
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
266:
> 264: * {@code ShellFolder} class. Whenever possible, the icon
> 265: * returned will be a multi-resolution icon image,
> 266: * which will allow better support for High DPI environments
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.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 19:36:12 GMT, Alexey Ivanov wrote: >> 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. > > 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. For documentation purposes, [JDK-8266948](https://bugs.openjdk.java.net/browse/JDK-8266948): ShellFolder2::getIconResource does not release the library loaded. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v6]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Make getShell32Icon method return multi-resolition image in case of requested icon size and actual icon size mismatch. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/a481b29b..10bae9be Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=04-05 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 20:01:37 GMT, Alexey Ivanov wrote:
>> 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.
>
> 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](https://bugs.openjdk.java.net/browse/JDK-8151385): _[hidpi]
> JOptionPane-Icons only partially visible when using Windows 10 L&F_.
I see - but still it has to be solved in the getShell32Icon method - which i'm
going to do in a moment.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 10:07:30 GMT, Alexey Ivanov wrote: >> Alexander Zuev has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains five additional >> commits since the last revision: >> >> - Small fixes >>Added testing for MultiResolutionImage >> - Merge remote-tracking branch 'origin/master' into JDK-8182043 >> - Update src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp >> >>Select one icon at a time. >> >>Co-authored-by: Alexey Ivanov >> <70774172+aivanov-...@users.noreply.github.com> >> - Small fixes >>Remived size parameter from makeIcon >>Removed useVGAColors usage as irrelevant since it is ignored on all >>supported platforms now >> - 8182043: Access to Windows Large Icons >>Fix updated based on the previous review. > > src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java > line 267: > >> 265: * returned will be a multi-resolution icon image, >> 266: * which will allow better scaling with different >> 267: * scaling factors. > > “…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. Yes, much better. I updated the wording and once the PR is approved i will fix it in CSR too. > src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line > 1098: > >> 1096: imageCache = getLargeIcon ? >> smallSystemImages : largeSystemImages; >> 1097: } >> 1098: newIcon2 = >> imageCache.get(Integer.valueOf(index)); > > Probably, explicit boxing is unnecessary. Removed. > src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp line 929: > >> 927: } >> 928: HRESULT hres; >> 929: IExtractIconW* pIcon; > > `pIcon->Release()` must be called before returning as done in `extractIcon`. Fixed. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v5]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: Slightly changing javadoc wording Removing unnecessary boxing of int Releasing the pIcon in native code properly - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/237d4070..a481b29b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2875&range=03-04 Stats: 6 lines in 3 files changed: 3 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/2875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2875/head:pull/2875 PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 18:48:31 GMT, Alexander Zuev wrote:
>> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line
>> 1196:
>>
>>> 1194: Image icon = makeIcon(hIcon);
>>> 1195: disposeIcon(hIcon);
>>> 1196: return icon;
>>
>> 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`.
>
> 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.
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](https://bugs.openjdk.java.net/browse/JDK-8151385): _[hidpi]
JOptionPane-Icons only partially visible when using Windows 10 L&F_.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 18:50:13 GMT, Alexander Zuev wrote:
>> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line
>> 1192:
>>
>>> 1190: */
>>> 1191: static Image getShell32Icon(int iconID, int size) {
>>> 1192: long hIcon = getIconResource("shell32.dll", iconID, size,
>>> size);
>>
>> 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.
>
> 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.
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.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: RFR: 8182043: Access to Windows Large Icons [v4]
On Tue, 11 May 2021 10:53:06 GMT, Alexey Ivanov wrote:
>> Alexander Zuev has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains five additional
>> commits since the last revision:
>>
>> - Small fixes
>>Added testing for MultiResolutionImage
>> - Merge remote-tracking branch 'origin/master' into JDK-8182043
>> - Update src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp
>>
>>Select one icon at a time.
>>
>>Co-authored-by: Alexey Ivanov
>> <70774172+aivanov-...@users.noreply.github.com>
>> - Small fixes
>>Remived size parameter from makeIcon
>>Removed useVGAColors usage as irrelevant since it is ignored on all
>>supported platforms now
>> - 8182043: Access to Windows Large Icons
>>Fix updated based on the previous review.
>
> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line
> 1192:
>
>> 1190: */
>> 1191: static Image getShell32Icon(int iconID, int size) {
>> 1192: long hIcon = getIconResource("shell32.dll", iconID, size,
>> size);
>
> 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.
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.
> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line
> 1196:
>
>> 1194: Image icon = makeIcon(hIcon);
>> 1195: disposeIcon(hIcon);
>> 1196: return icon;
>
> 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`.
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.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
