Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]
On Fri, 7 May 2021 17:30:15 GMT, Alexander Zuev wrote: >> src/java.desktop/share/classes/sun/awt/shell/ShellFolder.java line 213: >> >>> 211: * Returns the icon of the specified size used to display this >>> shell folder. >>> 212: * >>> 213: * @param size size of the icon > 0 (Valid range: 1 to 256) >> >> I'm unsure the valid range of 1 to 256 makes sense provided the icon smaller >> than 16×16 is never returned (on Windows at least). > > Well, user still can request 1x1 icon - we will return the multiresolution > image with minimal (1x1) icon inside that will be scaled every time the > actual painting occurs. The size will define the layout of the component with > the icon and can be auto-generated from the user code so i do not see why we > should limit the lowest requested size - especially in the shared instance > that not only specific for Windows platform. Even though 1×1 icon doesn't make much sense, you're right imposing a limitation is no good either. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]
On Fri, 30 Apr 2021 20:54:01 GMT, Alexey Ivanov wrote: >> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line >> 1146: >> >>> 1144: } >>> 1145: Map multiResolutionIcon = new HashMap<>(); >>> 1146: int start = size > MAX_QUALITY_ICON ? >>> ICON_RESOLUTIONS.length - 1 : 0; >> >> Does it make sense to always start at zero? >> The icons of smaller size will never be used, will they? >> Thus it's safe to start at the index which corresponds to the requested size >> if `size` matches, or the index such as `ICON_RESOLUTIONS[index] < size && >> ICON_RESOLUTIONS[index + 1] > size`. > > 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. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]
On Fri, 30 Apr 2021 20:34:01 GMT, Alexey Ivanov wrote:
>> Alexander Zuev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 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>
>
> src/java.desktop/share/classes/sun/awt/shell/ShellFolder.java line 213:
>
>> 211: * Returns the icon of the specified size used to display this
>> shell folder.
>> 212: *
>> 213: * @param size size of the icon > 0 (Valid range: 1 to 256)
>
> I'm unsure the valid range of 1 to 256 makes sense provided the icon smaller
> than 16×16 is never returned (on Windows at least).
Well, user still can request 1x1 icon - we will return the multiresolution
image with minimal (1x1) icon inside that will be scaled every time the actual
painting occurs. The size will define the layout of the component with the icon
and can be auto-generated from the user code so i do not see why we should
limit the lowest requested size - especially in the shared instance that not
only specific for Windows platform.
> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line
> 1193:
>
>> 1191: static Image getShell32Icon(int iconID, int size) {
>> 1192: Toolkit toolkit = Toolkit.getDefaultToolkit();
>> 1193: String shellIconBPP =
>> (String)toolkit.getDesktopProperty("win.icon.shellIconBPP");
>
> Looks like these lines aren't used any more and should be removed too.
Yes. Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]
On Fri, 30 Apr 2021 20:23:21 GMT, Alexey Ivanov wrote: >> Alexander Zuev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 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> > > 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: * magnification factors. > > I'm not sure what are the standard terms to refer to High DPI environments > and the scale factors associated with High DPI. In Windows environment it is scaling size or scaling factor. On Mac it is called scaled resolution. So i would say scaling factor is an appropriate term. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]
On Wed, 10 Mar 2021 20:53:43 GMT, Alexey Ivanov wrote: >> Alexander Zuev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 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> > > src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line > 1146: > >> 1144: } >> 1145: Map multiResolutionIcon = new HashMap<>(); >> 1146: int start = size > MAX_QUALITY_ICON ? >> ICON_RESOLUTIONS.length - 1 : 0; > > Does it make sense to always start at zero? > The icons of smaller size will never be used, will they? > Thus it's safe to start at the index which corresponds to the requested size > if `size` matches, or the index such as `ICON_RESOLUTIONS[index] < size && > ICON_RESOLUTIONS[index + 1] > size`. This comment is also about the case of not fetching icons of sizes smaller than requested size. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]
On Wed, 10 Mar 2021 20:40:40 GMT, Alexey Ivanov wrote:
>> Alexander Zuev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 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>
>
> test/jdk/javax/swing/JFileChooser/FileSystemView/SystemIconTest.java line 64:
>
>> 62: }
>> 63:
>> 64: if (icon.getIconWidth() != size) {
>
> Does it make sense to check that the icon is a multi-resolution image with
> the expected sizes?
> We know for sure `explorer.exe` contains the entire set of icons.
Since the test always expects a multi-resolution image, does it make sense to
assert `icon instanceof MultiResolutionImage` at the very least?
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]
On Thu, 29 Apr 2021 17:04:17 GMT, Alexander Zuev wrote: >> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line >> 1114: >> >>> 1112: bothIcons.put(getLargeIcon? >>> SMALL_ICON_SIZE : LARGE_ICON_SIZE, newIcon2); >>> 1113: newIcon = new >>> MultiResolutionIconImage(getLargeIcon ? LARGE_ICON_SIZE >>> 1114: : SMALL_ICON_SIZE, >>> bothIcons); >> >> I still propose to refactor this code to make it clearer. The code always >> returns two icons: _small + large_ or _large + small_ — combined in >> `MultiResolutionIconImage`. >> >> You can get `smallIcon` and `largeIcon` and then wrap them into >> `MultiResolutionIconImage` in the correct order and store each icon in the >> cache. >> >> My opinion hasn't changed here. >> >> I still think there's not much value in getting smaller icon size in >> addition to larger one. Or does it provide an alternative image for the case >> where the system scaling is 200% and the window is moved to a monitor with >> scaling of 100%? > > Getting smaller icon is relevant in the case of the scaling. I do not think > refactoring image caches from icons to multiresolution images will make code > much cleaner - at the end we will have to extract images from the > multiresolution image to repack them into different multiresolution image > because the base size of the image will not be the same and it does matter in > how they will be scaled on the different screens. And we still need to > extract both images because sometimes small resolution image looks not like > the large resolution one and for a reason - so small resolution image is not > blurred by the small detail on the large icon when scaling on the low > resolution screen. No, I can't see how it's relevant. If the scale factor is 100% and the requested icon size is 16, then 16×16 is used; if the window is moved to a monitor with scale factor 200%, then 32×32 icon is used for rendering which is already fetched and available in the multi-resolution image — perfect, there's the benefit. But when is it possible that the scale factor is less than 100%? If `JFileChooser` requests a large icon that is 32×32, then it will be used for rendering when the scale factor is 100%. When the scale factor is 200%, the icon of 64×64 is required but it's not available, instead there's 16×16 icon. For this icon to be used, the scale factor needs to be 50%. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]
On Fri, 30 Apr 2021 12:27:19 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:
>
> 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>
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: * magnification factors.
I'm not sure what are the standard terms to refer to High DPI environments and
the scale factors associated with High DPI.
src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java line
275:
> 273: *
> 274: *
> 275: * @param f a File object
Suggestion:
* @param f a {@code File} object
src/java.desktop/share/classes/sun/awt/shell/ShellFolder.java line 213:
> 211: * Returns the icon of the specified size used to display this shell
> folder.
> 212: *
> 213: * @param size size of the icon > 0 (Valid range: 1 to 256)
I'm unsure the valid range of 1 to 256 makes sense provided the icon smaller
than 16×16 is never returned (on Windows at least).
src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line 1193:
> 1191: static Image getShell32Icon(int iconID, int size) {
> 1192: Toolkit toolkit = Toolkit.getDefaultToolkit();
> 1193: String shellIconBPP =
> (String)toolkit.getDesktopProperty("win.icon.shellIconBPP");
Looks like these lines aren't used any more and should be removed too.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]
On Thu, 29 Apr 2021 18:47:27 GMT, Alexander Zuev wrote: >> src/java.desktop/windows/classes/sun/awt/shell/Win32ShellFolder2.java line >> 1044: >> >>> 1042: new BufferedImage(iconSize, iconSize, >>> BufferedImage.TYPE_INT_ARGB); >>> 1043: img.setRGB(0, 0, iconSize, iconSize, iconBits, 0, >>> iconSize); >>> 1044: return img; >> >> There are cases where the size of the buffered image is different from the >> requested size. It could affect the layout because it breaks the assumption >> that the returned image has the requested size but it may be larger. (Or is >> it no longer possible?) I think it should be wrapped into >> `MultiResolutionIconImage` in this case. > > Actually in makeImage we do not use requested size, we return the bits that > system returns to us. How the generated image is treated depends on the > implementation of the public methods - where it matters they are wrapped in > the corresponding multi resolution images so it behaves correctly in UI. Okay, if it *always* wrapped in a multi-resolution image where it *matters*. - PR: https://git.openjdk.java.net/jdk/pull/2875
Re: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]
> Fix updated after first round of review. Alexander Zuev has updated the pull request incrementally with one additional commit since the last revision: 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> - Changes: - all: https://git.openjdk.java.net/jdk/pull/2875/files - new: https://git.openjdk.java.net/jdk/pull/2875/files/6607b61f..4a360621 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2875=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2875=01-02 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: [OpenJDK 2D-Dev] RFR: 8182043: Access to Windows Large Icons [v3]
On Wed, 10 Mar 2021 20:44:19 GMT, Alexey Ivanov wrote:
>> Alexander Zuev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 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>
>
> src/java.desktop/share/classes/javax/swing/filechooser/FileSystemView.java
> line 264:
>
>> 262: *
>> 263: * The default implementation gets information from the
>> 264: * ShellFolder class. Whenever possible, the icon
>
> Do we continue using `` elements? I thought that {@code} is the
> preferred way to markup class names.
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/2875
