Hi Kevin,
attached please find a revised patch. My comments are inlined.
Am 28.07.2016 um 18:03 schrieb Kevin Rushforth <kevin.rushfo...@oracle.com
<mailto:kevin.rushfo...@oracle.com>>:
Hi
Alexander Nyssen wrote:
Hi,
I have added my comments below:
Am 28.07.2016 um 17:22 schrieb Kevin Rushforth <kevin.rushfo...@oracle.com
<mailto:kevin.rushfo...@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