Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-11-09 Thread Phil Race
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-24 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-23 Thread Sergey Bylokhov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-17 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-17 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-12 Thread Sergey Bylokhov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-06 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-06 Thread Sergey Bylokhov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-06 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-06 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-06 Thread Sergey Bylokhov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-06 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-06 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-06 Thread Sergey Bylokhov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-06 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-06 Thread Alexey Ivanov
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.

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-06 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-06 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-10-05 Thread Sergey Bylokhov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-29 Thread Semyon Sadetsky
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:

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-29 Thread Sergey Bylokhov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-29 Thread Sergey Bylokhov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-29 Thread Sergey Bylokhov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-29 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-29 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-28 Thread Sergey Bylokhov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-28 Thread Philip Race
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 +

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-28 Thread Semyon Sadetsky
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

2017-09-28 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-28 Thread Philip Race
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-28 Thread Alexey Ivanov
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…

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-28 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-28 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-27 Thread Sergey Bylokhov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-26 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-26 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-26 Thread Alexey Ivanov
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,

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-25 Thread Sergey Bylokhov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-22 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-22 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-22 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-22 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-22 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-22 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-22 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-22 Thread Alexey Ivanov
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-21 Thread Sergey Bylokhov
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.

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-21 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-21 Thread Sergey Bylokhov
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,

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-21 Thread Semyon Sadetsky
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

Re: [10] Review request for 8182043: Access to Windows Large Icons

2017-09-20 Thread Sergey Bylokhov
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?

[10] Review request for 8182043: Access to Windows Large Icons

2017-09-13 Thread Semyon Sadetsky
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