Re: [OpenJDK 2D-Dev] 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: [OpenJDK 2D-Dev] 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: [OpenJDK 2D-Dev] 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: [OpenJDK 2D-Dev] 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: [OpenJDK 2D-Dev] 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: [OpenJDK 2D-Dev] 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: [OpenJDK 2D-Dev] 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: [OpenJDK 2D-Dev] 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: [OpenJDK 2D-Dev] 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: [OpenJDK 2D-Dev] 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: [OpenJDK 2D-Dev] 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: [OpenJDK 2D-Dev] 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: [OpenJDK 2D-Dev] 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=2875=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2875=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
