Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Sergey Bylokhov]

Looks fine to me. Thanks.

> 
> Sure, updated in place.
> Thanks,
> 
> Alexander.
> On 01/09/2017 06:09 PM, Sergey Bylokhov wrote:
>> Looks fine.
>> But it seems that this field can be removed?
>> private static boolean isGtkSupported =  false;
>> After the fix for JDK-8150954 this field is not used.
>> 
>>> 
>>> Hello,
>>> 
>>> please review the fix
>>> 
>>> http://cr.openjdk.java.net/~azvegint/jdk/9/8165705/00/ 
>>> 
>>> 
>>> for the issue
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8165705 
>>> 
>>> 
>>> Gtk is now default option for taking screenshots with Robot.
>>> 
>>> -- 
>>> Thanks,
>>> Alexander.
>>> 
>> 
> 

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Alexander Zvegintsev]

Sure, updated in place.

Thanks,

Alexander.

On 01/09/2017 06:09 PM, Sergey Bylokhov wrote:

Looks fine.
But it seems that this field can be removed?
private static boolean isGtkSupported =false;
After the fix for JDK-8150954 this field is not used.



Hello,

please review the fix

http://cr.openjdk.java.net/~azvegint/jdk/9/8165705/00/ 



for the issue

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

Gtk is now default option for taking screenshots with Robot.

--
Thanks,
Alexander.

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Semyon Sadetsky]

On 11/29/2016 5:52 PM, Sergey Bylokhov wrote:


On 29.11.16 17:39, Semyon Sadetsky wrote:

On 11/28/2016 11:03 PM, Sergey Bylokhov wrote:


On 24.11.16 22:24, Semyon Sadetsky wrote:

Having an extra unspecified functionality on which application can
depends is not better, it is also slower, and during evaluation of
bugs it will be necessary to know that this can be switched at 
runtime.

I don't believe that it may affect performance even slightly.


But it will affect, and other mentioned points are still valid.

I did not get this. Can you provide those extra points?


Having an extra unspecified functionality on which application can
depends is not better, it is also slower, and during evaluation of
bugs it will be necessary to know that this can be switched at runtime.

This can be said about any functionality.

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Sergey Bylokhov]

On 29.11.16 17:39, Semyon Sadetsky wrote:

On 11/28/2016 11:03 PM, Sergey Bylokhov wrote:


On 24.11.16 22:24, Semyon Sadetsky wrote:

Having an extra unspecified functionality on which application can
depends is not better, it is also slower, and during evaluation of
bugs it will be necessary to know that this can be switched at runtime.

I don't believe that it may affect performance even slightly.


But it will affect, and other mentioned points are still valid.

I did not get this. Can you provide those extra points?


Having an extra unspecified functionality on which application can
depends is not better, it is also slower, and during evaluation of
bugs it will be necessary to know that this can be switched at runtime.

--
Best regards, Sergey.

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Semyon Sadetsky]

On 11/28/2016 11:03 PM, Sergey Bylokhov wrote:


On 24.11.16 22:24, Semyon Sadetsky wrote:

Having an extra unspecified functionality on which application can
depends is not better, it is also slower, and during evaluation of
bugs it will be necessary to know that this can be switched at runtime.

I don't believe that it may affect performance even slightly.


But it will affect, and other mentioned points are still valid.

I did not get this. Can you provide those extra points?

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Sergey Bylokhov]

On 24.11.16 22:24, Semyon Sadetsky wrote:

Having an extra unspecified functionality on which application can
depends is not better, it is also slower, and during evaluation of
bugs it will be necessary to know that this can be switched at runtime.

I don't believe that it may affect performance even slightly.


But it will affect, and other mentioned points are still valid.

--
Best regards, Sergey.

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Semyon Sadetsky]

On 24.11.2016 21:29, Sergey Bylokhov wrote:


On 24.11.16 19:20, Semyon Sadetsky wrote:

I think a user may want to switch the screenshot algorithm in runtime
and I don't see the reason why do not allow this.


There are should be an opposite vision. This is not a public api(this
property is for debug only) and we should limit its usage and possible
abuse by the application.

That is not an argument as well. Having extra functionality for free is
better than disable it unless there are disadvantages.


Having an extra unspecified functionality on which application can 
depends is not better, it is also slower, and during evaluation of 
bugs it will be necessary to know that this can be switched at runtime.

I don't believe that it may affect performance even slightly.

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Sergey Bylokhov]

On 24.11.16 19:20, Semyon Sadetsky wrote:

I think a user may want to switch the screenshot algorithm in runtime
and I don't see the reason why do not allow this.


There are should be an opposite vision. This is not a public api(this
property is for debug only) and we should limit its usage and possible
abuse by the application.

That is not an argument as well. Having extra functionality for free is
better than disable it unless there are disadvantages.


Having an extra unspecified functionality on which application can 
depends is not better, it is also slower, and during evaluation of bugs 
it will be necessary to know that this can be switched at runtime.


--
Best regards, Sergey.

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Semyon Sadetsky]

On 24.11.2016 18:56, Sergey Bylokhov wrote:

On 24.11.16 18:50, Semyon Sadetsky wrote:

Can you point to the spec where this is specified? "common usecase"
doesn't look as a satisfactory reason.


There is no such specification, but it is how we usually access the 
properties, and the current code follow it.



I think a user may want to switch the screenshot algorithm in runtime
and I don't see the reason why do not allow this.


There are should be an opposite vision. This is not a public api(this 
property is for debug only) and we should limit its usage and possible 
abuse by the application.
That is not an argument as well. Having extra functionality for free is 
better than disable it unless there are disadvantages.

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Sergey Bylokhov]

On 24.11.16 18:50, Semyon Sadetsky wrote:

Can you point to the spec where this is specified? "common usecase"
doesn't look as a satisfactory reason.


There is no such specification, but it is how we usually access the 
properties, and the current code follow it.



I think a user may want to switch the screenshot algorithm in runtime
and I don't see the reason why do not allow this.


There are should be an opposite vision. This is not a public api(this 
property is for debug only) and we should limit its usage and possible 
abuse by the application.


--
Best regards, Sergey.

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Semyon Sadetsky]

On 24.11.2016 17:54, Sergey Bylokhov wrote:

On 24.11.16 11:41, Semyon Sadetsky wrote:

On 23.11.2016 20:37, Sergey Bylokhov wrote:


On 23.11.16 18:37, Semyon Sadetsky wrote:

On 22.11.2016 02:06, Sergey Bylokhov wrote:


Hi, Semyon.
Why did you change the code to load the property each time the robot
will be created? I guess it is possible to change only the line in 
the

static block, so absent of "awt.robot.gtk" will mean use GTK.
I did this small refactoring because I find the present code too 
verbose

and poorly readable.


But probably it will be possible cleanup it further and to read this
property only once, as before the fix?

Probably. Can you provide a reason to keep this restriction?


This is common usecase, when the property read only once, and the 
value is used across all application timeframe. 
Can you point to the spec where this is specified? "common usecase" 
doesn't look as a satisfactory reason.
I think a user may want to switch the screenshot algorithm in runtime 
and I don't see the reason why do not allow this.
I am not sure but the "if" statement which had 3 lines of code does 
not look better that the old code.






On 18.11.16 14:24, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

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

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

Fix 8150954 made it possible to take screenshots from compositing
WM on
Linux without use of GTK library.

But the suggested solution is not compatible with some environments
and
also it cannot take screenshots in the full-screen mode. The 
proposed

fix suggests to use the GTK based screenshots by default and only
enable
the 8150954 logic if awt.robot.gtk system property is set to false.

The change should not limit the use of AWT robot to GTK2 compatible
applications because JDK9 supports both GTK versions.

--Semyon

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Sergey Bylokhov]

On 24.11.16 11:41, Semyon Sadetsky wrote:

On 23.11.2016 20:37, Sergey Bylokhov wrote:


On 23.11.16 18:37, Semyon Sadetsky wrote:

On 22.11.2016 02:06, Sergey Bylokhov wrote:


Hi, Semyon.
Why did you change the code to load the property each time the robot
will be created? I guess it is possible to change only the line in the
static block, so absent of "awt.robot.gtk" will mean use GTK.

I did this small refactoring because I find the present code too verbose
and poorly readable.


But probably it will be possible cleanup it further and to read this
property only once, as before the fix?

Probably. Can you provide a reason to keep this restriction?


This is common usecase, when the property read only once, and the value 
is used across all application timeframe. I am not sure but the "if" 
statement which had 3 lines of code does not look better that the old code.






On 18.11.16 14:24, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

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

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

Fix 8150954 made it possible to take screenshots from compositing
WM on
Linux without use of GTK library.

But the suggested solution is not compatible with some environments
and
also it cannot take screenshots in the full-screen mode. The proposed
fix suggests to use the GTK based screenshots by default and only
enable
the 8150954 logic if awt.robot.gtk system property is set to false.

The change should not limit the use of AWT robot to GTK2 compatible
applications because JDK9 supports both GTK versions.

--Semyon














--
Best regards, Sergey.

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Semyon Sadetsky]

On 23.11.2016 20:37, Sergey Bylokhov wrote:


On 23.11.16 18:37, Semyon Sadetsky wrote:

On 22.11.2016 02:06, Sergey Bylokhov wrote:


Hi, Semyon.
Why did you change the code to load the property each time the robot
will be created? I guess it is possible to change only the line in the
static block, so absent of "awt.robot.gtk" will mean use GTK.

I did this small refactoring because I find the present code too verbose
and poorly readable.


But probably it will be possible cleanup it further and to read this 
property only once, as before the fix?

Probably. Can you provide a reason to keep this restriction?




On 18.11.16 14:24, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

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

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

Fix 8150954 made it possible to take screenshots from compositing 
WM on

Linux without use of GTK library.

But the suggested solution is not compatible with some environments 
and

also it cannot take screenshots in the full-screen mode. The proposed
fix suggests to use the GTK based screenshots by default and only 
enable

the 8150954 logic if awt.robot.gtk system property is set to false.

The change should not limit the use of AWT robot to GTK2 compatible
applications because JDK9 supports both GTK versions.

--Semyon

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Sergey Bylokhov]

On 23.11.16 18:37, Semyon Sadetsky wrote:

On 22.11.2016 02:06, Sergey Bylokhov wrote:


Hi, Semyon.
Why did you change the code to load the property each time the robot
will be created? I guess it is possible to change only the line in the
static block, so absent of "awt.robot.gtk" will mean use GTK.

I did this small refactoring because I find the present code too verbose
and poorly readable.


But probably it will be possible cleanup it further and to read this 
property only once, as before the fix?




On 18.11.16 14:24, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

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

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

Fix 8150954 made it possible to take screenshots from compositing WM on
Linux without use of GTK library.

But the suggested solution is not compatible with some environments and
also it cannot take screenshots in the full-screen mode. The proposed
fix suggests to use the GTK based screenshots by default and only enable
the 8150954 logic if awt.robot.gtk system property is set to false.

The change should not limit the use of AWT robot to GTK2 compatible
applications because JDK9 supports both GTK versions.

--Semyon









--
Best regards, Sergey.

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Semyon Sadetsky]

On 22.11.2016 02:06, Sergey Bylokhov wrote:


Hi, Semyon.
Why did you change the code to load the property each time the robot 
will be created? I guess it is possible to change only the line in the 
static block, so absent of "awt.robot.gtk" will mean use GTK.
I did this small refactoring because I find the present code too verbose 
and poorly readable.


On 18.11.16 14:24, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

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

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

Fix 8150954 made it possible to take screenshots from compositing WM on
Linux without use of GTK library.

But the suggested solution is not compatible with some environments and
also it cannot take screenshots in the full-screen mode. The proposed
fix suggests to use the GTK based screenshots by default and only enable
the 8150954 logic if awt.robot.gtk system property is set to false.

The change should not limit the use of AWT robot to GTK2 compatible
applications because JDK9 supports both GTK versions.

--Semyon

Re: [9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Sergey Bylokhov]

Hi, Semyon.
Why did you change the code to load the property each time the robot 
will be created? I guess it is possible to change only the line in the 
static block, so absent of "awt.robot.gtk" will mean use GTK.


On 18.11.16 14:24, Semyon Sadetsky wrote:

Hello,

Please review fix for JDK9:

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

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

Fix 8150954 made it possible to take screenshots from compositing WM on
Linux without use of GTK library.

But the suggested solution is not compatible with some environments and
also it cannot take screenshots in the full-screen mode. The proposed
fix suggests to use the GTK based screenshots by default and only enable
the 8150954 logic if awt.robot.gtk system property is set to false.

The change should not limit the use of AWT robot to GTK2 compatible
applications because JDK9 supports both GTK versions.

--Semyon




--
Best regards, Sergey.

[9] Review request for 8165705: Robot.createScreenCapture produces black screenshot on Oracle Linux 7.1

[Semyon Sadetsky]

Hello,

Please review fix for JDK9:

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

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

Fix 8150954 made it possible to take screenshots from compositing WM on 
Linux without use of GTK library.


But the suggested solution is not compatible with some environments and 
also it cannot take screenshots in the full-screen mode. The proposed 
fix suggests to use the GTK based screenshots by default and only enable 
the 8150954 logic if awt.robot.gtk system property is set to false.


The change should not limit the use of AWT robot to GTK2 compatible 
applications because JDK9 supports both GTK versions.


--Semyon