Re: [10] Review Request: JDK-8145795 : [PIT] java/awt/Window/ScreenLocation/ScreenLocationTest.java fails (can assign Integer.MAX_VALUE to Window dimensions)

[Sergey Bylokhov]

On 17/10/2017 04:48, Pankaj Bansal wrote:

I can see that XDragSourceContextPeer.java also contains ScaleUp and ScaleDown 
functions, but I don't see them being called from anywhere. So have not made 
changes in the same.


Looks like XDragSourceContextPeer.scaleUp() is unused, but scaleDown() 
is used in a few places.




Regards,
Pankaj Bansal

-Original Message-
From: Sergey Bylokhov
Sent: Tuesday, October 17, 2017 4:03 AM
To: Pankaj Bansal; awt-dev@openjdk.java.net
Subject: Re:  [10] Review Request: JDK-8145795 : [PIT] 
java/awt/Window/ScreenLocation/ScreenLocationTest.java fails (can assign 
Integer.MAX_VALUE to Window dimensions)

Hi, Pankaj.
Can you please check that the same changes needs to be applied to the
XToolkit:

X11GraphicsConfig.scaleUp()
X11GraphicsConfig.scaleDown()
XlibUtil.scaleDown()

On 09/10/2017 00:53, Pankaj Bansal wrote:

Hi,

Please review the fix for JDK 10.

Bug:

https://bugs.openjdk.java.net/browse/JDK-8145795

Webrev:

http://cr.openjdk.java.net/~aghaisas/pankaj/8145795/webrev.00/

Issue:

The awt window size is capped at a max value, but with hidpi support,
an regression was introduced because of which the window size is wrong
and even INT_MAX window size is possible.

Fix:

The issue is due to the int overflow. There was no check to keep the
window size between int min and max values. Due to which, the windows
size was wrong. Added int limit checks.

Regards,

Pankaj Bansal




--
Best regards, Sergey.




--
Best regards, Sergey.

Re: [10] Review request for 8148619: Select the closest resolution variant in BaseMultiResolutionImage

[Phil Race]

This class which is concrete - not abstract - would have been better 
called "BasicMultiResolutionImage"


I do think I have issues with the documentation on this class and some 
aspects of
its behaviour that should be documented and implemented, but I don't 
think we should

try to make it something it was not intended to be.

I'm not going to  try to solve it all in this email but here are some of 
my thoughts.


- There is nothing inherently wrong with the idea that the application 
needs to provide a
sorted list of images in increasing size so long as it is documented as 
such.

If a smarter or different algorithm is required, the developer can write one
subclassing AbstractMultiResolutionImage

- I think these words need examination and clarification :
"For best effect the array of images should be sorted with each image being
both wider and taller than the previous image."

What does "for best effect mean" ? Perhaps it means it won't throw any
exceptions and will still paint, but the image selection algorithm is 
designed

with that assumption.

Let's instead say
- The algorithm used by this class is a simple linear search of the 
array to find

the first image that meets its criteria

- The first image that is both at least as tall and at least as wide as 
the target area

is selected.

- In the event that no image meets this, the final image will be selected

- It is the application's responsibility to ensure they are sorted 
accordingly.


-  The criteria are based on the assumption that in most cases, scaling
   down an image will produce better results than scaling up an image.

- If the provided images do not share the same aspect ratio,
  as might be the case if the application expects them to be used under
  non-uniform transforms, this selection algorithm may not be appropriate.

- There is no notion of selection by closeness.
  If the application does not want a very large image scaled down,
  by a large factor, it should provide additional images, or not use 
this class.



Since I don't think you can adapt this class to solve all these problems 
without

too many changes, it is better to stick to what it claims.

-phil.


On 09/08/2017 11:48 AM, Semyon Sadetsky wrote:

On 09/08/2017 11:29 AM, Sergey Bylokhov wrote:

This is not a mistake, it was done intentionally because this is a 
small/basic/simple/fast implementation.
This is important criteria but it is not enough for public API. In 
internal classes the internal implementation logic could be shared 
between the provider class and its client, but this obviously violates 
the encapsulation concept.  But this is not suitable for a public API. 
Look at the way how this API is designed: To have 
BaseMultiResolutionImage woking correctly the client must provide 
BaseMultiResolutionImage internal data structure prepared by a certain 
rule in constructor argument. This API design looks inappropriate to 
common standard without any significant reason.


--Semyon


On 9/8/17 11:06, Semyon Sadetsky wrote:

On 09/07/2017 01:41 PM, Sergey Bylokhov wrote:

Hi, Semyon.

The responsibility for sorting of an array was intentionally moved 
to the user, because the getResolutionVariant method () is called 
in each draw of the image. For this purpose in documentation for a 
class and in a documentation for constructors it was specified that 
the array shall be sorted. It is the reason why the bug of 
JDK-8147849 was closed.
This seems to me a mistake in the API design which this update is 
trying to fix. Since we opened this API for everybody's use the base 
implementation of the MultiResolutionImage  should be covering its 
most frequent and general usage. And in this general usage the 
appropriate resolution among the provided image variants should be 
chosen at the time the MR image is drawn. This is the logic we use 
internally in JDK to draw icons and other UI stuff on the HiDPI 
display and it is in demand for user applications in most cases.
When a predefined order of the image variant selection is required 
then getResolutionVariant() may be overridden to achieve that. But 
obviously this is a rare case and it shouldn't be a basic 
implementation.


--Semyon


RFE which you try to fix cover another use-case:
If the user will use the sorted array  [@1 ,@20] then we should 
select @1 if "-Dsun.java2d.uiScale=2" is used but not @20


On 9/6/17 19:31, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK10:

bug: https://bugs.openjdk.java.net/browse/JDK-8148619

webrev: http://cr.openjdk.java.net/~ssadetsky/8148619/webrev.00/

The algorithm selecting the best resolution variant of the 
BaseMultiResolutionImage was updated to be insensitive to the 
order of image variants in the initially provided array.


The BaseMultiResolutionImage specification was updated 
correspondingly.


--Semyon

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

[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 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

[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 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 8074824: Resolve disabled warnings for libawt_xawt

[Semyon Sadetsky]

Hi Ajit,

Thank you for the information. I ran SwingSet under GTK2. No issues found.

--Semyon

On 10/16/2017 09:41 PM, Ajit Ghaisas wrote:

Hi Semyon,

 I had attempted this fix some time back and had run into some regression 
for SwingSet2.
 There was a crash while using SwingSet2 with GTK look-and-feel with GTK-2.
 I do not remember the exact steps, but just playing with SwingSet2 had 
produced this crash.

 Can you please check your fix does not cause such a regression?

Regards,
Ajit

-Original Message-
From: Sergey Bylokhov
Sent: Tuesday, October 17, 2017 5:15 AM
To: Semyon Sadetsky; Phil Race; awt-dev@openjdk.java.net
Subject: Re:  [10] Review request for 8074824: Resolve disabled 
warnings for libawt_xawt

Hi, Semyon.
A few notes:> I ran OGL tests and found a typo. Below the update webrev> 
http://cr.openjdk.java.net/~ssadetsky/8074824/webrev.01/

   - What is a typo? It seems the v0 and v1 are identical.
   - The new "IS_SAFE_SIZE_T" macro returns different result than the old one for 
"0"
   - What is the reason to inline some of the shaders in OGLPaints.c and leave as-is others? For example 
"noCycleCode" was inlined and "texCoordCalcCode" still stored in the "static const char 
*". It seems that the "static const" style is more readable.


--
Best regards, Sergey.

Re: [10] Review request for 8074824: Resolve disabled warnings for libawt_xawt

[Semyon Sadetsky]

Hi Sergey,

On 10/16/2017 04:45 PM, Sergey Bylokhov wrote:

Hi, Semyon.
A few notes:> I ran OGL tests and found a typo. Below the update 
webrev> http://cr.openjdk.java.net/~ssadetsky/8074824/webrev.01/


 - What is a typo? It seems the v0 and v1 are identical.

It's in line 516 of the OGLPaints.c.
 - The new "IS_SAFE_SIZE_T" macro returns different result than the 
old one for "0"

O.K.
#define IS_SAFE_SIZE_T(x) (((x) + 1) > 0 && \
    (x == 0 || (unsigned long long)(x) - 1u < 
SIZE_MAX))


I will add this "0" case upon push if you don't mind.
 - What is the reason to inline some of the shaders in OGLPaints.c and 
leave as-is others? For example "noCycleCode" was inlined and 
"texCoordCalcCode" still stored in the "static const char *". It seems 
that the "static const" style is more readable.
It is impossible to have printf() format other then literal string in 
gcc without a warning. There is no way to avoid it.


--Semyon

Re: [10] Review Request: JDK-8145795 : [PIT] java/awt/Window/ScreenLocation/ScreenLocationTest.java fails (can assign Integer.MAX_VALUE to Window dimensions)

[Pankaj Bansal]

Hi Sergey,

Yes, similar changes have to be made in XToolkit also. What happens in XToolkit 
is that after the int overflow, when garbage width and height is passed to 
XCreateWindow, it handles it properly at its own end. So the test passes 
without any fix also. But the into overflow should not happen in the first 
place.

I have made changes in X11GraphicsConfig and XlibUtil Scaling functions and 
incorporated in the webrev. 
Webrev: 
http://cr.openjdk.java.net/~mhalder/pankaj/8145795/webrev.01/

I can see that XDragSourceContextPeer.java also contains ScaleUp and ScaleDown 
functions, but I don't see them being called from anywhere. So have not made 
changes in the same.

Regards,
Pankaj Bansal

-Original Message-
From: Sergey Bylokhov 
Sent: Tuesday, October 17, 2017 4:03 AM
To: Pankaj Bansal; awt-dev@openjdk.java.net
Subject: Re:  [10] Review Request: JDK-8145795 : [PIT] 
java/awt/Window/ScreenLocation/ScreenLocationTest.java fails (can assign 
Integer.MAX_VALUE to Window dimensions)

Hi, Pankaj.
Can you please check that the same changes needs to be applied to the
XToolkit:

X11GraphicsConfig.scaleUp()
X11GraphicsConfig.scaleDown()
XlibUtil.scaleDown()

On 09/10/2017 00:53, Pankaj Bansal wrote:
> Hi,
> 
> Please review the fix for JDK 10.
> 
> Bug:
> 
> https://bugs.openjdk.java.net/browse/JDK-8145795
> 
> Webrev:
> 
> http://cr.openjdk.java.net/~aghaisas/pankaj/8145795/webrev.00/
> 
> Issue:
> 
> The awt window size is capped at a max value, but with hidpi support, 
> an regression was introduced because of which the window size is wrong 
> and even INT_MAX window size is possible.
> 
> Fix:
> 
> The issue is due to the int overflow. There was no check to keep the 
> window size between int min and max values. Due to which, the windows 
> size was wrong. Added int limit checks.
> 
> Regards,
> 
> Pankaj Bansal
> 


--
Best regards, Sergey.

Re: [10] Review request for JDK-8163265: [macosx] numpad 0 instead of VK_0

[Manajit Halder]

Thank you Sergey and Alex,

I will update @bugid in the test before push. 

Regards,
Manajit

> On 17-Oct-2017, at 2:37 PM, Alexander Zvegintsev 
>  wrote:
> 
> +1
> 
> Thanks,
> Alexander.
> 
> On 17/10/2017 03:12, Sergey Bylokhov wrote:
>> Looks fine.
>> Please update the @bugid in the test before the push.
>> 
>> On 16/10/2017 06:38, Manajit Halder wrote:
>>> Hi All,
>>> 
>>> Kindly review the fix for JDK10.
>>> 
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8163265
>>> 
>>> Webrev:
>>> http://cr.openjdk.java.net/~mhalder/8163265/webrev.00/
>>> 
>>> Issue:
>>> Wrong event code received for numbers 0 to 9 in case if any of the arrow 
>>> keys (left, right, up and down) are pressed before pressing any of the 
>>> number keys (0 to 9). Instead of number key event codes, NUMPAD number key 
>>> event are received and the test fails. But if the arrow keys are not 
>>> pressed before pressing the number keys  0 to 9 then correct key codes are 
>>> received for the numbers and the test passes.
>>> 
>>> Cause:
>>> NSNumericPadKeyMask is used in the code to find out whether the number key 
>>> pressed is number or NUMPAD number. But NSNumericPadKeyMask is also set if 
>>> any of the arrow keys are pressed (NSUpArrowFunctionKey, 
>>> NSDownArrowFunctionKey, NSLeftArrowFunctionKey, and 
>>> NSRightArrowFunctionKey).
>>> 
>>> Fix:
>>> To distinguish between the number and NUMPAD keys the key values are also 
>>> checked because the key values are different for numbers and NUMPAD numbers.
>>> 
>>> Regards,
>>> Manajit
>> 
>> 
> 

Re: [10] Review request for JDK-8163265: [macosx] numpad 0 instead of VK_0

[Alexander Zvegintsev]

+1

Thanks,
Alexander.

On 17/10/2017 03:12, Sergey Bylokhov wrote:

Looks fine.
Please update the @bugid in the test before the push.

On 16/10/2017 06:38, Manajit Halder wrote:

Hi All,

Kindly review the fix for JDK10.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8163265

Webrev:
http://cr.openjdk.java.net/~mhalder/8163265/webrev.00/

Issue:
Wrong event code received for numbers 0 to 9 in case if any of the 
arrow keys (left, right, up and down) are pressed before pressing any 
of the number keys (0 to 9). Instead of number key event codes, 
NUMPAD number key event are received and the test fails. But if the 
arrow keys are not pressed before pressing the number keys  0 to 9 
then correct key codes are received for the numbers and the test passes.


Cause:
NSNumericPadKeyMask is used in the code to find out whether the 
number key pressed is number or NUMPAD number. But 
NSNumericPadKeyMask is also set if any of the arrow keys are pressed 
(NSUpArrowFunctionKey, NSDownArrowFunctionKey, 
NSLeftArrowFunctionKey, and NSRightArrowFunctionKey).


Fix:
To distinguish between the number and NUMPAD keys the key values are 
also checked because the key values are different for numbers and 
NUMPAD numbers.


Regards,
Manajit