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