Re: [10] Review request for 8182043: Access to Windows Large Icons
This thread is very long and despite trying I am sure I have not absorbed every discussion point to date. I think a concise summary of the current opinions from Semyon would help as I don't yet see a concensus emerging and if we are adding a new public API we need to be sure that 1) It is supportable ? - The question around whether Windows sometimes gives us a scaled image even when we think it is giving us a full resolution image is one that needs to be considered 2) returning something backed by a multi-res image seems like it should be explored for feasibility. I am not sure why a developer would need multiple images to be manually managed and selected in this case when elsewhere we can select an appropriate one automatically. PERHAPS we can even retro-fit the implementation of the existing API to support this ? 3) It does what the developer actually needs ? Although he is just one person, has anyone gone to ask the person who submitted this request what he would LIKE to see .. as distinct from what he has been doing as a workaround to date. By this I mean maybe he has had to manually select a large icon and use it where he needs it .. perhaps he is doing this only because we aren't doing it automatically ? So I am not even really looking at the code here .. although I have .. we need to get the high level issues sorted out first. So Semyon: AI: provide that summary .. and along with it answer as much of 1 - 3 as you can and also go find the submitter and ask him for input. -phil. On 10/24/2017 09:10 AM, Alexey Ivanov wrote: Hi Sergey, On 24/10/2017 00:22, Sergey Bylokhov wrote: On 17/10/2017 09:42, Alexey Ivanov wrote: This is my understanding, before those fix the maximum size was 32x32. It is my understanding too. Thus limiting the size of icon to 128 pixels seemed reasonable. At this moment the buffer for icon pixels is allocated on the stack, therefore the size cannot be dynamic. If memory for icon pixels is allocated dynamically, the limitation can be removed. It makes sense to address this limitation under a separate issue, do you agree? I agree that this can be moved to a separate bug. But my complaints were not related to it. - The new flags are not strictly specified. It is unclear what are small and large icons. For example the old "getSystemIcon" specified that it returns "an icon as it would be displayed by a native file chooser". How the new flags are related to this? I see that the SMALL flag is the same as the old method, and the large cion is just a HiDPI version of default icon. - SMALL/LARGE flags depends from the native main screen scale, probably it is possible to remove this dependency? Otherwise in the two screen configuration it will produce different images depending from the main screen. Per my understanding, SMALL and LARGE flags correspond to the previous private API behaviour where you could get two icon sizes: 16×16 and 32×32. These flags will return the same logical-sized icons, subject to HiDPI scaling performed by native system. So these constants provide backward compatibility to sun.awt.shell.ShellFolder.getShellFolder(file).getIcon(getLargeIcon): either SMALL or LARGE. - In case of multi-monitor config in the discussion above there was a suggestion that the user can create MRI on top of the new API. I am not sure how it will work because we already return MRI for most of the files. I am still thinking that we should investigate the possibility to return MRI directly w/o ImageIcon/Icon wrappers. This MRI could load data lazily depends from the destination scale. The user ill able to get any resolution variants via getResolutionVariant(double,double) and also can get the list of the icons attached to some file via getResolutionVariants(); My suggestion was to create MRI internally inside the new API. [1] This seems like the best option. On the other hand, we need to overcome native system scaling, otherwise the rendering could be broken. Testing performed by you [2] and me [3] shows that native system scales the returned hIcon, and, if there are several displays in the system with different scaling factors, the underlying icon size could be wrong. It seems there's no easy-to-use API for icons, Microsoft admits [4] working with icons is one of the areas where Win32 API does not provide a DPI context. It is still possible to use low-level resource APIs to enumerate all the provided sizes in RT_GROUP_ICON resource. Regards, Alexey [1] http://mail.openjdk.java.net/pipermail/awt-dev/2017-September/013147.html [2] http://mail.openjdk.java.net/pipermail/awt-dev/2017-October/013179.html [3] http://mail.openjdk.java.net/pipermail/awt-dev/2017-October/013181.html [4] https://msdn.microsoft.com/library/windows/desktop/mt843498(v=vs.85).aspx#Common_Pitfalls__Win32__
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Sergey, On 24/10/2017 00:22, Sergey Bylokhov wrote: On 17/10/2017 09:42, Alexey Ivanov wrote: This is my understanding, before those fix the maximum size was 32x32. It is my understanding too. Thus limiting the size of icon to 128 pixels seemed reasonable. At this moment the buffer for icon pixels is allocated on the stack, therefore the size cannot be dynamic. If memory for icon pixels is allocated dynamically, the limitation can be removed. It makes sense to address this limitation under a separate issue, do you agree? I agree that this can be moved to a separate bug. But my complaints were not related to it. - The new flags are not strictly specified. It is unclear what are small and large icons. For example the old "getSystemIcon" specified that it returns "an icon as it would be displayed by a native file chooser". How the new flags are related to this? I see that the SMALL flag is the same as the old method, and the large cion is just a HiDPI version of default icon. - SMALL/LARGE flags depends from the native main screen scale, probably it is possible to remove this dependency? Otherwise in the two screen configuration it will produce different images depending from the main screen. Per my understanding, SMALL and LARGE flags correspond to the previous private API behaviour where you could get two icon sizes: 16×16 and 32×32. These flags will return the same logical-sized icons, subject to HiDPI scaling performed by native system. So these constants provide backward compatibility to sun.awt.shell.ShellFolder.getShellFolder(file).getIcon(getLargeIcon): either SMALL or LARGE. - In case of multi-monitor config in the discussion above there was a suggestion that the user can create MRI on top of the new API. I am not sure how it will work because we already return MRI for most of the files. I am still thinking that we should investigate the possibility to return MRI directly w/o ImageIcon/Icon wrappers. This MRI could load data lazily depends from the destination scale. The user ill able to get any resolution variants via getResolutionVariant(double,double) and also can get the list of the icons attached to some file via getResolutionVariants(); My suggestion was to create MRI internally inside the new API. [1] This seems like the best option. On the other hand, we need to overcome native system scaling, otherwise the rendering could be broken. Testing performed by you [2] and me [3] shows that native system scales the returned hIcon, and, if there are several displays in the system with different scaling factors, the underlying icon size could be wrong. It seems there's no easy-to-use API for icons, Microsoft admits [4] working with icons is one of the areas where Win32 API does not provide a DPI context. It is still possible to use low-level resource APIs to enumerate all the provided sizes in RT_GROUP_ICON resource. Regards, Alexey [1] http://mail.openjdk.java.net/pipermail/awt-dev/2017-September/013147.html [2] http://mail.openjdk.java.net/pipermail/awt-dev/2017-October/013179.html [3] http://mail.openjdk.java.net/pipermail/awt-dev/2017-October/013181.html [4] https://msdn.microsoft.com/library/windows/desktop/mt843498(v=vs.85).aspx#Common_Pitfalls__Win32__
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 17/10/2017 09:42, Alexey Ivanov wrote: This is my understanding, before those fix the maximum size was 32x32. It is my understanding too. Thus limiting the size of icon to 128 pixels seemed reasonable. At this moment the buffer for icon pixels is allocated on the stack, therefore the size cannot be dynamic. If memory for icon pixels is allocated dynamically, the limitation can be removed. It makes sense to address this limitation under a separate issue, do you agree? I agree that this can be moved to a separate bug. But my complaints were not related to it. - The new flags are not strictly specified. It is unclear what are small and large icons. For example the old "getSystemIcon" specified that it returns "an icon as it would be displayed by a native file chooser". How the new flags are related to this? I see that the SMALL flag is the same as the old method, and the large cion is just a HiDPI version of default icon. - SMALL/LARGE flags depends from the native main screen scale, probably it is possible to remove this dependency? Otherwise in the two screen configuration it will produce different images depending from the main screen. - In case of multi-monitor config in the discussion above there was a suggestion that the user can create MRI on top of the new API. I am not sure how it will work because we already return MRI for most of the files. I am still thinking that we should investigate the possibility to return MRI directly w/o ImageIcon/Icon wrappers. This MRI could load data lazily depends from the destination scale. The user ill able to get any resolution variants via getResolutionVariant(double,double) and also can get the list of the icons attached to some file via getResolutionVariants(); Regards, Alexey But these constants are related to the predefined system icon sizes while the Extract() may scale icon to any size. Does it really scale for any size? For example if I request the icon for some pdf,java,txt files of size 100 then: - On HiDPI screen I get the native icon of size 64. - On LowDPI screen I get the native icon of size 32. In both cases the user will get MRI, which will scale the native icon. http://mail.openjdk.java.net/pipermail/awt-dev/2017-October/013179.html Why returning MRI is wrong? The MRI is good, if this is what the user is expected. My point was that the native system is not scale the icon and the user gets MRI which content is unrelated to the passed "size". Which calls are you talking about DPI-aware or DPI-unaware? This is jdk10client + the current patch. -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Semyon, On 06/10/2017 21:33, Semyon Sadetsky wrote: Hi Alexey, On 10/06/2017 11:42 AM, Alexey Ivanov wrote: Hi Semyon, Sergey, On 30/09/2017 00:08, Semyon Sadetsky wrote: On 9/29/2017 3:15 PM, Sergey Bylokhov wrote: On 9/29/17 12:39, Semyon Sadetsky wrote: Why 128 pixels? Windows shell usually provides icons up to 256 pixels, for example there are 256×256 icons for folders and generic file type. It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html Sergey, it is not clear how those links are related to the icon size returned by Windows? It was a fix where the MAX_ICON_SIZE=128 was added. Actually it limits nothing. We told about the Extract call which may return any size. Yes, it does. It limits the size of the returned icon to 128×128. I guess if a larger icon is requested, then we'll get a distorted image. This is artificial limitation when the image is transfered from native to java. WinAPI may return bigger images without any issues. Yes, I understand it. Yet, it's still a limitation. With the current implementation, the caller of the API cannot get icon larger than 128×128. As far as I understand the bug above, it is possible that OS returns some other size. You've probably didn't understand what Alexey meant. The Extract call may return any size you request (it does scaling internally if there are no suitable image) > But the bug above is about queering the fixed size (small or long) which size is determined by OS shell according to the current scale. For those fixed sizes we use SHGetFileInfo not the Extract. And every time we will try to make an icon it will be limited to 128x128. But it is not critical. The issue is that this api, as you said, will depends from some general "current scale". which is unrelated to the transform of the screen in java. If the user will want to use FILE_ICON_LARGE, then to work properly he will need to use this code every time in the the paint(): Icon icon = getSystemIcon(file, FILE_ICON_LARGE); Icon hicon = getSystemIcon(file, icon.getIconWidth()*currentScreenScale); This is just wrong. The first line is the correct one for both HiDPI and nonHiDPI. If you want to have icons like in native apps. For custom behavior - please use the second line. Why is it wrong? getSystemIcon(file) requests FILE_ICON_SMALL from the OS, then all Java has to paint at any DPI scale is 16×16 icon. Or am I missing anything? Sorry, I did not get how is the small icon related to the code above. Probably we understood it differently because the explanation is not the best. My interpretation is: For the low DPI screen one should use icon=getSystemIcon(file, FILE_ICON_LARGE) when the window is moved to hiDPI screen the hicon=getSystemIcon(file, icon.getIconWidth()*currentScreenScale) should be used. And this approach is wrong. The primary purpose of the current fix is to fix the compatibility issue we got when we closed shell folder API in 9. The user code which doesn't work in 9 should not be changed in the way proposed by Sergey. This code should be updated to use getSystemIcon(file, FILE_ICON_LARGE) instead of closed getIcon(true) and getSystemIcon(file, FILE_ICON_SMALL) instead of closed getIcon(false). The newly written code may use getSystemIcon(file, FILE_ICON_LARGE/SMALL) to get the icon in the native platform which determines its size at the current DPI (DPI-unaware usage) or getSystemIcon(file, size) to have any custom size which can be multiplied by DPI. I see no reason to use both approaches simultaneously at any circumstances. --Semyon Thank you Semyon for your explanation. I think I understand it better now. In either case, Swing components should use the size in component coordinates. In case of JFileChooser file view, it's FILE_ICON_SMALL. I didn't expect native OS, Windows in my case, to adjust icon resolution automatically to account for HiDPI scaling because documentation for IExtractIcon::Extract [1] does not mention that such scaling is performed. However, my testing [2] with opening JFileChooser in SwingDemo shows that the scaling is performed, at least in some cases. Overall, the fix looks fine. It resolves the stated problem that is it provides access to larger icons via public API. The issues with the current icon size limitation to 128 and with HiDPI support, if any, can be resolved later under separate bug ids. Does it sound sensible? Regards, Alexey [1] https://msdn.microsoft.com/en-us/library/windows/desktop/bb761850%28v=vs.85%29.aspx [2] http://mail.openjdk.java.net/pipermail/awt-dev/2017-October/013181.html
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Sergey, Semyon, On 12/10/2017 21:39, Sergey Bylokhov wrote: On 06/10/2017 17:16, Semyon Sadetsky wrote: The maximum icon which we used before you fix is 32pixel's icon(yes it is a large icon), and 128 is a size of this icon on 4k monitor( The windows can return a 128pixel's icon on 4k monitor) The EXTRA_LARGE and JAMBO was not used in our code. So, it is not supposed to work on 8k monitor? Why we should have this limit while the native platform hasn't it? This is my understanding, before those fix the maximum size was 32x32. It is my understanding too. Thus limiting the size of icon to 128 pixels seemed reasonable. At this moment the buffer for icon pixels is allocated on the stack, therefore the size cannot be dynamic. If memory for icon pixels is allocated dynamically, the limitation can be removed. It makes sense to address this limitation under a separate issue, do you agree? Regards, Alexey But these constants are related to the predefined system icon sizes while the Extract() may scale icon to any size. Does it really scale for any size? For example if I request the icon for some pdf,java,txt files of size 100 then: - On HiDPI screen I get the native icon of size 64. - On LowDPI screen I get the native icon of size 32. In both cases the user will get MRI, which will scale the native icon. http://mail.openjdk.java.net/pipermail/awt-dev/2017-October/013179.html Why returning MRI is wrong? The MRI is good, if this is what the user is expected. My point was that the native system is not scale the icon and the user gets MRI which content is unrelated to the passed "size". Which calls are you talking about DPI-aware or DPI-unaware? This is jdk10client + the current patch.
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 06/10/2017 17:16, Semyon Sadetsky wrote: The maximum icon which we used before you fix is 32pixel's icon(yes it is a large icon), and 128 is a size of this icon on 4k monitor( The windows can return a 128pixel's icon on 4k monitor) The EXTRA_LARGE and JAMBO was not used in our code. So, it is not supposed to work on 8k monitor? Why we should have this limit while the native platform hasn't it? This is my understanding, before those fix the maximum size was 32x32. But these constants are related to the predefined system icon sizes while the Extract() may scale icon to any size. Does it really scale for any size? For example if I request the icon for some pdf,java,txt files of size 100 then: - On HiDPI screen I get the native icon of size 64. - On LowDPI screen I get the native icon of size 32. In both cases the user will get MRI, which will scale the native icon. http://mail.openjdk.java.net/pipermail/awt-dev/2017-October/013179.html Why returning MRI is wrong? The MRI is good, if this is what the user is expected. My point was that the native system is not scale the icon and the user gets MRI which content is unrelated to the passed "size". Which calls are you talking about DPI-aware or DPI-unaware? This is jdk10client + the current patch. -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 10/06/2017 03:57 PM, Sergey Bylokhov wrote: On 10/6/17 13:50, Semyon Sadetsky wrote: This is not true because in addition to LARGE which you probably mean as 32-128 (ignoring the fact that this sizes may be changed in the Windows registry), Windows supports EXTRA_LARGE and JAMBO since Vista. The maximum icon which we used before you fix is 32pixel's icon(yes it is a large icon), and 128 is a size of this icon on 4k monitor( The windows can return a 128pixel's icon on 4k monitor) The EXTRA_LARGE and JAMBO was not used in our code. So, it is not supposed to work on 8k monitor? Why we should have this limit while the native platform hasn't it? But these constants are related to the predefined system icon sizes while the Extract() may scale icon to any size. Does it really scale for any size? For example if I request the icon for some pdf,java,txt files of size 100 then: - On HiDPI screen I get the native icon of size 64. - On LowDPI screen I get the native icon of size 32. In both cases the user will get MRI, which will scale the native icon. http://mail.openjdk.java.net/pipermail/awt-dev/2017-October/013179.html Why returning MRI is wrong? Which calls are you talking about DPI-aware or DPI-unaware?
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 10/6/17 13:50, Semyon Sadetsky wrote: This is not true because in addition to LARGE which you probably mean as 32-128 (ignoring the fact that this sizes may be changed in the Windows registry), Windows supports EXTRA_LARGE and JAMBO since Vista. The maximum icon which we used before you fix is 32pixel's icon(yes it is a large icon), and 128 is a size of this icon on 4k monitor( The windows can return a 128pixel's icon on 4k monitor) The EXTRA_LARGE and JAMBO was not used in our code. But these constants are related to the predefined system icon sizes while the Extract() may scale icon to any size. Does it really scale for any size? For example if I request the icon for some pdf,java,txt files of size 100 then: - On HiDPI screen I get the native icon of size 64. - On LowDPI screen I get the native icon of size 32. In both cases the user will get MRI, which will scale the native icon. http://mail.openjdk.java.net/pipermail/awt-dev/2017-October/013179.html -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 10/06/2017 01:16 PM, Sergey Bylokhov wrote: On 10/6/17 13:01, Semyon Sadetsky wrote: On 10/06/2017 12:38 PM, Sergey Bylokhov wrote: On 10/6/17 09:53, Alexey Ivanov wrote: It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html I see. And it can be changed, if deemed necessary, can't it? Yes we can. Since you were the person who approved this fix can you explain why the limitation was introduced? We can decide whether this limitation should be reverted in a separate bug. The maximum icon which we use is 32pixel's icon, and 128 is a size of this icon on 4k monitor. This is not true because in addition to LARGE which you probably mean as 32-128 (ignoring the fact that this sizes may be changed in the Windows registry), Windows supports EXTRA_LARGE and JAMBO since Vista. But these constants are related to the predefined system icon sizes while the Extract() may scale icon to any size. So, I still do not see what was the reason to introduce this limitation. As far as I understand the bug above, it is possible that OS returns some other size. No, it is not. In that bug icons are extracted from Image List which is created a part of Toolbar: 1036 HWND hWndToolbar = ::CreateWindowEx(0, TOOLBARCLASSNAME, NULL, Then an icon is extracted from that image list. Obviously, Toolbar can create and creates, as the bug report shows, its icon set of different size depending on the current DPI setting, or rather the DPI settings of the main display. As for JOptionPane, the icons are loaded using ::LoadIcon which loads icon of the default size only. Depending on the current DPI setting, it may return icon of larger size. Yet in this fix, the file icon is requested with explicit size. You will get the size you requested. Probably we have some other bug in the fix, but unfortunately I cannot confirm behavior you describe. For example if I request the icon for some pdf,java.txt files of size 100m then: - On HiDPI screen I get the native icon of size 64. - On LowDPI screen I get the native icon of size 32. In both cases the user will get MRI, which will scale the native icon.
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Alexey, On 10/06/2017 11:42 AM, Alexey Ivanov wrote: Hi Semyon, Sergey, On 30/09/2017 00:08, Semyon Sadetsky wrote: On 9/29/2017 3:15 PM, Sergey Bylokhov wrote: On 9/29/17 12:39, Semyon Sadetsky wrote: Why 128 pixels? Windows shell usually provides icons up to 256 pixels, for example there are 256×256 icons for folders and generic file type. It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html Sergey, it is not clear how those links are related to the icon size returned by Windows? It was a fix where the MAX_ICON_SIZE=128 was added. Actually it limits nothing. We told about the Extract call which may return any size. Yes, it does. It limits the size of the returned icon to 128×128. I guess if a larger icon is requested, then we'll get a distorted image. This is artificial limitation when the image is transfered from native to java. WinAPI may return bigger images without any issues. As far as I understand the bug above, it is possible that OS returns some other size. You've probably didn't understand what Alexey meant. The Extract call may return any size you request (it does scaling internally if there are no suitable image) > But the bug above is about queering the fixed size (small or long) which size is determined by OS shell according to the current scale. For those fixed sizes we use SHGetFileInfo not the Extract. And every time we will try to make an icon it will be limited to 128x128. But it is not critical. The issue is that this api, as you said, will depends from some general "current scale". which is unrelated to the transform of the screen in java. If the user will want to use FILE_ICON_LARGE, then to work properly he will need to use this code every time in the the paint(): Icon icon = getSystemIcon(file, FILE_ICON_LARGE); Icon hicon = getSystemIcon(file, icon.getIconWidth()*currentScreenScale); This is just wrong. The first line is the correct one for both HiDPI and nonHiDPI. If you want to have icons like in native apps. For custom behavior - please use the second line. Why is it wrong? getSystemIcon(file) requests FILE_ICON_SMALL from the OS, then all Java has to paint at any DPI scale is 16×16 icon. Or am I missing anything? Sorry, I did not get how is the small icon related to the code above. Probably we understood it differently because the explanation is not the best. My interpretation is: For the low DPI screen one should use icon=getSystemIcon(file, FILE_ICON_LARGE) when the window is moved to hiDPI screen the hicon=getSystemIcon(file, icon.getIconWidth()*currentScreenScale) should be used. And this approach is wrong. The primary purpose of the current fix is to fix the compatibility issue we got when we closed shell folder API in 9. The user code which doesn't work in 9 should not be changed in the way proposed by Sergey. This code should be updated to use getSystemIcon(file, FILE_ICON_LARGE) instead of closed getIcon(true) and getSystemIcon(file, FILE_ICON_SMALL) instead of closed getIcon(false). The newly written code may use getSystemIcon(file, FILE_ICON_LARGE/SMALL) to get the icon in the native platform which determines its size at the current DPI (DPI-unaware usage) or getSystemIcon(file, size) to have any custom size which can be multiplied by DPI. I see no reason to use both approaches simultaneously at any circumstances. --Semyon Regards, Alexey
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 10/6/17 13:01, Semyon Sadetsky wrote: On 10/06/2017 12:38 PM, Sergey Bylokhov wrote: On 10/6/17 09:53, Alexey Ivanov wrote: It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html I see. And it can be changed, if deemed necessary, can't it? Yes we can. Since you were the person who approved this fix can you explain why the limitation was introduced? We can decide whether this limitation should be reverted in a separate bug. The maximum icon which we use is 32pixel's icon, and 128 is a size of this icon on 4k monitor. As far as I understand the bug above, it is possible that OS returns some other size. No, it is not. In that bug icons are extracted from Image List which is created a part of Toolbar: 1036 HWND hWndToolbar = ::CreateWindowEx(0, TOOLBARCLASSNAME, NULL, Then an icon is extracted from that image list. Obviously, Toolbar can create and creates, as the bug report shows, its icon set of different size depending on the current DPI setting, or rather the DPI settings of the main display. As for JOptionPane, the icons are loaded using ::LoadIcon which loads icon of the default size only. Depending on the current DPI setting, it may return icon of larger size. Yet in this fix, the file icon is requested with explicit size. You will get the size you requested. Probably we have some other bug in the fix, but unfortunately I cannot confirm behavior you describe. For example if I request the icon for some pdf,java.txt files of size 100m then: - On HiDPI screen I get the native icon of size 64. - On LowDPI screen I get the native icon of size 32. In both cases the user will get MRI, which will scale the native icon. -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 10/06/2017 12:38 PM, Sergey Bylokhov wrote: On 10/6/17 09:53, Alexey Ivanov wrote: It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html I see. And it can be changed, if deemed necessary, can't it? Yes we can. Since you were the person who approved this fix can you explain why the limitation was introduced? We can decide whether this limitation should be reverted in a separate bug. As far as I understand the bug above, it is possible that OS returns some other size. No, it is not. In that bug icons are extracted from Image List which is created a part of Toolbar: 1036 HWND hWndToolbar = ::CreateWindowEx(0, TOOLBARCLASSNAME, NULL, Then an icon is extracted from that image list. Obviously, Toolbar can create and creates, as the bug report shows, its icon set of different size depending on the current DPI setting, or rather the DPI settings of the main display. As for JOptionPane, the icons are loaded using ::LoadIcon which loads icon of the default size only. Depending on the current DPI setting, it may return icon of larger size. Yet in this fix, the file icon is requested with explicit size. You will get the size you requested. Probably we have some other bug in the fix, but unfortunately I cannot confirm behavior you describe. For example if I request the icon for some pdf,java.txt files of size 100m then: - On HiDPI screen I get the native icon of size 64. - On LowDPI screen I get the native icon of size 32. In both cases the user will get MRI, which will scale the native icon.
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Sergey, On 06/10/2017 20:38, Sergey Bylokhov wrote: On 10/6/17 09:53, Alexey Ivanov wrote: It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html I see. And it can be changed, if deemed necessary, can't it? Yes we can. As far as I understand the bug above, it is possible that OS returns some other size. No, it is not. In that bug icons are extracted from Image List which is created a part of Toolbar: 1036 HWND hWndToolbar = ::CreateWindowEx(0, TOOLBARCLASSNAME, NULL, Then an icon is extracted from that image list. Obviously, Toolbar can create and creates, as the bug report shows, its icon set of different size depending on the current DPI setting, or rather the DPI settings of the main display. As for JOptionPane, the icons are loaded using ::LoadIcon which loads icon of the default size only. Depending on the current DPI setting, it may return icon of larger size. Yet in this fix, the file icon is requested with explicit size. You will get the size you requested. Probably we have some other bug in the fix, but unfortunately I cannot confirm behavior you describe. For example if I request the icon for some pdf,java.txt files of size 100m then: - On HiDPI screen I get the native icon of size 64. - On LowDPI screen I get the native icon of size 32. In both cases the user will get MRI, which will scale the native icon. I tested it myself. For some reason, in some cases the icon looks crisp, in other cases it is scaled: At 200% the icons look this way: IconJava / JFileChooser Paint / Open dialog This PC DPI shortcut In the second row, the icon is obviously correct because the small icon is flat. Regards, Alexey
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 10/6/17 09:53, Alexey Ivanov wrote: It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html I see. And it can be changed, if deemed necessary, can't it? Yes we can. As far as I understand the bug above, it is possible that OS returns some other size. No, it is not. In that bug icons are extracted from Image List which is created a part of Toolbar: 1036 HWND hWndToolbar = ::CreateWindowEx(0, TOOLBARCLASSNAME, NULL, Then an icon is extracted from that image list. Obviously, Toolbar can create and creates, as the bug report shows, its icon set of different size depending on the current DPI setting, or rather the DPI settings of the main display. As for JOptionPane, the icons are loaded using ::LoadIcon which loads icon of the default size only. Depending on the current DPI setting, it may return icon of larger size. Yet in this fix, the file icon is requested with explicit size. You will get the size you requested. Probably we have some other bug in the fix, but unfortunately I cannot confirm behavior you describe. For example if I request the icon for some pdf,java.txt files of size 100m then: - On HiDPI screen I get the native icon of size 64. - On LowDPI screen I get the native icon of size 32. In both cases the user will get MRI, which will scale the native icon. -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Sergey, On 29/09/2017 22:06, Sergey Bylokhov wrote: On 9/29/17 07:11, Alexey Ivanov wrote: If I understand correctly, the introduction of the new API does not change the behaviour in this case, does it? The icon extracted from Windows was 16×16 and will continue to be used. That is the icon will be scaled when painted. To support HiDPI displays the implementation of |getFileChooser().getFileSystemView().getSystemIcon(f)| should be enhanced to fetch the icon at the appropriate size according to the current rendering scale. I mean in standard resolution, get 16×16 icon, for 125% scale factor, get 20×20 icon, for 150% scale factor, get icon 24×24. (As far as I know, |IExtractIcon::Extract| does not perform any scaling to account for HiDPI rendering. And it really can't because different displays can have different DPI settings. Thus if you request icon size of 16×16, you'll get 16×16 icon, not 32×32 even if this size is more appropriate to the current HiDPI scaling.) Yes, the old getSystemIcon(f) can be enhanced to support MRI and this will fix all its usages. But my point was for a new API and how this new API can solve the problem of access to different dpi icons. - We cannot use FILE_ICON_SMALL because it does not depend from the screen, and it is unclear what the small icons means. - We cannot use FILE_ICON_LARGE by the same reason. - We cannot request some specific size because we know the scale which should be applied to the default icon, but we do not know the size of the default icon. So everywhere we should do something like this: Icon icon = getSystemIcon(file); Icon hicon = getSystemIcon(file, icon.getIconWidth()*screenScale); I think getSystemIcon(file) could be re-implemented as a call to getSystemIcon(file, SMALL_ICON_SIZE * screenScale). The only problem is that screenScale is unknown, it cannot be determined in advance, right? Different icon sizes could be combined into MRI lazily. To support it, we could use different APIs to extract the icons. For example, we can get the list of icon sizes for a file type, and extract only “native” size. If larger size is required for HiDPI and the icon has it, then get the larger version and use it for rendering rather than scaling the one we already have. It is not necessary to update other parts of the Swing right now, but it should be possible to update it later and use the new API which will be added in this fix, since this fix is a about access to a good quality icons, some comments here: https://bugs.openjdk.java.net/browse/JDK-8156183 No, I didn't mean to update other parts. Since Swing operates logical coordinates, getting larger icon size from the underlying OS should be hidden from user. It could be done via MRI. But to request the right physical pixel size of the icon, it's necessary to know the current scaling factor in Graphics. Alternative is to get the entire set from the OS: for example if icon of SMALL_ICON_SIZE, i.e. 16×16, is requested, then the getSystemIcon(file) returns MRI which contains the following sizes: 16, 20, 24, 32. However, it means we'll waste system resources by requesting extra sizes that we'll throw away as soon as paint completes. To optimize this solution, the API can request only the sizes that correspond to screen DPI. For example, if there's only one display in the system, the only one (scaled) size is requested; if there are two displays, the returned MRI contains physical-sized icons for either display if their DPI settings are different. Regards, Alexey However, it looks to be out of the scope for this particular fix. Yet this approach will make UI look crispier at higher resolutions.
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Semyon, Sergey, On 30/09/2017 00:08, Semyon Sadetsky wrote: On 9/29/2017 3:15 PM, Sergey Bylokhov wrote: On 9/29/17 12:39, Semyon Sadetsky wrote: Why 128 pixels? Windows shell usually provides icons up to 256 pixels, for example there are 256×256 icons for folders and generic file type. It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html Sergey, it is not clear how those links are related to the icon size returned by Windows? It was a fix where the MAX_ICON_SIZE=128 was added. Actually it limits nothing. We told about the Extract call which may return any size. Yes, it does. It limits the size of the returned icon to 128×128. I guess if a larger icon is requested, then we'll get a distorted image. As far as I understand the bug above, it is possible that OS returns some other size. You've probably didn't understand what Alexey meant. The Extract call may return any size you request (it does scaling internally if there are no suitable image) > But the bug above is about queering the fixed size (small or long) which size is determined by OS shell according to the current scale. For those fixed sizes we use SHGetFileInfo not the Extract. And every time we will try to make an icon it will be limited to 128x128. But it is not critical. The issue is that this api, as you said, will depends from some general "current scale". which is unrelated to the transform of the screen in java. If the user will want to use FILE_ICON_LARGE, then to work properly he will need to use this code every time in the the paint(): Icon icon = getSystemIcon(file, FILE_ICON_LARGE); Icon hicon = getSystemIcon(file, icon.getIconWidth()*currentScreenScale); This is just wrong. The first line is the correct one for both HiDPI and nonHiDPI. If you want to have icons like in native apps. For custom behavior - please use the second line. Why is it wrong? getSystemIcon(file) requests FILE_ICON_SMALL from the OS, then all Java has to paint at any DPI scale is 16×16 icon. Or am I missing anything? Regards, Alexey
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Semyon, On 29/09/2017 20:39, Semyon Sadetsky wrote: On 9/29/2017 12:29 PM, Sergey Bylokhov wrote: On 9/29/17 07:34, Alexey Ivanov wrote: Ok, so it means that we will support 1-128 pixels natively(MAX_ICON_SIZE) and others via MRI. Why 128 pixels? Windows shell usually provides icons up to 256 pixels, for example there are 256×256 icons for folders and generic file type. It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html Sergey, it is not clear how those links are related to the icon size returned by Windows? Since |IExtractIcon::Extract| gives you the requested size, performing scaling if required, then MRI will never be crea As far as I understand the bug above, it is possible that OS returns some other size. You've probably didn't understand what Alexey meant. The Extract call may return any size you request (it does scaling internally if there are no suitable image). Yes, that's what I meant. IExtractIcon::Extract always returns the size you requested, if necessary it does internal scaling. But the bug above is about queering the fixed size (small or long) which size is determined by OS shell according to the current scale. For those fixed sizes we use SHGetFileInfo not the Extract. However, I'm afraid IExtractIcon::Extract does not scale the icon to the current DPI scaling, it's the task of the programmer to request the icon size that's required in current scaling settings to make sure the icon is crisp. Regards, Alexey --Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Sergey, Sorry for a long delay… On 29/09/2017 20:29, Sergey Bylokhov wrote: On 9/29/17 07:34, Alexey Ivanov wrote: Ok, so it means that we will support 1-128 pixels natively(MAX_ICON_SIZE) and others via MRI. Why 128 pixels? Windows shell usually provides icons up to 256 pixels, for example there are 256×256 icons for folders and generic file type. It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html I see. And it can be changed, if deemed necessary, can't it? Since |IExtractIcon::Extract| gives you the requested size, performing scaling if required, then MRI will never be crea As far as I understand the bug above, it is possible that OS returns some other size. No, it is not. In that bug icons are extracted from Image List which is created a part of Toolbar: 1036 HWND hWndToolbar = ::CreateWindowEx(0, TOOLBARCLASSNAME, NULL, Then an icon is extracted from that image list. Obviously, Toolbar can create and creates, as the bug report shows, its icon set of different size depending on the current DPI setting, or rather the DPI settings of the main display. As for JOptionPane, the icons are loaded using ::LoadIcon which loads icon of the default size only. Depending on the current DPI setting, it may return icon of larger size. Yet in this fix, the file icon is requested with explicit size. You will get the size you requested. Regards, Alexey
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 9/29/17 16:08, Semyon Sadetsky wrote: On 9/29/2017 3:15 PM, Sergey Bylokhov wrote: On 9/29/17 12:39, Semyon Sadetsky wrote: Why 128 pixels? Windows shell usually provides icons up to 256 pixels, for example there are 256×256 icons for folders and generic file type. It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html Sergey, it is not clear how those links are related to the icon size returned by Windows? It was a fix where the MAX_ICON_SIZE=128 was added. Actually it limits nothing. We told about the Extract call which may return any size. I do not understand how it could be "that it limits nothing". The method "makeIcon(long hIcon, int bsize)" is the only place where we create BufferedImage which contains the data of icon, and this content cannot be bigger than 128x128 pixels because of MAX_ICON_SIZE=128 And every time we will try to make an icon it will be limited to 128x128. But it is not critical. The issue is that this api, as you said, will depends from some general "current scale". which is unrelated to the transform of the screen in java. If the user will want to use FILE_ICON_LARGE, then to work properly he will need to use this code every time in the the paint(): Icon icon = getSystemIcon(file, FILE_ICON_LARGE); Icon hicon = getSystemIcon(file, icon.getIconWidth()*currentScreenScale); This is just wrong. The first line is the correct one for both HiDPI and nonHiDPI. If you want to have icons like in native apps. For custom behavior - please use the second line. Why it is wrong? If the native application will request the large icon, get the icon of size=32 and draw it to the screen, then the user will see the icon inside 32x32 pixels on the screen. If the same steps will be done in java the the user will see the icon inside 64x64 pixels, because we will apply the java scale on top of the native scale. In case of different dpi for a different screens we will get the similar bug, because the icon which include the native scale is not applicable for the low-dpi screen. -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 9/29/2017 3:15 PM, Sergey Bylokhov wrote: On 9/29/17 12:39, Semyon Sadetsky wrote: Why 128 pixels? Windows shell usually provides icons up to 256 pixels, for example there are 256×256 icons for folders and generic file type. It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html Sergey, it is not clear how those links are related to the icon size returned by Windows? It was a fix where the MAX_ICON_SIZE=128 was added. Actually it limits nothing. We told about the Extract call which may return any size. As far as I understand the bug above, it is possible that OS returns some other size. You've probably didn't understand what Alexey meant. The Extract call may return any size you request (it does scaling internally if there are no suitable image) > But the bug above is about queering the fixed size (small or long) which size is determined by OS shell according to the current scale. For those fixed sizes we use SHGetFileInfo not the Extract. And every time we will try to make an icon it will be limited to 128x128. But it is not critical. The issue is that this api, as you said, will depends from some general "current scale". which is unrelated to the transform of the screen in java. If the user will want to use FILE_ICON_LARGE, then to work properly he will need to use this code every time in the the paint(): Icon icon = getSystemIcon(file, FILE_ICON_LARGE); Icon hicon = getSystemIcon(file, icon.getIconWidth()*currentScreenScale); This is just wrong. The first line is the correct one for both HiDPI and nonHiDPI. If you want to have icons like in native apps. For custom behavior - please use the second line.
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 9/29/17 12:39, Semyon Sadetsky wrote: Why 128 pixels? Windows shell usually provides icons up to 256 pixels, for example there are 256×256 icons for folders and generic file type. It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html Sergey, it is not clear how those links are related to the icon size returned by Windows? It was a fix where the MAX_ICON_SIZE=128 was added. As far as I understand the bug above, it is possible that OS returns some other size. You've probably didn't understand what Alexey meant. The Extract call may return any size you request (it does scaling internally if there are no suitable image) > But the bug above is about queering the fixed size (small or long) which size is determined by OS shell according to the current scale. For those fixed sizes we use SHGetFileInfo not the Extract. And every time we will try to make an icon it will be limited to 128x128. But it is not critical. The issue is that this api, as you said, will depends from some general "current scale". which is unrelated to the transform of the screen in java. If the user will want to use FILE_ICON_LARGE, then to work properly he will need to use this code every time in the the paint(): Icon icon = getSystemIcon(file, FILE_ICON_LARGE); Icon hicon = getSystemIcon(file, icon.getIconWidth()*currentScreenScale); -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 9/29/17 07:11, Alexey Ivanov wrote: If I understand correctly, the introduction of the new API does not change the behaviour in this case, does it? The icon extracted from Windows was 16×16 and will continue to be used. That is the icon will be scaled when painted. To support HiDPI displays the implementation of |getFileChooser().getFileSystemView().getSystemIcon(f)| should be enhanced to fetch the icon at the appropriate size according to the current rendering scale. I mean in standard resolution, get 16×16 icon, for 125% scale factor, get 20×20 icon, for 150% scale factor, get icon 24×24. (As far as I know, |IExtractIcon::Extract| does not perform any scaling to account for HiDPI rendering. And it really can't because different displays can have different DPI settings. Thus if you request icon size of 16×16, you'll get 16×16 icon, not 32×32 even if this size is more appropriate to the current HiDPI scaling.) Yes, the old getSystemIcon(f) can be enhanced to support MRI and this will fix all its usages. But my point was for a new API and how this new API can solve the problem of access to different dpi icons. - We cannot use FILE_ICON_SMALL because it does not depend from the screen, and it is unclear what the small icons means. - We cannot use FILE_ICON_LARGE by the same reason. - We cannot request some specific size because we know the scale which should be applied to the default icon, but we do not know the size of the default icon. So everywhere we should do something like this: Icon icon = getSystemIcon(file); Icon hicon = getSystemIcon(file, icon.getIconWidth()*screenScale); Different icon sizes could be combined into MRI lazily. To support it, we could use different APIs to extract the icons. For example, we can get the list of icon sizes for a file type, and extract only “native” size. If larger size is required for HiDPI and the icon has it, then get the larger version and use it for rendering rather than scaling the one we already have. It is not necessary to update other parts of the Swing right now, but it should be possible to update it later and use the new API which will be added in this fix, since this fix is a about access to a good quality icons, some comments here: https://bugs.openjdk.java.net/browse/JDK-8156183 However, it looks to be out of the scope for this particular fix. Yet this approach will make UI look crispier at higher resolutions. -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 9/29/17 07:34, Alexey Ivanov wrote: Ok, so it means that we will support 1-128 pixels natively(MAX_ICON_SIZE) and others via MRI. Why 128 pixels? Windows shell usually provides icons up to 256 pixels, for example there are 256×256 icons for folders and generic file type. It is limitation of our implementation: https://bugs.openjdk.java.net/browse/JDK-8151385 http://mail.openjdk.java.net/pipermail/awt-dev/2016-March/010777.html Since |IExtractIcon::Extract| gives you the requested size, performing scaling if required, then MRI will never be crea As far as I understand the bug above, it is possible that OS returns some other size. -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Sergey, On 25/09/2017 21:44, Sergey Bylokhov wrote: On 9/22/17 04:22, Alexey Ivanov wrote: There's no way of knowing in advance. Explorer does not restrict the size of icons (now), it's up to developers of a particular file handler to provide icons. Usually, there's only one icon with size larger than 255. If there's the icon of the requested size, Explorer will give it to you, otherwise it will scale the closest available to the requested size. Windows documentation suggests the following sizes: https://msdn.microsoft.com/en-us/library/windows/desktop/dn742485(v=vs.85).aspx#size_requirements Ok, so it means that we will support 1-128 pixels natively(MAX_ICON_SIZE) and others via MRI. Why 128 pixels? Windows shell usually provides icons up to 256 pixels, for example there are 256×256 icons for folders and generic file type. Since |IExtractIcon::Extract| gives you the requested size, performing scaling if required, then MRI will never be created. As for sizes, 16 seems to be the minimum icon size (Windows shell overlay icons are 10×10). The reasonable maximum size seems to be 256. Regards, Alexey
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Sergey and Semyon, On 28/09/2017 19:49, Sergey Bylokhov wrote: On 9/28/17 10:57, Semyon Sadetsky wrote: Small and large don't have any special meanings for HiDPI. They are some conditional sizes established by the native platform for the current screen resolution. The question what is the current screens resolution when you have two screens? We should relay on the native platform always. So, the icon size returned by the native API is the correct approach. And possibility to use any other sizes is provided as well. On windows and Linux we cannot rely on the native system because all HiDPI support is implemented on our(jdk) side, the native system does not know how this icons are used. It is half of the correct size because on HiDPI it is better to use hidpi icons instead of lowdpi. Will the HiDPI unaware apps draw x2 icons correctly or not depends from our implementation. If we will return the MRI then it will be drawn in correct size even if the bigger(HiDPI) image will be used. This is not true. MRI has a basic size which uniquely determines its painted size in component coordinates. The size painted in component will be the same for all scales this is how HiDPI works in java. The size in a component coordinates will be the same, but if HiDPI image is used the real number of pixels to be drawn will be 4 times bigger, because low-dpi images will be scaled x2 and HiDPI images will be drawn as is. For example one of the consumer of this new API is WindowsFileView. How the code below should be changed to work on a different screens, and request the proper icon? WindowsFileChooserUI.java 1316 icon = getFileChooser().getFileSystemView().getSystemIcon(f); Why it should be changed? The code is requesting the proper icon. Because this method returns an icon of 16x16 pixels, which will be rescaled to 32x32 pixels in paint operation. The size should be equal 16x16 otherwise the component view will be distorted. But painted resolution is determined by native platform. The native platform may return icon of any size. If the size of the icon differs from 16x16 (32x32 for example) then it will be wrapped by MRI of 16x16 size. The draw resolution cannot be calculated by the native platform for each window in java. The windows provide a set of icons for each resolution, and we should select correct one depending from the scalefactor of our window. And we should draw bigger icons when the bigger dpi is in use. from the example above the code in WindowsFileChooserUI will use low-dpi icons on a HiDPI screen: 1316 icon = getFileChooser().getFileSystemView().getSystemIcon(f); How we should rewrite this code using a new API to support the icons which are large than default? If I understand correctly, the introduction of the new API does not change the behaviour in this case, does it? The icon extracted from Windows was 16×16 and will continue to be used. That is the icon will be scaled when painted. To support HiDPI displays the implementation of |getFileChooser().getFileSystemView().getSystemIcon(f)| should be enhanced to fetch the icon at the appropriate size according to the current rendering scale. I mean in standard resolution, get 16×16 icon, for 125% scale factor, get 20×20 icon, for 150% scale factor, get icon 24×24. (As far as I know, |IExtractIcon::Extract| does not perform any scaling to account for HiDPI rendering. And it really can't because different displays can have different DPI settings. Thus if you request icon size of 16×16, you'll get 16×16 icon, not 32×32 even if this size is more appropriate to the current HiDPI scaling.) Different icon sizes could be combined into MRI lazily. To support it, we could use different APIs to extract the icons. For example, we can get the list of icon sizes for a file type, and extract only “native” size. If larger size is required for HiDPI and the icon has it, then get the larger version and use it for rendering rather than scaling the one we already have. However, it looks to be out of the scope for this particular fix. Yet this approach will make UI look crispier at higher resolutions. Regards, Alexey
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 9/28/17 10:57, Semyon Sadetsky wrote: Small and large don't have any special meanings for HiDPI. They are some conditional sizes established by the native platform for the current screen resolution. The question what is the current screens resolution when you have two screens? We should relay on the native platform always. So, the icon size returned by the native API is the correct approach. And possibility to use any other sizes is provided as well. On windows and Linux we cannot rely on the native system because all HiDPI support is implemented on our(jdk) side, the native system does not know how this icons are used. It is half of the correct size because on HiDPI it is better to use hidpi icons instead of lowdpi. Will the HiDPI unaware apps draw x2 icons correctly or not depends from our implementation. If we will return the MRI then it will be drawn in correct size even if the bigger(HiDPI) image will be used. This is not true. MRI has a basic size which uniquely determines its painted size in component coordinates. The size painted in component will be the same for all scales this is how HiDPI works in java. The size in a component coordinates will be the same, but if HiDPI image is used the real number of pixels to be drawn will be 4 times bigger, because low-dpi images will be scaled x2 and HiDPI images will be drawn as is. For example one of the consumer of this new API is WindowsFileView. How the code below should be changed to work on a different screens, and request the proper icon? WindowsFileChooserUI.java 1316 icon = getFileChooser().getFileSystemView().getSystemIcon(f); Why it should be changed? The code is requesting the proper icon. Because this method returns an icon of 16x16 pixels, which will be rescaled to 32x32 pixels in paint operation. The size should be equal 16x16 otherwise the component view will be distorted. But painted resolution is determined by native platform. The native platform may return icon of any size. If the size of the icon differs from 16x16 (32x32 for example) then it will be wrapped by MRI of 16x16 size. The draw resolution cannot be calculated by the native platform for each window in java. The windows provide a set of icons for each resolution, and we should select correct one depending from the scalefactor of our window. And we should draw bigger icons when the bigger dpi is in use. from the example above the code in WindowsFileChooserUI will use low-dpi icons on a HiDPI screen: 1316 icon = getFileChooser().getFileSystemView().getSystemIcon(f); How we should rewrite this code using a new API to support the icons which are large than default? -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
We definitely can't backport an API change. Reading the bug report it seems they have been doing this by using internal APIs. For JDK 9, since illegal-access is allowed by default, they can continue to do that, so perhaps we can just not worry about the backport and solve this only for 10 + They can add reflective code to look for the 10 solution so it can run on all releases and automatically take advantage of the new API when it appears. Of course they should also re-work the existing code to be reflective and defer to the new API. If they do this then it would also be safe when illegal access is denied by default .. since it should be in a later release than the new API. -phil On 9/28/17, 11:02 AM, Semyon Sadetsky wrote: On 9/28/2017 10:51 AM, Philip Race wrote: If this is up for consideration for backporting - as appears to be the case - then I think you should post an updated webrev. Not sure that we can backport it because this would change the API. --Semyon -phil.
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 9/28/2017 10:51 AM, Philip Race wrote: If this is up for consideration for backporting - as appears to be the case - then I think you should post an updated webrev. Not sure that we can backport it because this would change the API. --Semyon -phil.
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Sergey, On 9/27/2017 12:22 PM, Sergey Bylokhov wrote: On 9/26/17 14:37, Semyon Sadetsky wrote: This means that on HiDPI screen the FILE_ICON_LARGE works in similar way as FILE_ICON_SMALL on non-HiDPI screen, and the meaning of the FILE_ICON_SMALL on HiDPI is unclear, because it is half of the correct size. Small and large don't have any special meanings for HiDPI. They are some conditional sizes established by the native platform for the current screen resolution. The question what is the current screens resolution when you have two screens? We should relay on the native platform always. So, the icon size returned by the native API is the correct approach. And possibility to use any other sizes is provided as well. Why is it half of the correct size? It is the same size as for non-HiDPI and that is the correct size because otherwise java UI component that is HiDPI unaware would paint icons 2 times bigger in size than it is required. But the resolution of small/large icons may differs in case of HiDPI because it is determined by the size of the images returned by the native platform by the small/large icon queries. It is half of the correct size because on HiDPI it is better to use hidpi icons instead of lowdpi. Will the HiDPI unaware apps draw x2 icons correctly or not depends from our implementation. If we will return the MRI then it will be drawn in correct size even if the bigger(HiDPI) image will be used. This is not true. MRI has a basic size which uniquely determines its painted size in component coordinates. The size painted in component will be the same for all scales this is how HiDPI works in java. For example one of the consumer of this new API is WindowsFileView. How the code below should be changed to work on a different screens, and request the proper icon? WindowsFileChooserUI.java 1316 icon = getFileChooser().getFileSystemView().getSystemIcon(f); Why it should be changed? The code is requesting the proper icon. Because this method returns an icon of 16x16 pixels, which will be rescaled to 32x32 pixels in paint operation. The size should be equal 16x16 otherwise the component view will be distorted. But painted resolution is determined by native platform. The native platform may return icon of any size. If the size of the icon differs from 16x16 (32x32 for example) then it will be wrapped by MRI of 16x16 size. --Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
If this is up for consideration for backporting - as appears to be the case - then I think you should post an updated webrev. -phil. On 9/28/17, 10:41 AM, Semyon Sadetsky wrote: Hi Alexey, On 9/28/2017 7:28 AM, Alexey Ivanov wrote: I think 0 should really be |NULL|. Ok, but both represent the null pointer in Win32. Yes, I know. Yet NULL makes it clear that it's a null-pointer rather than an index, size… It's still zero in the latest review. Agh... I will fix it inline when push if you don't mind. --Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
Sure! -- Alexey On 28/09/2017 18:41, Semyon Sadetsky wrote: Hi Alexey, On 9/28/2017 7:28 AM, Alexey Ivanov wrote: I think 0 should really be |NULL|. Ok, but both represent the null pointer in Win32. Yes, I know. Yet NULL makes it clear that it's a null-pointer rather than an index, size… It's still zero in the latest review. Agh... I will fix it inline when push if you don't mind. --Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Alexey, On 9/28/2017 7:28 AM, Alexey Ivanov wrote: I think 0 should really be |NULL|. Ok, but both represent the null pointer in Win32. Yes, I know. Yet NULL makes it clear that it's a null-pointer rather than an index, size… It's still zero in the latest review. Agh... I will fix it inline when push if you don't mind. --Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Semyon,
On 26/09/2017 21:58, Semyon Sadetsky wrote:
Hi Alexey,
On 9/26/2017 12:29 PM, Alexey Ivanov wrote:
Hi Semyon,
ShellFolder2.cpp
944 pIcon->Extract(szBuf, index, , *0*, sz);
I think 0 should really be |NULL|.
Ok, but both represent the null pointer in Win32.
Yes, I know. Yet NULL makes it clear that it's a null-pointer rather
than an index, size…
It's still zero in the latest review.
The result of the call is ignored now. Is it intentional?
Yes, it has been working the same way before the fix. The function
returns the Icon reference which is 0 in case of OS API call error.
Win32ShellFolder2.java
1010 private static Image makeIcon(long hIcon, int bsize) {
|bsize| was called |baseSize| previously, and it conveyed the meaning
better, didn't it?
1043 if(hIcon <= 0) {
1044 if (isDirectory()) {
1045 return getShell32Icon(4, size);
1046 } else {
1047 return getShell32Icon(1, size);
1048 }
I guess I understand what hides behind 4 and 1: generic folder and
generic file icon ids. Would declaring these numbers as constants
improve readability?
Ok.
|getIcon(int size)| and |getIcon(boolean getLargeIcon)| are somewhat
similar. The latter provides additional caching. Can it be simplified
to re-use portions of new |getIcon(int size)|?
It has additional difference because of query for a fixed icon size
from another API call. It's better to leave it as it is.
Okay.
Regards,
Alexey
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.02/
--Semyon
Regards,
Alexey
On 22/09/2017 22:05, Semyon Sadetsky wrote:
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 9/26/17 14:37, Semyon Sadetsky wrote: This means that on HiDPI screen the FILE_ICON_LARGE works in similar way as FILE_ICON_SMALL on non-HiDPI screen, and the meaning of the FILE_ICON_SMALL on HiDPI is unclear, because it is half of the correct size. Small and large don't have any special meanings for HiDPI. They are some conditional sizes established by the native platform for the current screen resolution. The question what is the current screens resolution when you have two screens? Why is it half of the correct size? It is the same size as for non-HiDPI and that is the correct size because otherwise java UI component that is HiDPI unaware would paint icons 2 times bigger in size than it is required. But the resolution of small/large icons may differs in case of HiDPI because it is determined by the size of the images returned by the native platform by the small/large icon queries. It is half of the correct size because on HiDPI it is better to use hidpi icons instead of lowdpi. Will the HiDPI unaware apps draw x2 icons correctly or not depends from our implementation. If we will return the MRI then it will be drawn in correct size even if the bigger(HiDPI) image will be used. For example one of the consumer of this new API is WindowsFileView. How the code below should be changed to work on a different screens, and request the proper icon? WindowsFileChooserUI.java 1316 icon = getFileChooser().getFileSystemView().getSystemIcon(f); Why it should be changed? The code is requesting the proper icon. Because this method returns an icon of 16x16 pixels, which will be rescaled to 32x32 pixels in paint operation. -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Sergey, On 9/25/2017 1:44 PM, Sergey Bylokhov wrote: On 9/22/17 04:22, Alexey Ivanov wrote: There's no way of knowing in advance. Explorer does not restrict the size of icons (now), it's up to developers of a particular file handler to provide icons. Usually, there's only one icon with size larger than 255. If there's the icon of the requested size, Explorer will give it to you, otherwise it will scale the closest available to the requested size. Windows documentation suggests the following sizes: https://msdn.microsoft.com/en-us/library/windows/desktop/dn742485(v=vs.85).aspx#size_requirements Ok, so it means that we will support 1-128 pixels natively(MAX_ICON_SIZE) and others via MRI. We will support all sizes natively not only 1-128. Windows does the scaling. As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows API to retrieve the recommended size for small and large icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the icons must be larger. But this depends from the the DPI of the screen, we cannot just request default FILE_ICON_SMALL/FILE_ICON_LARGE. If these constants will be added then we should use something like this to get correct icons: Icon icon = getSystemIcon(file, FILE_ICON_SMALL); Icon hicon = getSystemIcon(file, icon.getIconWidth()*screenScale); or Icon icon = getSystemIcon(file); Icon hicon = getSystemIcon(file, icon.getIconWidth()*screenScale); Or we can do: Icon hicon = getSystemIcon(file, FILE_ICON_LARGE); This means that on HiDPI screen the FILE_ICON_LARGE works in similar way as FILE_ICON_SMALL on non-HiDPI screen, and the meaning of the FILE_ICON_SMALL on HiDPI is unclear, because it is half of the correct size. Small and large don't have any special meanings for HiDPI. They are some conditional sizes established by the native platform for the current screen resolution. Why is it half of the correct size? It is the same size as for non-HiDPI and that is the correct size because otherwise java UI component that is HiDPI unaware would paint icons 2 times bigger in size than it is required. But the resolution of small/large icons may differs in case of HiDPI because it is determined by the size of the images returned by the native platform by the small/large icon queries. For example one of the consumer of this new API is WindowsFileView. How the code below should be changed to work on a different screens, and request the proper icon? WindowsFileChooserUI.java 1316 icon = getFileChooser().getFileSystemView().getSystemIcon(f); Why it should be changed? The code is requesting the proper icon.
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Alexey,
On 9/26/2017 12:29 PM, Alexey Ivanov wrote:
Hi Semyon,
ShellFolder2.cpp
944 pIcon->Extract(szBuf, index, , *0*, sz);
I think 0 should really be |NULL|.
Ok, but both represent the null pointer in Win32.
The result of the call is ignored now. Is it intentional?
Yes, it has been working the same way before the fix. The function
returns the Icon reference which is 0 in case of OS API call error.
Win32ShellFolder2.java
1010 private static Image makeIcon(long hIcon, int bsize) {
|bsize| was called |baseSize| previously, and it conveyed the meaning
better, didn't it?
1043 if(hIcon <= 0) {
1044 if (isDirectory()) {
1045 return getShell32Icon(4, size);
1046 } else {
1047 return getShell32Icon(1, size);
1048 }
I guess I understand what hides behind 4 and 1: generic folder and
generic file icon ids. Would declaring these numbers as constants
improve readability?
Ok.
|getIcon(int size)| and |getIcon(boolean getLargeIcon)| are somewhat
similar. The latter provides additional caching. Can it be simplified
to re-use portions of new |getIcon(int size)|?
It has additional difference because of query for a fixed icon size from
another API call. It's better to leave it as it is.
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.02/
--Semyon
Regards,
Alexey
On 22/09/2017 22:05, Semyon Sadetsky wrote:
Hi Alexey,
On 09/22/2017 01:01 PM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 20:06, Semyon Sadetsky wrote:
Hi Alexey,
On 09/22/2017 10:53 AM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 18:29, Semyon Sadetsky wrote:
Hi Alexey,
On 09/22/2017 09:43 AM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 17:13, Semyon Sadetsky wrote:
Hi Alexey,
Thank you for your exact clarification.
On 09/22/2017 04:22 AM, Alexey Ivanov wrote:
As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using
Windows API to retrieve the recommended size for small and
large icon size rather than defaulting to 16×16 and 32×32. If
HiDPI is in effect, the icons must be larger.
I also found this as most suitable approach for the moment.
Later this may be changed, for example, if Swing JFC is
re-factored to support shell determined icon sizes at HiDPI.
Swing UI scales to accommodate HiDPI settings. If fonts are
larger then icons should be larger too. Otherwise icons are too
small compared to surrounding text.
Anyway it could be postponed to a later fix.
Does it make sense to declare the standard sizes of 16×16 and
32×32 as constants at least in Java sources? This way, it would
be easier to find the places in code where a change is necessary.
This topic requires more investigations. At first, we need to
keep the API cross-platform and this requires comparing all
supported platforms in details. At the second, even for the
existing windows implementation there is an ambiguity in icons
sizes received form the OS shell. Windows platform has number of
predefined constants to query icon sizes (small, large, extra
large, jumbo...) but their actual size may differ depending on
the shell preferences.
Those icon sizes may be changed by Windows registry settings and
may depend on the hi-res scale. I did several experiments and
found that depending on the way of desktop scaling in Windows 10
(it has two ways the new one and the old) at the same scale 2x
the returned large icon, for example, may be 32 or 64 pixels in
size (this was the root cause of the 8151385 bug).
I would postpone digging in this direction because we are not
planning to update Swing JFC dialog for better HiDPI view in the
nearest future. Also,we don't have statistics how users may use
the API. Since that, the most flexible API that leaves to the
user the decision about icon size to query seems more preferable.
I totally agree with your points. And the new API provides means
for app developer to choose better-suited size for icons.
What about using constants, private ones, for the two standard
sizes instead of using “magic” numbers?
Which constants do you mean? The next for large and small sizes?
public static final int FILE_ICON_SMALL = -1;
public static final int FILE_ICON_LARGE = -2;
they cannot be private because they are part of the new API that is
requested in the bug. The bug asks enabling the query for the large
icon that ShellFolder had been providing before. The bug itself is
not related to HiDPI. The possibility to get arbitrary icon sizes
was added because after 9 this may be in demand for HiDPI UIs.
I'm talking about these two numbers in Win32ShellFolder2.java as an
example:
public Image getIcon(final boolean getLargeIcon) {
int size = getLargeIcon ? *32* : *16*;
Does it make sense to declare constants for the size of 16 and 32.
So that the places where they're used are more easily identified if
someone decides to update the code later for supporting system icon
size.
I updated the fix.
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/
--Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Semyon,
ShellFolder2.cpp
944 pIcon->Extract(szBuf, index, , *0*, sz);
I think 0 should really be |NULL|.
The result of the call is ignored now. Is it intentional?
Win32ShellFolder2.java
1010 private static Image makeIcon(long hIcon, int bsize) {
|bsize| was called |baseSize| previously, and it conveyed the meaning
better, didn't it?
1043 if(hIcon <= 0) {
1044 if (isDirectory()) {
1045 return getShell32Icon(4, size);
1046 } else {
1047 return getShell32Icon(1, size);
1048 }
I guess I understand what hides behind 4 and 1: generic folder and
generic file icon ids. Would declaring these numbers as constants
improve readability?
|getIcon(int size)| and |getIcon(boolean getLargeIcon)| are somewhat
similar. The latter provides additional caching. Can it be simplified to
re-use portions of new |getIcon(int size)|?
Regards,
Alexey
On 22/09/2017 22:05, Semyon Sadetsky wrote:
Hi Alexey,
On 09/22/2017 01:01 PM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 20:06, Semyon Sadetsky wrote:
Hi Alexey,
On 09/22/2017 10:53 AM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 18:29, Semyon Sadetsky wrote:
Hi Alexey,
On 09/22/2017 09:43 AM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 17:13, Semyon Sadetsky wrote:
Hi Alexey,
Thank you for your exact clarification.
On 09/22/2017 04:22 AM, Alexey Ivanov wrote:
As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using
Windows API to retrieve the recommended size for small and
large icon size rather than defaulting to 16×16 and 32×32. If
HiDPI is in effect, the icons must be larger.
I also found this as most suitable approach for the moment.
Later this may be changed, for example, if Swing JFC is
re-factored to support shell determined icon sizes at HiDPI.
Swing UI scales to accommodate HiDPI settings. If fonts are
larger then icons should be larger too. Otherwise icons are too
small compared to surrounding text.
Anyway it could be postponed to a later fix.
Does it make sense to declare the standard sizes of 16×16 and
32×32 as constants at least in Java sources? This way, it would
be easier to find the places in code where a change is necessary.
This topic requires more investigations. At first, we need to keep
the API cross-platform and this requires comparing all supported
platforms in details. At the second, even for the existing windows
implementation there is an ambiguity in icons sizes received form
the OS shell. Windows platform has number of predefined constants
to query icon sizes (small, large, extra large, jumbo...) but
their actual size may differ depending on the shell preferences.
Those icon sizes may be changed by Windows registry settings and
may depend on the hi-res scale. I did several experiments and
found that depending on the way of desktop scaling in Windows 10
(it has two ways the new one and the old) at the same scale 2x the
returned large icon, for example, may be 32 or 64 pixels in size
(this was the root cause of the 8151385 bug).
I would postpone digging in this direction because we are not
planning to update Swing JFC dialog for better HiDPI view in the
nearest future. Also,we don't have statistics how users may use
the API. Since that, the most flexible API that leaves to the user
the decision about icon size to query seems more preferable.
I totally agree with your points. And the new API provides means
for app developer to choose better-suited size for icons.
What about using constants, private ones, for the two standard
sizes instead of using “magic” numbers?
Which constants do you mean? The next for large and small sizes?
public static final int FILE_ICON_SMALL = -1;
public static final int FILE_ICON_LARGE = -2;
they cannot be private because they are part of the new API that is
requested in the bug. The bug asks enabling the query for the large
icon that ShellFolder had been providing before. The bug itself is
not related to HiDPI. The possibility to get arbitrary icon sizes
was added because after 9 this may be in demand for HiDPI UIs.
I'm talking about these two numbers in Win32ShellFolder2.java as an
example:
public Image getIcon(final boolean getLargeIcon) {
int size = getLargeIcon ? *32* : *16*;
Does it make sense to declare constants for the size of 16 and 32. So
that the places where they're used are more easily identified if
someone decides to update the code later for supporting system icon size.
I updated the fix.
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/
--Semyon
Regards,
Alexey
--Semyon
Other than that, the fix looks good to me.
Regards,
Alexey
--Semyon
Regards,
Alexey
--Semyon
Regards,
Alexey
On 9/13/17 11:01, Semyon Sadetsky wrote:
Hello,
Please review fix for JDK10 (the changes involve AWT and
Swing):
bug: https://bugs.openjdk.java.net/browse/JDK-8182043
webrev:
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
The fix opens the part of the ShellFolder API
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 9/22/17 04:22, Alexey Ivanov wrote: There's no way of knowing in advance. Explorer does not restrict the size of icons (now), it's up to developers of a particular file handler to provide icons. Usually, there's only one icon with size larger than 255. If there's the icon of the requested size, Explorer will give it to you, otherwise it will scale the closest available to the requested size. Windows documentation suggests the following sizes: https://msdn.microsoft.com/en-us/library/windows/desktop/dn742485(v=vs.85).aspx#size_requirements Ok, so it means that we will support 1-128 pixels natively(MAX_ICON_SIZE) and others via MRI. As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows API to retrieve the recommended size for small and large icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the icons must be larger. But this depends from the the DPI of the screen, we cannot just request default FILE_ICON_SMALL/FILE_ICON_LARGE. If these constants will be added then we should use something like this to get correct icons: Icon icon = getSystemIcon(file, FILE_ICON_SMALL); Icon hicon = getSystemIcon(file, icon.getIconWidth()*screenScale); or Icon icon = getSystemIcon(file); Icon hicon = getSystemIcon(file, icon.getIconWidth()*screenScale); Or we can do: Icon hicon = getSystemIcon(file, FILE_ICON_LARGE); This means that on HiDPI screen the FILE_ICON_LARGE works in similar way as FILE_ICON_SMALL on non-HiDPI screen, and the meaning of the FILE_ICON_SMALL on HiDPI is unclear, because it is half of the correct size. For example one of the consumer of this new API is WindowsFileView. How the code below should be changed to work on a different screens, and request the proper icon? WindowsFileChooserUI.java 1316 icon = getFileChooser().getFileSystemView().getSystemIcon(f); -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Alexey,
On 09/22/2017 01:01 PM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 20:06, Semyon Sadetsky wrote:
Hi Alexey,
On 09/22/2017 10:53 AM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 18:29, Semyon Sadetsky wrote:
Hi Alexey,
On 09/22/2017 09:43 AM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 17:13, Semyon Sadetsky wrote:
Hi Alexey,
Thank you for your exact clarification.
On 09/22/2017 04:22 AM, Alexey Ivanov wrote:
As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using
Windows API to retrieve the recommended size for small and large
icon size rather than defaulting to 16×16 and 32×32. If HiDPI is
in effect, the icons must be larger.
I also found this as most suitable approach for the moment.
Later this may be changed, for example, if Swing JFC is
re-factored to support shell determined icon sizes at HiDPI.
Swing UI scales to accommodate HiDPI settings. If fonts are larger
then icons should be larger too. Otherwise icons are too small
compared to surrounding text.
Anyway it could be postponed to a later fix.
Does it make sense to declare the standard sizes of 16×16 and
32×32 as constants at least in Java sources? This way, it would be
easier to find the places in code where a change is necessary.
This topic requires more investigations. At first, we need to keep
the API cross-platform and this requires comparing all supported
platforms in details. At the second, even for the existing windows
implementation there is an ambiguity in icons sizes received form
the OS shell. Windows platform has number of predefined constants
to query icon sizes (small, large, extra large, jumbo...) but their
actual size may differ depending on the shell preferences.
Those icon sizes may be changed by Windows registry settings and
may depend on the hi-res scale. I did several experiments and found
that depending on the way of desktop scaling in Windows 10 (it has
two ways the new one and the old) at the same scale 2x the returned
large icon, for example, may be 32 or 64 pixels in size (this was
the root cause of the 8151385 bug).
I would postpone digging in this direction because we are not
planning to update Swing JFC dialog for better HiDPI view in the
nearest future. Also,we don't have statistics how users may use the
API. Since that, the most flexible API that leaves to the user the
decision about icon size to query seems more preferable.
I totally agree with your points. And the new API provides means for
app developer to choose better-suited size for icons.
What about using constants, private ones, for the two standard sizes
instead of using “magic” numbers?
Which constants do you mean? The next for large and small sizes?
public static final int FILE_ICON_SMALL = -1;
public static final int FILE_ICON_LARGE = -2;
they cannot be private because they are part of the new API that is
requested in the bug. The bug asks enabling the query for the large
icon that ShellFolder had been providing before. The bug itself is
not related to HiDPI. The possibility to get arbitrary icon sizes was
added because after 9 this may be in demand for HiDPI UIs.
I'm talking about these two numbers in Win32ShellFolder2.java as an
example:
public Image getIcon(final boolean getLargeIcon) {
int size = getLargeIcon ? *32* : *16*;
Does it make sense to declare constants for the size of 16 and 32. So
that the places where they're used are more easily identified if
someone decides to update the code later for supporting system icon size.
I updated the fix.
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.01/
--Semyon
Regards,
Alexey
--Semyon
Other than that, the fix looks good to me.
Regards,
Alexey
--Semyon
Regards,
Alexey
--Semyon
Regards,
Alexey
On 9/13/17 11:01, Semyon Sadetsky wrote:
Hello,
Please review fix for JDK10 (the changes involve AWT and
Swing):
bug: https://bugs.openjdk.java.net/browse/JDK-8182043
webrev:
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
The fix opens the part of the ShellFolder API for getting
system icons which was decided to be left closed during
the 8081722 enhancement review in 9.
Also the fix extends the API by adding possibility to
query file icons of arbitrary size and implements this
extension for Windows platform.
--Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Semyon,
On 22/09/2017 20:06, Semyon Sadetsky wrote:
Hi Alexey,
On 09/22/2017 10:53 AM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 18:29, Semyon Sadetsky wrote:
Hi Alexey,
On 09/22/2017 09:43 AM, Alexey Ivanov wrote:
Hi Semyon,
On 22/09/2017 17:13, Semyon Sadetsky wrote:
Hi Alexey,
Thank you for your exact clarification.
On 09/22/2017 04:22 AM, Alexey Ivanov wrote:
As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using
Windows API to retrieve the recommended size for small and large
icon size rather than defaulting to 16×16 and 32×32. If HiDPI is
in effect, the icons must be larger.
I also found this as most suitable approach for the moment.
Later this may be changed, for example, if Swing JFC is
re-factored to support shell determined icon sizes at HiDPI.
Swing UI scales to accommodate HiDPI settings. If fonts are larger
then icons should be larger too. Otherwise icons are too small
compared to surrounding text.
Anyway it could be postponed to a later fix.
Does it make sense to declare the standard sizes of 16×16 and 32×32
as constants at least in Java sources? This way, it would be easier
to find the places in code where a change is necessary.
This topic requires more investigations. At first, we need to keep
the API cross-platform and this requires comparing all supported
platforms in details. At the second, even for the existing windows
implementation there is an ambiguity in icons sizes received form
the OS shell. Windows platform has number of predefined constants to
query icon sizes (small, large, extra large, jumbo...) but their
actual size may differ depending on the shell preferences.
Those icon sizes may be changed by Windows registry settings and may
depend on the hi-res scale. I did several experiments and found that
depending on the way of desktop scaling in Windows 10 (it has two
ways the new one and the old) at the same scale 2x the returned
large icon, for example, may be 32 or 64 pixels in size (this was
the root cause of the 8151385 bug).
I would postpone digging in this direction because we are not
planning to update Swing JFC dialog for better HiDPI view in the
nearest future. Also,we don't have statistics how users may use the
API. Since that, the most flexible API that leaves to the user the
decision about icon size to query seems more preferable.
I totally agree with your points. And the new API provides means for
app developer to choose better-suited size for icons.
What about using constants, private ones, for the two standard sizes
instead of using “magic” numbers?
Which constants do you mean? The next for large and small sizes?
public static final int FILE_ICON_SMALL = -1;
public static final int FILE_ICON_LARGE = -2;
they cannot be private because they are part of the new API that is
requested in the bug. The bug asks enabling the query for the large
icon that ShellFolder had been providing before. The bug itself is not
related to HiDPI. The possibility to get arbitrary icon sizes was
added because after 9 this may be in demand for HiDPI UIs.
I'm talking about these two numbers in Win32ShellFolder2.java as an example:
public Image getIcon(final boolean getLargeIcon) {
int size = getLargeIcon ? *32* : *16*;
Does it make sense to declare constants for the size of 16 and 32. So
that the places where they're used are more easily identified if someone
decides to update the code later for supporting system icon size.
Regards,
Alexey
--Semyon
Other than that, the fix looks good to me.
Regards,
Alexey
--Semyon
Regards,
Alexey
--Semyon
Regards,
Alexey
On 9/13/17 11:01, Semyon Sadetsky wrote:
Hello,
Please review fix for JDK10 (the changes involve AWT and
Swing):
bug: https://bugs.openjdk.java.net/browse/JDK-8182043
webrev:
http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/
The fix opens the part of the ShellFolder API for getting
system icons which was decided to be left closed during the
8081722 enhancement review in 9.
Also the fix extends the API by adding possibility to query
file icons of arbitrary size and implements this extension
for Windows platform.
--Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Alexey, On 09/22/2017 10:53 AM, Alexey Ivanov wrote: Hi Semyon, On 22/09/2017 18:29, Semyon Sadetsky wrote: Hi Alexey, On 09/22/2017 09:43 AM, Alexey Ivanov wrote: Hi Semyon, On 22/09/2017 17:13, Semyon Sadetsky wrote: Hi Alexey, Thank you for your exact clarification. On 09/22/2017 04:22 AM, Alexey Ivanov wrote: As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows API to retrieve the recommended size for small and large icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the icons must be larger. I also found this as most suitable approach for the moment. Later this may be changed, for example, if Swing JFC is re-factored to support shell determined icon sizes at HiDPI. Swing UI scales to accommodate HiDPI settings. If fonts are larger then icons should be larger too. Otherwise icons are too small compared to surrounding text. Anyway it could be postponed to a later fix. Does it make sense to declare the standard sizes of 16×16 and 32×32 as constants at least in Java sources? This way, it would be easier to find the places in code where a change is necessary. This topic requires more investigations. At first, we need to keep the API cross-platform and this requires comparing all supported platforms in details. At the second, even for the existing windows implementation there is an ambiguity in icons sizes received form the OS shell. Windows platform has number of predefined constants to query icon sizes (small, large, extra large, jumbo...) but their actual size may differ depending on the shell preferences. Those icon sizes may be changed by Windows registry settings and may depend on the hi-res scale. I did several experiments and found that depending on the way of desktop scaling in Windows 10 (it has two ways the new one and the old) at the same scale 2x the returned large icon, for example, may be 32 or 64 pixels in size (this was the root cause of the 8151385 bug). I would postpone digging in this direction because we are not planning to update Swing JFC dialog for better HiDPI view in the nearest future. Also,we don't have statistics how users may use the API. Since that, the most flexible API that leaves to the user the decision about icon size to query seems more preferable. I totally agree with your points. And the new API provides means for app developer to choose better-suited size for icons. What about using constants, private ones, for the two standard sizes instead of using “magic” numbers? Which constants do you mean? The next for large and small sizes? public static final int FILE_ICON_SMALL = -1; public static final int FILE_ICON_LARGE = -2; they cannot be private because they are part of the new API that is requested in the bug. The bug asks enabling the query for the large icon that ShellFolder had been providing before. The bug itself is not related to HiDPI. The possibility to get arbitrary icon sizes was added because after 9 this may be in demand for HiDPI UIs. --Semyon Other than that, the fix looks good to me. Regards, Alexey --Semyon Regards, Alexey --Semyon Regards, Alexey On 9/13/17 11:01, Semyon Sadetsky wrote: Hello, Please review fix for JDK10 (the changes involve AWT and Swing): bug: https://bugs.openjdk.java.net/browse/JDK-8182043 webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/ The fix opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9. Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform. --Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Semyon, On 22/09/2017 18:29, Semyon Sadetsky wrote: Hi Alexey, On 09/22/2017 09:43 AM, Alexey Ivanov wrote: Hi Semyon, On 22/09/2017 17:13, Semyon Sadetsky wrote: Hi Alexey, Thank you for your exact clarification. On 09/22/2017 04:22 AM, Alexey Ivanov wrote: As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows API to retrieve the recommended size for small and large icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the icons must be larger. I also found this as most suitable approach for the moment. Later this may be changed, for example, if Swing JFC is re-factored to support shell determined icon sizes at HiDPI. Swing UI scales to accommodate HiDPI settings. If fonts are larger then icons should be larger too. Otherwise icons are too small compared to surrounding text. Anyway it could be postponed to a later fix. Does it make sense to declare the standard sizes of 16×16 and 32×32 as constants at least in Java sources? This way, it would be easier to find the places in code where a change is necessary. This topic requires more investigations. At first, we need to keep the API cross-platform and this requires comparing all supported platforms in details. At the second, even for the existing windows implementation there is an ambiguity in icons sizes received form the OS shell. Windows platform has number of predefined constants to query icon sizes (small, large, extra large, jumbo...) but their actual size may differ depending on the shell preferences. Those icon sizes may be changed by Windows registry settings and may depend on the hi-res scale. I did several experiments and found that depending on the way of desktop scaling in Windows 10 (it has two ways the new one and the old) at the same scale 2x the returned large icon, for example, may be 32 or 64 pixels in size (this was the root cause of the 8151385 bug). I would postpone digging in this direction because we are not planning to update Swing JFC dialog for better HiDPI view in the nearest future. Also,we don't have statistics how users may use the API. Since that, the most flexible API that leaves to the user the decision about icon size to query seems more preferable. I totally agree with your points. And the new API provides means for app developer to choose better-suited size for icons. What about using constants, private ones, for the two standard sizes instead of using “magic” numbers? Other than that, the fix looks good to me. Regards, Alexey --Semyon Regards, Alexey --Semyon Regards, Alexey On 9/13/17 11:01, Semyon Sadetsky wrote: Hello, Please review fix for JDK10 (the changes involve AWT and Swing): bug: https://bugs.openjdk.java.net/browse/JDK-8182043 webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/ The fix opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9. Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform. --Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Alexey, On 09/22/2017 09:43 AM, Alexey Ivanov wrote: Hi Semyon, On 22/09/2017 17:13, Semyon Sadetsky wrote: Hi Alexey, Thank you for your exact clarification. On 09/22/2017 04:22 AM, Alexey Ivanov wrote: As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows API to retrieve the recommended size for small and large icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the icons must be larger. I also found this as most suitable approach for the moment. Later this may be changed, for example, if Swing JFC is re-factored to support shell determined icon sizes at HiDPI. Swing UI scales to accommodate HiDPI settings. If fonts are larger then icons should be larger too. Otherwise icons are too small compared to surrounding text. Anyway it could be postponed to a later fix. Does it make sense to declare the standard sizes of 16×16 and 32×32 as constants at least in Java sources? This way, it would be easier to find the places in code where a change is necessary. This topic requires more investigations. At first, we need to keep the API cross-platform and this requires comparing all supported platforms in details. At the second, even for the existing windows implementation there is an ambiguity in icons sizes received form the OS shell. Windows platform has number of predefined constants to query icon sizes (small, large, extra large, jumbo...) but their actual size may differ depending on the shell preferences. Those icon sizes may be changed by Windows registry settings and may depend on the hi-res scale. I did several experiments and found that depending on the way of desktop scaling in Windows 10 (it has two ways the new one and the old) at the same scale 2x the returned large icon, for example, may be 32 or 64 pixels in size (this was the root cause of the 8151385 bug). I would postpone digging in this direction because we are not planning to update Swing JFC dialog for better HiDPI view in the nearest future. Also,we don't have statistics how users may use the API. Since that, the most flexible API that leaves to the user the decision about icon size to query seems more preferable. --Semyon Regards, Alexey --Semyon Regards, Alexey On 9/13/17 11:01, Semyon Sadetsky wrote: Hello, Please review fix for JDK10 (the changes involve AWT and Swing): bug: https://bugs.openjdk.java.net/browse/JDK-8182043 webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/ The fix opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9. Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform. --Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Semyon, On 22/09/2017 17:13, Semyon Sadetsky wrote: Hi Alexey, Thank you for your exact clarification. On 09/22/2017 04:22 AM, Alexey Ivanov wrote: As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows API to retrieve the recommended size for small and large icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the icons must be larger. I also found this as most suitable approach for the moment. Later this may be changed, for example, if Swing JFC is re-factored to support shell determined icon sizes at HiDPI. Swing UI scales to accommodate HiDPI settings. If fonts are larger then icons should be larger too. Otherwise icons are too small compared to surrounding text. Anyway it could be postponed to a later fix. Does it make sense to declare the standard sizes of 16×16 and 32×32 as constants at least in Java sources? This way, it would be easier to find the places in code where a change is necessary. Regards, Alexey --Semyon Regards, Alexey On 9/13/17 11:01, Semyon Sadetsky wrote: Hello, Please review fix for JDK10 (the changes involve AWT and Swing): bug: https://bugs.openjdk.java.net/browse/JDK-8182043 webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/ The fix opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9. Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform. --Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Alexey, Thank you for your exact clarification. On 09/22/2017 04:22 AM, Alexey Ivanov wrote: Hi Sergey, On 22/09/2017 04:21, Sergey Bylokhov wrote: On 9/21/17 14:12, Semyon Sadetsky wrote: On 09/21/2017 01:52 PM, Sergey Bylokhov wrote: On 9/21/17 08:54, Semyon Sadetsky wrote: On 09/20/2017 02:41 PM, Sergey Bylokhov wrote: Hi, Semyon I have some initial comments which are based on the two bugs: JDK-8182043 and JDK-8156183. getSystemIcon(File file, int size): - How the user will know what values/sizes should be passed, what values are supported? It is unlikely that he will pass all values in between 8-256? Supported sizes are described in the method spec, aren't they? This API doesn't imply any size limitation like the 8-256 you mentioned. Does it mean that all sizes will be supported(1-Integer.MAX_INT)? I guess it is unlikely that the the explorer.exe will contains the icons bigger than 1024. This is a a cross-platform API, not a windows specific implementation. This was an example, and the question was how the user will know what icons are supported by some specific file. There's no way of knowing in advance. Explorer does not restrict the size of icons (now), it's up to developers of a particular file handler to provide icons. Usually, there's only one icon with size larger than 255. If there's the icon of the requested size, Explorer will give it to you, otherwise it will scale the closest available to the requested size. This I think a general approach for all platforms. Since the icons size may be set to arbitrarily value in the shell the shell should have a way to scale to it. Windows documentation suggests the following sizes: https://msdn.microsoft.com/en-us/library/windows/desktop/dn742485(v=vs.85).aspx#size_requirements As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows API to retrieve the recommended size for small and large icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the icons must be larger. I also found this as most suitable approach for the moment. Later this may be changed, for example, if Swing JFC is re-factored to support shell determined icon sizes at HiDPI. --Semyon Regards, Alexey "For any positive size value the exact file icon size is queried": - This should be double checked because our implementation can return MultiResolutionIconImage if the system returns the icon which size is different from requested. FILE_ICON_SMALL(FILE_ICON_LARGE); - What these parameters mean? Is it the smallest(biggest) supported size or is it a default size? Can it be different if different dpi are used on the screen? For example 16(32) by default and 32(64) on HiDPI? They means what they have been meaning FileChooserUI implementation for the Windows L which operates by two fixed icon sizes, large and small. But it is not necessary will be used by our implementation of FileChooserUI when this API became public. For example in the description of JDK-8156183 the user said that large icons are used in "a media file browser" and "32x32 isn't large by the standards of current-millennium operating systems". But even in our FileChooserUI implementation shouldn't we use x2 icons in case of HiDPI? FILE_ICON_SMALL - is it a smallest supported size? User may use the new method to get icons at any resolution. Small/large sizes are preserved for backward compatibility, because before this change only those two sizes were available. Backward compatibility to what? There was know public API for this. It is still unclear what is a "the small or large file icon variant" means. FILE_ICON_SMALL: - It seems that this value duplicate functionality of the old getSystemIcon(File) method? How this can be got from the spec? It may return the same size but not necessarily. Then how the old method and the new one are related? Will the old method returns the size in between FILE_ICON_SMALL and FILE_ICON_LARGE? I didn't get how it would be possible? Possible what? It is unclear how the two methods are related to each other. Probably it will be better to provide to the user the set(list/mri/array/etc) of supported images, or if it is really slow the set(list/mri/array/etc) of supported sizes, and the user will be able to pass some meaningful sizes. This is not a good idea. Extracting all available icon resolutions might take significant time and since icons are cached it would be waste of RAM. It should be measured, we can try to load them lazy, or provide the list of sizes which are supported. Sorry, I didn't get what are you proposing to measure? And how do you propose to get the icon size without reading the image? On 9/13/17 11:01, Semyon Sadetsky wrote: Hello, Please review fix for JDK10 (the changes involve AWT and Swing): bug: https://bugs.openjdk.java.net/browse/JDK-8182043 webrev:
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi Sergey, On 22/09/2017 04:21, Sergey Bylokhov wrote: On 9/21/17 14:12, Semyon Sadetsky wrote: On 09/21/2017 01:52 PM, Sergey Bylokhov wrote: On 9/21/17 08:54, Semyon Sadetsky wrote: On 09/20/2017 02:41 PM, Sergey Bylokhov wrote: Hi, Semyon I have some initial comments which are based on the two bugs: JDK-8182043 and JDK-8156183. getSystemIcon(File file, int size): - How the user will know what values/sizes should be passed, what values are supported? It is unlikely that he will pass all values in between 8-256? Supported sizes are described in the method spec, aren't they? This API doesn't imply any size limitation like the 8-256 you mentioned. Does it mean that all sizes will be supported(1-Integer.MAX_INT)? I guess it is unlikely that the the explorer.exe will contains the icons bigger than 1024. This is a a cross-platform API, not a windows specific implementation. This was an example, and the question was how the user will know what icons are supported by some specific file. There's no way of knowing in advance. Explorer does not restrict the size of icons (now), it's up to developers of a particular file handler to provide icons. Usually, there's only one icon with size larger than 255. If there's the icon of the requested size, Explorer will give it to you, otherwise it will scale the closest available to the requested size. Windows documentation suggests the following sizes: https://msdn.microsoft.com/en-us/library/windows/desktop/dn742485(v=vs.85).aspx#size_requirements As for FILE_ICON_SMALL and FILE_ICON_LARGE, I'd suggest using Windows API to retrieve the recommended size for small and large icon size rather than defaulting to 16×16 and 32×32. If HiDPI is in effect, the icons must be larger. Regards, Alexey "For any positive size value the exact file icon size is queried": - This should be double checked because our implementation can return MultiResolutionIconImage if the system returns the icon which size is different from requested. FILE_ICON_SMALL(FILE_ICON_LARGE); - What these parameters mean? Is it the smallest(biggest) supported size or is it a default size? Can it be different if different dpi are used on the screen? For example 16(32) by default and 32(64) on HiDPI? They means what they have been meaning FileChooserUI implementation for the Windows L which operates by two fixed icon sizes, large and small. But it is not necessary will be used by our implementation of FileChooserUI when this API became public. For example in the description of JDK-8156183 the user said that large icons are used in "a media file browser" and "32x32 isn't large by the standards of current-millennium operating systems". But even in our FileChooserUI implementation shouldn't we use x2 icons in case of HiDPI? FILE_ICON_SMALL - is it a smallest supported size? User may use the new method to get icons at any resolution. Small/large sizes are preserved for backward compatibility, because before this change only those two sizes were available. Backward compatibility to what? There was know public API for this. It is still unclear what is a "the small or large file icon variant" means. FILE_ICON_SMALL: - It seems that this value duplicate functionality of the old getSystemIcon(File) method? How this can be got from the spec? It may return the same size but not necessarily. Then how the old method and the new one are related? Will the old method returns the size in between FILE_ICON_SMALL and FILE_ICON_LARGE? I didn't get how it would be possible? Possible what? It is unclear how the two methods are related to each other. Probably it will be better to provide to the user the set(list/mri/array/etc) of supported images, or if it is really slow the set(list/mri/array/etc) of supported sizes, and the user will be able to pass some meaningful sizes. This is not a good idea. Extracting all available icon resolutions might take significant time and since icons are cached it would be waste of RAM. It should be measured, we can try to load them lazy, or provide the list of sizes which are supported. Sorry, I didn't get what are you proposing to measure? And how do you propose to get the icon size without reading the image? On 9/13/17 11:01, Semyon Sadetsky wrote: Hello, Please review fix for JDK10 (the changes involve AWT and Swing): bug: https://bugs.openjdk.java.net/browse/JDK-8182043 webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/ The fix opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9. Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform. --Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 9/21/17 14:12, Semyon Sadetsky wrote: On 09/21/2017 01:52 PM, Sergey Bylokhov wrote: On 9/21/17 08:54, Semyon Sadetsky wrote: On 09/20/2017 02:41 PM, Sergey Bylokhov wrote: Hi, Semyon I have some initial comments which are based on the two bugs: JDK-8182043 and JDK-8156183. getSystemIcon(File file, int size): - How the user will know what values/sizes should be passed, what values are supported? It is unlikely that he will pass all values in between 8-256? Supported sizes are described in the method spec, aren't they? This API doesn't imply any size limitation like the 8-256 you mentioned. Does it mean that all sizes will be supported(1-Integer.MAX_INT)? I guess it is unlikely that the the explorer.exe will contains the icons bigger than 1024. This is a a cross-platform API, not a windows specific implementation. This was an example, and the question was how the user will know what icons are supported by some specific file. "For any positive size value the exact file icon size is queried": - This should be double checked because our implementation can return MultiResolutionIconImage if the system returns the icon which size is different from requested. FILE_ICON_SMALL(FILE_ICON_LARGE); - What these parameters mean? Is it the smallest(biggest) supported size or is it a default size? Can it be different if different dpi are used on the screen? For example 16(32) by default and 32(64) on HiDPI? They means what they have been meaning FileChooserUI implementation for the Windows L which operates by two fixed icon sizes, large and small. But it is not necessary will be used by our implementation of FileChooserUI when this API became public. For example in the description of JDK-8156183 the user said that large icons are used in "a media file browser" and "32x32 isn't large by the standards of current-millennium operating systems". But even in our FileChooserUI implementation shouldn't we use x2 icons in case of HiDPI? FILE_ICON_SMALL - is it a smallest supported size? User may use the new method to get icons at any resolution. Small/large sizes are preserved for backward compatibility, because before this change only those two sizes were available. Backward compatibility to what? There was know public API for this. It is still unclear what is a "the small or large file icon variant" means. FILE_ICON_SMALL: - It seems that this value duplicate functionality of the old getSystemIcon(File) method? How this can be got from the spec? It may return the same size but not necessarily. Then how the old method and the new one are related? Will the old method returns the size in between FILE_ICON_SMALL and FILE_ICON_LARGE? I didn't get how it would be possible? Possible what? It is unclear how the two methods are related to each other. Probably it will be better to provide to the user the set(list/mri/array/etc) of supported images, or if it is really slow the set(list/mri/array/etc) of supported sizes, and the user will be able to pass some meaningful sizes. This is not a good idea. Extracting all available icon resolutions might take significant time and since icons are cached it would be waste of RAM. It should be measured, we can try to load them lazy, or provide the list of sizes which are supported. Sorry, I didn't get what are you proposing to measure? And how do you propose to get the icon size without reading the image? On 9/13/17 11:01, Semyon Sadetsky wrote: Hello, Please review fix for JDK10 (the changes involve AWT and Swing): bug: https://bugs.openjdk.java.net/browse/JDK-8182043 webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/ The fix opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9. Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform. --Semyon -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 09/21/2017 01:52 PM, Sergey Bylokhov wrote: On 9/21/17 08:54, Semyon Sadetsky wrote: On 09/20/2017 02:41 PM, Sergey Bylokhov wrote: Hi, Semyon I have some initial comments which are based on the two bugs: JDK-8182043 and JDK-8156183. getSystemIcon(File file, int size): - How the user will know what values/sizes should be passed, what values are supported? It is unlikely that he will pass all values in between 8-256? Supported sizes are described in the method spec, aren't they? This API doesn't imply any size limitation like the 8-256 you mentioned. Does it mean that all sizes will be supported(1-Integer.MAX_INT)? I guess it is unlikely that the the explorer.exe will contains the icons bigger than 1024. This is a a cross-platform API, not a windows specific implementation. "For any positive size value the exact file icon size is queried": - This should be double checked because our implementation can return MultiResolutionIconImage if the system returns the icon which size is different from requested. FILE_ICON_SMALL(FILE_ICON_LARGE); - What these parameters mean? Is it the smallest(biggest) supported size or is it a default size? Can it be different if different dpi are used on the screen? For example 16(32) by default and 32(64) on HiDPI? They means what they have been meaning FileChooserUI implementation for the Windows L which operates by two fixed icon sizes, large and small. But it is not necessary will be used by our implementation of FileChooserUI when this API became public. For example in the description of JDK-8156183 the user said that large icons are used in "a media file browser" and "32x32 isn't large by the standards of current-millennium operating systems". But even in our FileChooserUI implementation shouldn't we use x2 icons in case of HiDPI? FILE_ICON_SMALL - is it a smallest supported size? User may use the new method to get icons at any resolution. Small/large sizes are preserved for backward compatibility, because before this change only those two sizes were available. FILE_ICON_SMALL: - It seems that this value duplicate functionality of the old getSystemIcon(File) method? How this can be got from the spec? It may return the same size but not necessarily. Then how the old method and the new one are related? Will the old method returns the size in between FILE_ICON_SMALL and FILE_ICON_LARGE? I didn't get how it would be possible? Probably it will be better to provide to the user the set(list/mri/array/etc) of supported images, or if it is really slow the set(list/mri/array/etc) of supported sizes, and the user will be able to pass some meaningful sizes. This is not a good idea. Extracting all available icon resolutions might take significant time and since icons are cached it would be waste of RAM. It should be measured, we can try to load them lazy, or provide the list of sizes which are supported. Sorry, I didn't get what are you proposing to measure? And how do you propose to get the icon size without reading the image? On 9/13/17 11:01, Semyon Sadetsky wrote: Hello, Please review fix for JDK10 (the changes involve AWT and Swing): bug: https://bugs.openjdk.java.net/browse/JDK-8182043 webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/ The fix opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9. Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform. --Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 9/21/17 08:54, Semyon Sadetsky wrote: On 09/20/2017 02:41 PM, Sergey Bylokhov wrote: Hi, Semyon I have some initial comments which are based on the two bugs: JDK-8182043 and JDK-8156183. getSystemIcon(File file, int size): - How the user will know what values/sizes should be passed, what values are supported? It is unlikely that he will pass all values in between 8-256? Supported sizes are described in the method spec, aren't they? This API doesn't imply any size limitation like the 8-256 you mentioned. Does it mean that all sizes will be supported(1-Integer.MAX_INT)? I guess it is unlikely that the the explorer.exe will contains the icons bigger than 1024. "For any positive size value the exact file icon size is queried": - This should be double checked because our implementation can return MultiResolutionIconImage if the system returns the icon which size is different from requested. FILE_ICON_SMALL(FILE_ICON_LARGE); - What these parameters mean? Is it the smallest(biggest) supported size or is it a default size? Can it be different if different dpi are used on the screen? For example 16(32) by default and 32(64) on HiDPI? They means what they have been meaning FileChooserUI implementation for the Windows L which operates by two fixed icon sizes, large and small. But it is not necessary will be used by our implementation of FileChooserUI when this API became public. For example in the description of JDK-8156183 the user said that large icons are used in "a media file browser" and "32x32 isn't large by the standards of current-millennium operating systems". But even in our FileChooserUI implementation shouldn't we use x2 icons in case of HiDPI? FILE_ICON_SMALL - is it a smallest supported size? FILE_ICON_SMALL: - It seems that this value duplicate functionality of the old getSystemIcon(File) method? How this can be got from the spec? It may return the same size but not necessarily. Then how the old method and the new one are related? Will the old method returns the size in between FILE_ICON_SMALL and FILE_ICON_LARGE? Probably it will be better to provide to the user the set(list/mri/array/etc) of supported images, or if it is really slow the set(list/mri/array/etc) of supported sizes, and the user will be able to pass some meaningful sizes. This is not a good idea. Extracting all available icon resolutions might take significant time and since icons are cached it would be waste of RAM. It should be measured, we can try to load them lazy, or provide the list of sizes which are supported. On 9/13/17 11:01, Semyon Sadetsky wrote: Hello, Please review fix for JDK10 (the changes involve AWT and Swing): bug: https://bugs.openjdk.java.net/browse/JDK-8182043 webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/ The fix opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9. Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform. --Semyon -- Best regards, Sergey.
Re: [10] Review request for 8182043: Access to Windows Large Icons
On 09/20/2017 02:41 PM, Sergey Bylokhov wrote: Hi, Semyon I have some initial comments which are based on the two bugs: JDK-8182043 and JDK-8156183. getSystemIcon(File file, int size): - How the user will know what values/sizes should be passed, what values are supported? It is unlikely that he will pass all values in between 8-256? Supported sizes are described in the method spec, aren't they? This API doesn't imply any size limitation like the 8-256 you mentioned. "For any positive size value the exact file icon size is queried": - This should be double checked because our implementation can return MultiResolutionIconImage if the system returns the icon which size is different from requested. FILE_ICON_SMALL(FILE_ICON_LARGE); - What these parameters mean? Is it the smallest(biggest) supported size or is it a default size? Can it be different if different dpi are used on the screen? For example 16(32) by default and 32(64) on HiDPI? They means what they have been meaning FileChooserUI implementation for the Windows L which operates by two fixed icon sizes, large and small. FILE_ICON_SMALL: - It seems that this value duplicate functionality of the old getSystemIcon(File) method? How this can be got from the spec? It may return the same size but not necessarily. Probably it will be better to provide to the user the set(list/mri/array/etc) of supported images, or if it is really slow the set(list/mri/array/etc) of supported sizes, and the user will be able to pass some meaningful sizes. This is not a good idea. Extracting all available icon resolutions might take significant time and since icons are cached it would be waste of RAM. On 9/13/17 11:01, Semyon Sadetsky wrote: Hello, Please review fix for JDK10 (the changes involve AWT and Swing): bug: https://bugs.openjdk.java.net/browse/JDK-8182043 webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/ The fix opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9. Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform. --Semyon
Re: [10] Review request for 8182043: Access to Windows Large Icons
Hi, Semyon I have some initial comments which are based on the two bugs: JDK-8182043 and JDK-8156183. getSystemIcon(File file, int size): - How the user will know what values/sizes should be passed, what values are supported? It is unlikely that he will pass all values in between 8-256? "For any positive size value the exact file icon size is queried": - This should be double checked because our implementation can return MultiResolutionIconImage if the system returns the icon which size is different from requested. FILE_ICON_SMALL(FILE_ICON_LARGE); - What these parameters mean? Is it the smallest(biggest) supported size or is it a default size? Can it be different if different dpi are used on the screen? For example 16(32) by default and 32(64) on HiDPI? FILE_ICON_SMALL: - It seems that this value duplicate functionality of the old getSystemIcon(File) method? Probably it will be better to provide to the user the set(list/mri/array/etc) of supported images, or if it is really slow the set(list/mri/array/etc) of supported sizes, and the user will be able to pass some meaningful sizes. On 9/13/17 11:01, Semyon Sadetsky wrote: Hello, Please review fix for JDK10 (the changes involve AWT and Swing): bug: https://bugs.openjdk.java.net/browse/JDK-8182043 webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/ The fix opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9. Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform. --Semyon -- Best regards, Sergey.
[10] Review request for 8182043: Access to Windows Large Icons
Hello, Please review fix for JDK10 (the changes involve AWT and Swing): bug: https://bugs.openjdk.java.net/browse/JDK-8182043 webrev: http://cr.openjdk.java.net/~ssadetsky/8182043/webrev.00/ The fix opens the part of the ShellFolder API for getting system icons which was decided to be left closed during the 8081722 enhancement review in 9. Also the fix extends the API by adding possibility to query file icons of arbitrary size and implements this extension for Windows platform. --Semyon
