Re: [PATCH] 8160325: Provide a public API to obtain the FXCanvas for an embedded scene.

[Alexander Zvegintsev]

Looks fine to me too.


On 8/16/16 1:45 AM, Kevin Rushforth wrote:
The 2016-08-12 revision looks fine to me, except for a missing space 
as noted in JBS (no need for a new patch if that is the only issue found).


While we wait for approval from the JDK 9 release team, we need 
another reviewer for this.


Alexander Z: can you take a look?

-- Kevin


Alexander Nyssen wrote:

Hi Kevin,

attached please find a revised patch that contains the corrections 
you requested. The patch is now applicable to the new module 
structure (modues with 'javafx.' prefix).


Regards,
Alexander





Am 12.08.2016 um 00:00 schrieb Kevin Rushforth 
>:




Alexander Nyssen wrote:

Hi Kevin,

thanks for your feedback. Please fin my comments inline.

   

Am 09.08.2016 um 03:10 schrieb Kevin Rushforth:

I uploaded the patch, reviewed it, and provided comments in the bug report. The 
short version is:

* The new API looks good

* There is a missing '@since 9' in the javadoc comments along with a few typos 
/ style issues
 

I will take care of it. Is there a style guide?
   


Not a current one. The ones I pointed out in the JBS issue were 
either grammatical or capitalization (other than the '@since' which 
is required for all new API).



* Rather than using reflection and setAccessible in the implementation, please 
add a public getHostContainer method to EmbeddedWindow (since it is an internal 
method, there is no concern with doing that -- it isn't API).
 

Such a getHost() method was already introduced to EmbeddedWindow as part of the 
patch (to obtain the HostContainer from it). The reflection code within 
getFXCanvas() is used to access the enclosing instance (i.e. the FXCanvas) of 
the HostContainer. We could only circumvent this by introducing a constructor 
to HostContainer and by passing in the enclosing FXCanvas instance explicitly 
(so we can query it via an explicit getter, which would have to be introduced 
in addition). Would you prefer that?
   


Ah, I see what you are doing now. There is an easier way, then. Just 
add a default (package) scope 'fxCanvas' variable in the inner class 
initialized to 'FXCanvas.this' and you will be able to access it 
directly, since an instance of an inner class can always get access 
to the instance of the enclosing class.


private class HostContainer implements HostInterface {
final FXCanvas fxCanvas = FXCanvas.this;
...

I updated the bug report with this and the other style issues. I 
haven't tested it yet, but other than the listed issues it looks 
very close to being done.


-- Kevin


   

Additionally, I requested JDK 9 release team approval for this. The approval 
process can proceed in parallel with your addressing the issues I raised.

— Kevin
 

Regards,
Alexander

   

Alexander Nyssen wrote:
 

Hi Kevin,

attached please find a revised patch. My comments are inlined.

   

Am 28.07.2016 um 18:03 schrieb Kevin Rushforth >:

Hi

Alexander Nyssen wrote:
 

Hi,

I have added my comments below:


   

Am 28.07.2016 um 17:22 schrieb Kevin Rushforth >:

I got the attachment, since Alexander also CCed me directly. I will attach it 
shortly.
 

Thanks!
   

Done.

 

I do have two comments on this:

1) We are past Feature Freeze, so all Enhancements need formal JDK 9 R-team 
approval [1][2]. In this case, the justification can be internal API that is no 
longer accessible in JDK 9 due to Jigsaw (I would be very reluctant to consider 
any other Enhancement request this late in the process), but I will need to 
look at it and then take it through the approval process, provided that I feel 
it is in scope.
 

I was not aware about this, but I would of course appreciate if it could be 
included (due to Jigsaw). Thanks for considering it at least.
   

I'll take a closer look tomorrow or Monday (no more time today). At first 
glance it seems like something reasonable to take forward.
 

That sounds promising. Thanks!

   

2) Some of the changes you list seem unrelated to this enhancement and are 
better done as separate issues (e.g., the rework of the SWTCursorsTest). Also, 
I am unconvinced of the need to force GTK 2; in fact it seems at odds with the 
work we have done with JEP 283 [3].
 

Well, the test case refactoring is somehow related, as I introduced the common 
SWT rule while introducing the second SWT test. However, I could provide it as 
a separate contribution if that was wished (and a JIRA issue was provided), but 
the rest of this contribution of course requires it as a prerequisite. If this 
enhancement could not be included in JDK 9, I would have 

Re: [PATCH] 8160325: Provide a public API to obtain the FXCanvas for an embedded scene.

[Kevin Rushforth]

The 2016-08-12 revision looks fine to me, except for a missing space as 
noted in JBS (no need for a new patch if that is the only issue found).


While we wait for approval from the JDK 9 release team, we need another 
reviewer for this.


Alexander Z: can you take a look?

-- Kevin


Alexander Nyssen wrote:

Hi Kevin,

attached please find a revised patch that contains the corrections you 
requested. The patch is now applicable to the new module structure 
(modues with 'javafx.' prefix).


Regards,
Alexander





Am 12.08.2016 um 00:00 schrieb Kevin Rushforth 
>:




Alexander Nyssen wrote:

Hi Kevin,

thanks for your feedback. Please fin my comments inline.

  

Am 09.08.2016 um 03:10 schrieb Kevin Rushforth :

I uploaded the patch, reviewed it, and provided comments in the bug report. The 
short version is:

* The new API looks good

* There is a missing '@since 9' in the javadoc comments along with a few typos 
/ style issues


I will take care of it. Is there a style guide?
  


Not a current one. The ones I pointed out in the JBS issue were 
either grammatical or capitalization (other than the '@since' which 
is required for all new API).



* Rather than using reflection and setAccessible in the implementation, please 
add a public getHostContainer method to EmbeddedWindow (since it is an internal 
method, there is no concern with doing that -- it isn't API).


Such a getHost() method was already introduced to EmbeddedWindow as part of the 
patch (to obtain the HostContainer from it). The reflection code within 
getFXCanvas() is used to access the enclosing instance (i.e. the FXCanvas) of 
the HostContainer. We could only circumvent this by introducing a constructor 
to HostContainer and by passing in the enclosing FXCanvas instance explicitly 
(so we can query it via an explicit getter, which would have to be introduced 
in addition). Would you prefer that?
  


Ah, I see what you are doing now. There is an easier way, then. Just 
add a default (package) scope 'fxCanvas' variable in the inner class 
initialized to 'FXCanvas.this' and you will be able to access it 
directly, since an instance of an inner class can always get access 
to the instance of the enclosing class.


private class HostContainer implements HostInterface {
final FXCanvas fxCanvas = FXCanvas.this;
...

I updated the bug report with this and the other style issues. I 
haven't tested it yet, but other than the listed issues it looks very 
close to being done.


-- Kevin


  

Additionally, I requested JDK 9 release team approval for this. The approval 
process can proceed in parallel with your addressing the issues I raised.

— Kevin


Regards,
Alexander

  

Alexander Nyssen wrote:


Hi Kevin,

attached please find a revised patch. My comments are inlined.

  

Am 28.07.2016 um 18:03 schrieb Kevin Rushforth >:

Hi

Alexander Nyssen wrote:


Hi,

I have added my comments below:


  

Am 28.07.2016 um 17:22 schrieb Kevin Rushforth >:

I got the attachment, since Alexander also CCed me directly. I will attach it 
shortly.


Thanks!
  

Done.



I do have two comments on this:

1) We are past Feature Freeze, so all Enhancements need formal JDK 9 R-team 
approval [1][2]. In this case, the justification can be internal API that is no 
longer accessible in JDK 9 due to Jigsaw (I would be very reluctant to consider 
any other Enhancement request this late in the process), but I will need to 
look at it and then take it through the approval process, provided that I feel 
it is in scope.


I was not aware about this, but I would of course appreciate if it could be 
included (due to Jigsaw). Thanks for considering it at least.
  

I'll take a closer look tomorrow or Monday (no more time today). At first 
glance it seems like something reasonable to take forward.


That sounds promising. Thanks!

  

2) Some of the changes you list seem unrelated to this enhancement and are 
better done as separate issues (e.g., the rework of the SWTCursorsTest). Also, 
I am unconvinced of the need to force GTK 2; in fact it seems at odds with the 
work we have done with JEP 283 [3].


Well, the test case refactoring is somehow related, as I introduced the common 
SWT rule while introducing the second SWT test. However, I could provide it as 
a separate contribution if that was wished (and a JIRA issue was provided), but 
the rest of this contribution of course requires it as a prerequisite. If this 
enhancement could not be included in JDK 9, I would have to provide it as a 
separate contribution, as I would have to re-introduce 

Re: [PATCH] 8160325: Provide a public API to obtain the FXCanvas for an embedded scene.

[Alexander Nyssen]

Hi Kevin,

attached please find a revised patch that contains the corrections you 
requested. The patch is now applicable to the new module structure (modues with 
'javafx.' prefix).

Regards,
Alexander


> Am 12.08.2016 um 00:00 schrieb Kevin Rushforth :
> 
> 
> 
> Alexander Nyssen wrote:
>> 
>> Hi Kevin,
>> 
>> thanks for your feedback. Please fin my comments inline.
>> 
>>   
>>> Am 09.08.2016 um 03:10 schrieb Kevin Rushforth  
>>> :
>>> 
>>> I uploaded the patch, reviewed it, and provided comments in the bug report. 
>>> The short version is:
>>> 
>>> * The new API looks good
>>> 
>>> * There is a missing '@since 9' in the javadoc comments along with a few 
>>> typos / style issues
>>> 
>> I will take care of it. Is there a style guide?
>>   
> 
> Not a current one. The ones I pointed out in the JBS issue were either 
> grammatical or capitalization (other than the '@since' which is required for 
> all new API).
> 
>>> * Rather than using reflection and setAccessible in the implementation, 
>>> please add a public getHostContainer method to EmbeddedWindow (since it is 
>>> an internal method, there is no concern with doing that -- it isn't API).
>>> 
>> Such a getHost() method was already introduced to EmbeddedWindow as part of 
>> the patch (to obtain the HostContainer from it). The reflection code within 
>> getFXCanvas() is used to access the enclosing instance (i.e. the FXCanvas) 
>> of the HostContainer. We could only circumvent this by introducing a 
>> constructor to HostContainer and by passing in the enclosing FXCanvas 
>> instance explicitly (so we can query it via an explicit getter, which would 
>> have to be introduced in addition). Would you prefer that?
>>   
> 
> Ah, I see what you are doing now. There is an easier way, then. Just add a 
> default (package) scope 'fxCanvas' variable in the inner class initialized to 
> 'FXCanvas.this' and you will be able to access it directly, since an instance 
> of an inner class can always get access to the instance of the enclosing 
> class.
> 
> private class HostContainer implements HostInterface {
> final FXCanvas fxCanvas = FXCanvas.this;
> ...
> 
> I updated the bug report with this and the other style issues. I haven't 
> tested it yet, but other than the listed issues it looks very close to being 
> done.
> 
> -- Kevin
> 
> 
>>   
>>> Additionally, I requested JDK 9 release team approval for this. The 
>>> approval process can proceed in parallel with your addressing the issues I 
>>> raised.
>>> 
>>> — Kevin
>>> 
>> Regards,
>> Alexander
>> 
>>   
>>> Alexander Nyssen wrote:
>>> 
 Hi Kevin,
 
 attached please find a revised patch. My comments are inlined.
 
   
> Am 28.07.2016 um 18:03 schrieb Kevin Rushforth 
>  
>  >:
> 
> Hi
> 
> Alexander Nyssen wrote:
> 
>> Hi,
>> 
>> I have added my comments below:
>> 
>> 
>>   
>>> Am 28.07.2016 um 17:22 schrieb Kevin Rushforth 
>>>  
>>>  
>>> >:
>>> 
>>> I got the attachment, since Alexander also CCed me directly. I will 
>>> attach it shortly.
>>> 
>> Thanks!
>>   
> Done.
> 
> 
>>> I do have two comments on this:
>>> 
>>> 1) We are past Feature Freeze, so all Enhancements need formal JDK 9 
>>> R-team approval [1][2]. In this case, the justification can be internal 
>>> API that is no longer accessible in JDK 9 due to Jigsaw (I would be 
>>> very reluctant to consider any other Enhancement request this late in 
>>> the process), but I will need to look at it and then take it through 
>>> the approval process, provided that I feel it is in scope.
>>> 
>> I was not aware about this, but I would of course appreciate if it could 
>> be included (due to Jigsaw). Thanks for considering it at least.
>>   
> I'll take a closer look tomorrow or Monday (no more time today). At first 
> glance it seems like something reasonable to take forward.
> 
 That sounds promising. Thanks!
 
   
>>> 2) Some of the changes you list seem unrelated to this enhancement and 
>>> are better done as separate issues (e.g., the rework of the 
>>> SWTCursorsTest). Also, I am unconvinced of the need to force GTK 2; in 
>>> fact it seems at odds with the work we have done with JEP 283 [3].
>>> 
>> Well, the test case refactoring is somehow related, as I introduced the 
>> common SWT rule while introducing the second SWT test. 

Re: [PATCH] 8160325: Provide a public API to obtain the FXCanvas for an embedded scene.

[Kevin Rushforth]

Alexander Nyssen wrote:

Hi Kevin,

thanks for your feedback. Please fin my comments inline.

  

Am 09.08.2016 um 03:10 schrieb Kevin Rushforth :

I uploaded the patch, reviewed it, and provided comments in the bug report. The 
short version is:

* The new API looks good

* There is a missing '@since 9' in the javadoc comments along with a few typos 
/ style issues



I will take care of it. Is there a style guide?
  


Not a current one. The ones I pointed out in the JBS issue were either 
grammatical or capitalization (other than the '@since' which is required 
for all new API).



* Rather than using reflection and setAccessible in the implementation, please 
add a public getHostContainer method to EmbeddedWindow (since it is an internal 
method, there is no concern with doing that -- it isn't API).



Such a getHost() method was already introduced to EmbeddedWindow as part of the 
patch (to obtain the HostContainer from it). The reflection code within 
getFXCanvas() is used to access the enclosing instance (i.e. the FXCanvas) of 
the HostContainer. We could only circumvent this by introducing a constructor 
to HostContainer and by passing in the enclosing FXCanvas instance explicitly 
(so we can query it via an explicit getter, which would have to be introduced 
in addition). Would you prefer that?
  


Ah, I see what you are doing now. There is an easier way, then. Just add 
a default (package) scope 'fxCanvas' variable in the inner class 
initialized to 'FXCanvas.this' and you will be able to access it 
directly, since an instance of an inner class can always get access to 
the instance of the enclosing class.


   private class HostContainer implements HostInterface {
   final FXCanvas fxCanvas = FXCanvas.this;
   ...

I updated the bug report with this and the other style issues. I haven't 
tested it yet, but other than the listed issues it looks very close to 
being done.


-- Kevin
  

  

Additionally, I requested JDK 9 release team approval for this. The approval 
process can proceed in parallel with your addressing the issues I raised.

— Kevin



Regards,
Alexander

  

Alexander Nyssen wrote:


Hi Kevin,

attached please find a revised patch. My comments are inlined.

  

Am 28.07.2016 um 18:03 schrieb Kevin Rushforth >:

Hi

Alexander Nyssen wrote:


Hi,

I have added my comments below:


  

Am 28.07.2016 um 17:22 schrieb Kevin Rushforth >:

I got the attachment, since Alexander also CCed me directly. I will attach it 
shortly.


Thanks!
  

Done.



I do have two comments on this:

1) We are past Feature Freeze, so all Enhancements need formal JDK 9 R-team 
approval [1][2]. In this case, the justification can be internal API that is no 
longer accessible in JDK 9 due to Jigsaw (I would be very reluctant to consider 
any other Enhancement request this late in the process), but I will need to 
look at it and then take it through the approval process, provided that I feel 
it is in scope.


I was not aware about this, but I would of course appreciate if it could be 
included (due to Jigsaw). Thanks for considering it at least.
  

I'll take a closer look tomorrow or Monday (no more time today). At first 
glance it seems like something reasonable to take forward.


That sounds promising. Thanks!

  

2) Some of the changes you list seem unrelated to this enhancement and are 
better done as separate issues (e.g., the rework of the SWTCursorsTest). Also, 
I am unconvinced of the need to force GTK 2; in fact it seems at odds with the 
work we have done with JEP 283 [3].


Well, the test case refactoring is somehow related, as I introduced the common 
SWT rule while introducing the second SWT test. However, I could provide it as 
a separate contribution if that was wished (and a JIRA issue was provided), but 
the rest of this contribution of course requires it as a prerequisite. If this 
enhancement could not be included in JDK 9, I would have to provide it as a 
separate contribution, as I would have to re-introduce FXCanvasTest in other 
succeeding bugfix contributions (JDK-8143596, JDK-8143596).
  

I see. I did take a quick look at this and the test changes seem fine as part 
of this. I see you created the new test with 'hg cp' (or similar) which records 
it as a copy of the SWTCursorsTest.java file, which given the number of changes 
is not needed (and not really useful), but that's easy to fix.


Done (I copied it within IntelliJ and the IDE seems to have applied hg copy).

  

There are several white space changes in FXCanvas.java that should be reverted. 
Our policy is that we do not make unrelated changes, including white space 
changes, in portions of a file that aren't otherwise modified by a 

Re: [PATCH] 8160325: Provide a public API to obtain the FXCanvas for an embedded scene.

[Alexander Nyssen]

Hi Kevin,

thanks for your feedback. Please fin my comments inline.

> Am 09.08.2016 um 03:10 schrieb Kevin Rushforth :
> 
> I uploaded the patch, reviewed it, and provided comments in the bug report. 
> The short version is:
> 
> * The new API looks good
> 
> * There is a missing '@since 9' in the javadoc comments along with a few 
> typos / style issues

I will take care of it. Is there a style guide?

> 
> * Rather than using reflection and setAccessible in the implementation, 
> please add a public getHostContainer method to EmbeddedWindow (since it is an 
> internal method, there is no concern with doing that -- it isn't API).

Such a getHost() method was already introduced to EmbeddedWindow as part of the 
patch (to obtain the HostContainer from it). The reflection code within 
getFXCanvas() is used to access the enclosing instance (i.e. the FXCanvas) of 
the HostContainer. We could only circumvent this by introducing a constructor 
to HostContainer and by passing in the enclosing FXCanvas instance explicitly 
(so we can query it via an explicit getter, which would have to be introduced 
in addition). Would you prefer that?

> 
> Additionally, I requested JDK 9 release team approval for this. The approval 
> process can proceed in parallel with your addressing the issues I raised.
> 
> — Kevin

Regards,
Alexander

> 
> 
> Alexander Nyssen wrote:
>> Hi Kevin,
>> 
>> attached please find a revised patch. My comments are inlined.
>> 
>>> Am 28.07.2016 um 18:03 schrieb Kevin Rushforth >> >:
>>> 
>>> Hi
>>> 
>>> Alexander Nyssen wrote:
 Hi,
 
 I have added my comments below:
 
 
> Am 28.07.2016 um 17:22 schrieb Kevin Rushforth 
> >:
> 
> I got the attachment, since Alexander also CCed me directly. I will 
> attach it shortly.
 
 Thanks!
>>> 
>>> Done.
>>> 
>> 
> I do have two comments on this:
> 
> 1) We are past Feature Freeze, so all Enhancements need formal JDK 9 
> R-team approval [1][2]. In this case, the justification can be internal 
> API that is no longer accessible in JDK 9 due to Jigsaw (I would be very 
> reluctant to consider any other Enhancement request this late in the 
> process), but I will need to look at it and then take it through the 
> approval process, provided that I feel it is in scope.
 
 I was not aware about this, but I would of course appreciate if it could 
 be included (due to Jigsaw). Thanks for considering it at least.
>>> 
>>> I'll take a closer look tomorrow or Monday (no more time today). At first 
>>> glance it seems like something reasonable to take forward.
>> 
>> That sounds promising. Thanks!
>> 
>>> 
> 2) Some of the changes you list seem unrelated to this enhancement and 
> are better done as separate issues (e.g., the rework of the 
> SWTCursorsTest). Also, I am unconvinced of the need to force GTK 2; in 
> fact it seems at odds with the work we have done with JEP 283 [3].
 
 Well, the test case refactoring is somehow related, as I introduced the 
 common SWT rule while introducing the second SWT test. However, I could 
 provide it as a separate contribution if that was wished (and a JIRA issue 
 was provided), but the rest of this contribution of course requires it as 
 a prerequisite. If this enhancement could not be included in JDK 9, I 
 would have to provide it as a separate contribution, as I would have to 
 re-introduce FXCanvasTest in other succeeding bugfix contributions 
 (JDK-8143596, JDK-8143596).
>>> 
>>> I see. I did take a quick look at this and the test changes seem fine as 
>>> part of this. I see you created the new test with 'hg cp' (or similar) 
>>> which records it as a copy of the SWTCursorsTest.java file, which given the 
>>> number of changes is not needed (and not really useful), but that's easy to 
>>> fix.
>> 
>> Done (I copied it within IntelliJ and the IDE seems to have applied hg copy).
>> 
>>> 
>>> There are several white space changes in FXCanvas.java that should be 
>>> reverted. Our policy is that we do not make unrelated changes, including 
>>> white space changes, in portions of a file that aren't otherwise modified 
>>> by a patch.
>> 
>> Done (I used the IntelliJ formatter). 
>>> 
 The GTK2 flag I introduced just affects SWT. As the swt library that is 
 bundled is rather old (3.7.2) that seemed to be safer (we have observed 
 quite a few problems when running SWT on GTK3). We can of course remove it 
 if tests are not affected by it.
>>> 
>>> We don't actually bundle swt itself, although we do download an old copy to 
>>> link against, and to run tests against. In any case, given that our minimum 
>>> Linux platform for JDK 9 is Ubuntu 16.04, it might not have GTK2 installed 
>>> by default. Please revert this 

Re: [PATCH] 8160325: Provide a public API to obtain the FXCanvas for an embedded scene.

[Kevin Rushforth]

I uploaded the patch, reviewed it, and provided comments in the bug 
report. The short version is:


* The new API looks good

* There is a missing '@since 9' in the javadoc comments along with a few 
typos / style issues


* Rather than using reflection and setAccessible in the implementation, 
please add a public getHostContainer method to EmbeddedWindow (since it 
is an internal method, there is no concern with doing that -- it isn't API).


Additionally, I requested JDK 9 release team approval for this. The 
approval process can proceed in parallel with your addressing the issues 
I raised.


-- Kevin


Alexander Nyssen wrote:

Hi Kevin,

attached please find a revised patch. My comments are inlined.

Am 28.07.2016 um 18:03 schrieb Kevin Rushforth 
>:


Hi

Alexander Nyssen wrote:

Hi,

I have added my comments below:


Am 28.07.2016 um 17:22 schrieb Kevin Rushforth 
>:


I got the attachment, since Alexander also CCed me directly. I will 
attach it shortly.


Thanks!


Done.




I do have two comments on this:

1) We are past Feature Freeze, so all Enhancements need formal JDK 
9 R-team approval [1][2]. In this case, the justification can be 
internal API that is no longer accessible in JDK 9 due to Jigsaw (I 
would be very reluctant to consider any other Enhancement request 
this late in the process), but I will need to look at it and then 
take it through the approval process, provided that I feel it is in 
scope.


I was not aware about this, but I would of course appreciate if it 
could be included (due to Jigsaw). Thanks for considering it at least.


I'll take a closer look tomorrow or Monday (no more time today). At 
first glance it seems like something reasonable to take forward.


That sounds promising. Thanks!



2) Some of the changes you list seem unrelated to this enhancement 
and are better done as separate issues (e.g., the rework of the 
SWTCursorsTest). Also, I am unconvinced of the need to force GTK 2; 
in fact it seems at odds with the work we have done with JEP 283 [3].


Well, the test case refactoring is somehow related, as I introduced 
the common SWT rule while introducing the second SWT test. However, 
I could provide it as a separate contribution if that was wished 
(and a JIRA issue was provided), but the rest of this contribution 
of course requires it as a prerequisite. If this enhancement could 
not be included in JDK 9, I would have to provide it as a separate 
contribution, as I would have to re-introduce FXCanvasTest in other 
succeeding bugfix contributions (JDK-8143596, JDK-8143596).


I see. I did take a quick look at this and the test changes seem fine 
as part of this. I see you created the new test with 'hg cp' (or 
similar) which records it as a copy of the SWTCursorsTest.java file, 
which given the number of changes is not needed (and not really 
useful), but that's easy to fix.


Done (I copied it within IntelliJ and the IDE seems to have applied hg 
copy).




There are several white space changes in FXCanvas.java that should be 
reverted. Our policy is that we do not make unrelated changes, 
including white space changes, in portions of a file that aren't 
otherwise modified by a patch.


Done (I used the IntelliJ formatter). 



The GTK2 flag I introduced just affects SWT. As the swt library that 
is bundled is rather old (3.7.2) that seemed to be safer (we have 
observed quite a few problems when running SWT on GTK3). We can of 
course remove it if tests are not affected by it.


We don't actually bundle swt itself, although we do download an old 
copy to link against, and to run tests against. In any case, given 
that our minimum Linux platform for JDK 9 is Ubuntu 16.04, it might 
not have GTK2 installed by default. Please revert this change to 
build.gradle. If test issues arise on Linux we will deal with it at 
that time (possibly by moving to a newer version of swt to run tests).


I removed the SWT option. However, the previous logger message is no 
longer valid and should be removed, so the patch still contains a 
change to build.gradle.




Thanks.

— Kevin


Regards,
Alexander












— Kevin


Regards,
Alexander



[1] 
http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-June/004485.html

[2]  http://openjdk.java.net/projects/jdk9/fc-extension-process
[3] https://bugs.openjdk.java.net/browse/JDK-8145568


Phil Race wrote:

The mailing list rejects attachments so we got nothing.

-phil.

On 7/28/2016 8:06 AM, Alexander Nyssen wrote:

Hi Kevin, all,

attached please find a patch that fixes JDK-8160325. The patch 
comprises the following changes:


- Provided static FXCanvas#getFXCanvas(Scene) method to obtain 
the FXCanvas instance embedding the given Scene instance.
- Added EmbeddedWindow.getHost() so the HostInterface can be 
retrieved.
- Added 

Re: [PATCH] 8160325: Provide a public API to obtain the FXCanvas for an embedded scene.

[Kevin Rushforth]

Hi

Alexander Nyssen wrote:

Hi,

I have added my comments below:


Am 28.07.2016 um 17:22 schrieb Kevin Rushforth 
>:


I got the attachment, since Alexander also CCed me directly. I will 
attach it shortly.


Thanks!


Done.


I do have two comments on this:

1) We are past Feature Freeze, so all Enhancements need formal JDK 9 
R-team approval [1][2]. In this case, the justification can be 
internal API that is no longer accessible in JDK 9 due to Jigsaw (I 
would be very reluctant to consider any other Enhancement request 
this late in the process), but I will need to look at it and then 
take it through the approval process, provided that I feel it is in 
scope.


I was not aware about this, but I would of course appreciate if it 
could be included (due to Jigsaw). Thanks for considering it at least.


I'll take a closer look tomorrow or Monday (no more time today). At 
first glance it seems like something reasonable to take forward.


2) Some of the changes you list seem unrelated to this enhancement 
and are better done as separate issues (e.g., the rework of the 
SWTCursorsTest). Also, I am unconvinced of the need to force GTK 2; 
in fact it seems at odds with the work we have done with JEP 283 [3].


Well, the test case refactoring is somehow related, as I introduced 
the common SWT rule while introducing the second SWT test. However, I 
could provide it as a separate contribution if that was wished (and a 
JIRA issue was provided), but the rest of this contribution of course 
requires it as a prerequisite. If this enhancement could not be 
included in JDK 9, I would have to provide it as a separate 
contribution, as I would have to re-introduce FXCanvasTest in other 
succeeding bugfix contributions (JDK-8143596, JDK-8143596).


I see. I did take a quick look at this and the test changes seem fine as 
part of this. I see you created the new test with 'hg cp' (or similar) 
which records it as a copy of the SWTCursorsTest.java file, which given 
the number of changes is not needed (and not really useful), but that's 
easy to fix.


There are several white space changes in FXCanvas.java that should be 
reverted. Our policy is that we do not make unrelated changes, including 
white space changes, in portions of a file that aren't otherwise 
modified by a patch.


The GTK2 flag I introduced just affects SWT. As the swt library that 
is bundled is rather old (3.7.2) that seemed to be safer (we have 
observed quite a few problems when running SWT on GTK3). We can of 
course remove it if tests are not affected by it.


We don't actually bundle swt itself, although we do download an old copy 
to link against, and to run tests against. In any case, given that our 
minimum Linux platform for JDK 9 is Ubuntu 16.04, it might not have GTK2 
installed by default. Please revert this change to build.gradle. If test 
issues arise on Linux we will deal with it at that time (possibly by 
moving to a newer version of swt to run tests).


Thanks.

-- Kevin






— Kevin


Regards,
Alexander



[1] http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-June/004485.html
[2]  http://openjdk.java.net/projects/jdk9/fc-extension-process
[3] https://bugs.openjdk.java.net/browse/JDK-8145568


Phil Race wrote:

The mailing list rejects attachments so we got nothing.

-phil.

On 7/28/2016 8:06 AM, Alexander Nyssen wrote:

Hi Kevin, all,

attached please find a patch that fixes JDK-8160325. The patch 
comprises the following changes:


- Provided static FXCanvas#getFXCanvas(Scene) method to obtain the 
FXCanvas instance embedding the given Scene instance.

- Added EmbeddedWindow.getHost() so the HostInterface can be retrieved.
- Added FXCanvasTest with a test method to test correct behavior of 
FXCanvas#getFXCanvas(Scene).
- Introduced SwtTest JUnit MethodRule to have more concise tests 
and ensure it is also used by SWTCursorsTest.

- Ensured SWT tests are executed using GTK2 on Linux.

I reworked the existing SWTCursorsTest while introducing 
FXCanvasTest to be more concise.


Regards,
Alexander