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

Tue Aug 9 01:10:47 UTC 2016

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 
>> <kevin.rushforth at oracle.com <mailto:kevin.rushforth at oracle.com>>:
>>
>> Hi
>>
>> Alexander Nyssen wrote:
>>> Hi,
>>>
>>> I have added my comments below:
>>>
>>>
>>>> Am 28.07.2016 um 17:22 schrieb Kevin Rushforth 
>>>> <kevin.rushforth at oracle.com <mailto:kevin.rushforth at oracle.com>>:
>>>>
>>>> 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 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
>>>>>>
>>>>>
>>>
>