[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: [PATCH] 8160325: Provide a public API to obtain the FXCanvas for an embedded scene.
From:       Alexander Nyssen <alexander () nyssen ! org>
Date:       2016-07-28 18:11:08
Message-ID: D5C7830C-3245-47A5-A5F7-9DEE2312286D () nyssen ! org
[Download RAW message or body]

Hi Kevin,

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

> Am 28.07.2016 um 18:03 schrieb Kevin Rushforth <kevin.rushforth@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@oracle.com \
> > > <mailto:kevin.rushforth@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 \
> > > <http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-June/004485.html> [2]  \
> > > http://openjdk.java.net/projects/jdk9/fc-extension-process \
> > > <http://openjdk.java.net/projects/jdk9/fc-extension-process> [3] \
> > > https://bugs.openjdk.java.net/browse/JDK-8145568 \
> > > <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
> > > > > 
> > > > 
> > 


[prev in list] [next in list] [prev in thread] [next in thread]