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 <kevin.rushfo...@oracle.com>: > > > > Alexander Nyssen wrote: >> >> Hi Kevin, >> >> thanks for your feedback. Please fin my comments inline. >> >> >>> Am 09.08.2016 um 03:10 schrieb Kevin Rushforth <kevin.rushfo...@oracle.com> >>> <mailto:kevin.rushfo...@oracle.com>: >>> >>> 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 >>>>> <kevin.rushfo...@oracle.com <mailto:kevin.rushfo...@oracle.com> >>>>> <mailto: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> >>>>>>> <mailto: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 >>>>>>> <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 >>>>>>>>> >>>>>>>>> >>
