Hi Kevin, sorry for that (I’m a Git guy and don’t feel very comfortable with Mercurial yet). I have attached a revised patch that (hopefully) provides the correct changes.
Regards, Alexander > Am 27.07.2016 um 18:51 schrieb Kevin Rushforth <[email protected]>: > > I attached the patch to the JBS issue. You have introduced a bug in the > SWTCursorsTest.java JUnit test -- it appears to be an almost-exact copy of > the manual test (including the now-incorrect class name). Also, you have > inadvertently included an unwanted modification to .idea/modules.xml that > should be reverted. Please send a new patch with the above two fixed. > > The rest looks fine to me, but I will wait to verify until you provide an > update to fix the unit test. > > As for the JIGSAW mode tests, I agree that can/should be a follow-on effort. > > -- Kevin > > > Alexander Nyssen wrote: >> >> Hi Kevin, all, >> >> attached please find an updated patch and my replies to Kevin’s comments at >> https://bugs.openjdk.java.net/browse/JDK-8088147: >> <https://bugs.openjdk.java.net/browse/JDK-8088147:> >> >>> build.gradle >>> >>> 1. In the following: >>> >>> + if (IS_JIGSAW_TEST) { >>> + enabled = false // FIXME: JIGSAW -- support this with modules >>> + logger.info <http://logger.info/>("JIGSAW Testing disabled for swt") >>> >>> I verified that it correctly skips this in JIGSAW mode. Can you look into >>> implementing this, though? It should not be difficult, and once we switch >>> to only supporting Jigsaw mode the tests will never be run until this is >>> done. If it does turn out to be too much work, then we could consider it as >>> a follow-on. >>> >> >> As already indicated in an earlier mail I would prefer to treat building up >> JIGSAW tests as a follow-on. I have not gained much experience with JIGSAW >> yet, but I would be willing to support this as far as I can. The problem I >> currently see is that SWT is still an automatic module, and JIGSAW-based SWT >> tests would have to access code from the swt-debug.jar, which is as well an >> automatic module. AFAIK, we would have to turn SWT into a named module first >> in order to make swt-debug.jar available on its module path. That seems to >> be out of scope here and should probably be investigated in an own issue. >> >>> 2. Platform logic: >>> >>> + if(IS_MAC){ >>> + enabled = false >>> ... >>> + } >>> + if(IS_LINUX){ >>> >>> Normally we would handle this in the test itself by using assumeTrue and >>> platform checks. In this case, though, since it applies to all SWT tests >>> run on Mac (or Linux in case of the warning), this seems fine. >>> >>> Please fix the spacing to match our coding conventions, though. There >>> should be a space after the 'if' and a space before the '{'. So: >>> >>> if (IS_MAC) { >> >> Ok. fine. We could even try to get SWT tests working on Mac by falling back >> to ant-based test execution here, but I would propose to handle this in a >> different issue as well (after all, this issue is about SWT image cursors >> and not about building up an SWT test harness). >> >>> modules/swt/src/main/java/javafx/embed/swt/SWTCursors.java >>> >>> 3. Spacing issues: >>> >>> + if(display == null){ >>> >>> Please add a space after '(' and before '}' >>> >>> >>> -} >>> +} >>> \ No newline at end of file >>> >>> Please restore the newline after the last line. >> >> Done. >> >>> SWTCursorsTest.java >>> >>> 4. Need a blank line before the package declaration: >>> >>> + */ >>> +package test.javafx.embed.swt; >>> + >> >> Done. >> >>> 5. Can you sort the imports alphabetically? >> >> I organized the imports and removed unused ones. It seems they are pretty >> much consistent with those of the other SWT classes. >> >> >>> 6. Since the test loops waiting on a latch, please add a 10 second timeout >>> for the test: >>> >>> @Test(timeout=10000) >>> public void testImageCursor() throws Throwable { >> >> Done. >> >>> SWTImageCursorTest.java >>> >>> 7. The following can use a lambda (as the setOnMouseEntered already did). >>> >>> + rect.setOnMouseExited(new EventHandler<MouseEvent>() { >>> + @Override >>> + public void handle(MouseEvent event) { >>> + scene.setCursor(null); >>> + } >>> + }); >> >> Done. >> >> Kevin, thanks for picking this up. >> >> Regards, >> Alexander >> >> = >> >> >>> Am 27.07.2016 um 01:15 schrieb Kevin Rushforth <[email protected] >>> <mailto:[email protected]>>: >>> >>> Picking this back up again, I have just added my comments to JBS issue. >>> >>> https://bugs.openjdk.java.net/browse/JDK-8088147 >>> <https://bugs.openjdk.java.net/browse/JDK-8088147> >>> >>> The comments are minor in scope, so I suspect it will be ready for final >>> review after one more iteration. It will need one other reviewer, since I >>> am sponsoring the change. >>> >>> Could someone else review this as well (maybe Alexander Z or Sergey)? >>> >>> -- Kevin >>> >>> >>> Kevin Rushforth wrote: >>>> I'm back, and given that the review will take some time anyway, I will >>>> sponsor this once the review is complete. I see that Guru uploaded the >>>> patch for you (thanks, Guru) so I can test it next week. >>>> >>>> -- Kevin >>>> >>>> >>>> Alexander Nyssen wrote: >>>>> It seems I was unsuccessful again. If somebody would be willing to >>>>> sponsor this contribution while Kevin is away (or at least update the >>>>> patch provided for JDK-8088147), I could mail the patch privately. >>>>> >>>>> Regards, >>>>> Alexander >>>>> >>>>> >>>>>> Am 11.07.2016 um 19:59 schrieb Alexander Nyssen <[email protected]> >>>>>> <mailto:[email protected]>: >>>>>> >>>>>> It seems my attachment has again been ‚consumed‘ by the list. Trying >>>>>> again with an archive containing the patch file. >>>>>> >>>>>> Regards, >>>>>> Alexander >>>>>> >>>>>> >>>>>> >>>>>>> Am 08.07.2016 um 23:28 schrieb Alexander Nyssen <[email protected]> >>>>>>> <mailto:[email protected]>: >>>>>>> >>>>>>> Hi Kevin, all, >>>>>>> attached please find a revised patch, where I have added the required >>>>>>> export to PlatformImpl and have fixed the build script, so it does no >>>>>>> longer break when being executed in jigsaw mode. >>>>>>> As swt is no named module yet, I have disabled the JUnit test in jigsaw >>>>>>> mode for now. We would probably have to set up swt as a named module to >>>>>>> enable this, and would further have to make swt-debug.jar available as >>>>>>> an automated module on its module path. But this is a different problem. >>>>>>> >>>>>>> Regards, >>>>>>> Alexander >>>>>>> >>>>>>> >>>>>>> >>>>>>>> Am 30.06.2016 um 12:14 schrieb Alexander Nyssen <[email protected]> >>>>>>>> <mailto:[email protected]>: >>>>>>>> >>>>>>>> Hi Kevin, >>>>>>>> >>>>>>>> >>>>>>>>> Am 30.06.2016 um 01:20 schrieb Kevin Rushforth >>>>>>>>> <[email protected]> <mailto:[email protected]>: >>>>>>>>> >>>>>>>>> Hi Alexander, >>>>>>>>> >>>>>>>>> I attached the patch to the bug: >>>>>>>>> >>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8088147 >>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8088147> >>>>>>>>> >>>>>>>>> If I build it and run the manual test in "legacy" mode, meaning run >>>>>>>>> everything with 9+109 and the legacy jfxrt.jar file, then it runs and >>>>>>>>> the cursor now changes. So this looks like a good starting point for >>>>>>>>> a fix. >>>>>>>>> >>>>>>>> Fine. >>>>>>>> >>>>>>>> >>>>>>>>> I tried building and running this in Jigsaw mode (building with >>>>>>>>> jdk-9+109, but running the tests with a more recent JDK that includes >>>>>>>>> modularization support), and noticed two problems right away that >>>>>>>>> must be addressed: >>>>>>>>> >>>>>>>>> 1. The unit tests for SWT are missing some of the jigsaw test tasks >>>>>>>>> so the build fails right away with an exception from gradle: >>>>>>>>> >>>>>>>>> >>>>>>>>>> Task with name 'jigsawTestsLinux' not found in project ':swt'. >>>>>>>>>> >>>>>>>>> Wiring up SWT-based tests to our unit test harness will take a bit >>>>>>>>> more work than what you have provided (not even counting the Mac >>>>>>>>> issue, which could be handled by excluding the test on Mac). In the >>>>>>>>> short term, relying on manual tests for this fix might be best. >>>>>>>>> >>>>>>>>> >>>>>>>> I did not execute the tests in jigsaw mode yet, because other tests >>>>>>>> failed in this mode, too (as indicated in an earlier discussion). I >>>>>>>> will try to set things up in a virtual machine with Windows and/or >>>>>>>> Linux so I can work on the Gradle tests without having to deal with >>>>>>>> the Mac issue. The test harness will IMHO also be required for other >>>>>>>> contributions, and it would of course be fine if the automated test, I >>>>>>>> included in this patch, could be executed as well. >>>>>>>> >>>>>>>> >>>>>>>>> 2. You have introduced a dependency on a new internal package, >>>>>>>>> com.sun.javafx.tk. If this is required in order to implement the fix, >>>>>>>>> then you will need to add this package to the list of packages >>>>>>>>> exports to javafx.swt in PlatformImpl; otherwise the following >>>>>>>>> exception is thrown at runtime: >>>>>>>>> >>>>>>>>> Exception in thread "JavaFX Application Thread" >>>>>>>>> java.lang.IllegalAccessError: class javafx.embed.swt.SWTCursors (in >>>>>>>>> module javafx.swt) cannot access class com.sun.javafx.tk.Toolkit (in >>>>>>>>> module javafx.graphics) because module javafx.graphics does not >>>>>>>>> export com.sun.javafx.tk to module javafx.swt >>>>>>>>> >>>>>>>> This dependency is required unless there is public API to convert a >>>>>>>> platform image (which is provided by the image cursor frame) to an >>>>>>>> image. To me, Image image = >>>>>>>> Toolkit.getImageAccessor().fromPlatformImage(cursorFrame.getPlatformImage()); >>>>>>>> >>>>>>>> seemed to be the way to go. I will thus add the respective export in a >>>>>>>> revised patch. >>>>>>>> >>>>>>>> >>>>>>>>> I won't have time to sponsor this change until the second half of >>>>>>>>> July, but if others have time, the review can proceed and I'll pick >>>>>>>>> it back up then if it is in good enough shape to run. >>>>>>>>> >>>>>>>> Especially setting up the SWT test harness will be kind of a blocker >>>>>>>> for succeeding contributions, so it would be nice if somebody could >>>>>>>> step in. >>>>>>>> >>>>>>>> Regards, >>>>>>>> Alexander >>>>>>>> >>>>>>>> >>>>>>>>> -- Kevin >>>>>>>>> >>>>>>>>> >>>>>>>>> Kevin Rushforth wrote: >>>>>>>>> >>>>>>>>>> Hi Alexander, >>>>>>>>>> >>>>>>>>>> It looks like your patch was stripped out by the mailing list server. >>>>>>>>>> >>>>>>>>>> Can you please send me the patch offline, as a zip file (so line >>>>>>>>>> endings are preserved across different systems), and I will unzip it >>>>>>>>>> and attach it to the bug report. >>>>>>>>>> >>>>>>>>>> -- Kevin >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Alexander Nyssen wrote: >>>>>>>>>> >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> I have worked on a first contribution related to JDK-8088147. >>>>>>>>>>> Attached please find a patch (created in extended Git format) that >>>>>>>>>>> comprises the related changes. I have augmented the implementation >>>>>>>>>>> of javafx.embed.swt.SWTCursors to handle the image cursor case. I >>>>>>>>>>> further adjusted javafx.scene.Scene to update the cursor frame (in >>>>>>>>>>> addition to the cursor) within synchronizeSceneProperties, so the >>>>>>>>>>> cursor is not cleared in the first pulse succeeding the cursor >>>>>>>>>>> property change. >>>>>>>>>>> I have added an automated JUnit test (SWTCursorsTest) to the swt >>>>>>>>>>> module, as well as a manual test (SWTImageCursorTest) to the >>>>>>>>>>> systemTests module, with which the proper behavior can be verified. >>>>>>>>>>> As no tests for SWT existed so far, I updated the build.gradle and >>>>>>>>>>> gradle.properties files to support an SWT_TEST option, which allows >>>>>>>>>>> to handle them similar to Swing tests. I also added the respective >>>>>>>>>>> SWT dependency to the systemTests module. Please note that the >>>>>>>>>>> JUnit test can currently not be executed using Gradle on the Mac >>>>>>>>>>> (where the manual test currently is the single option; the >>>>>>>>>>> automated tests are disabled), because there SWT-based tests >>>>>>>>>>> require the -XstartOnFirstThread option that is currently not >>>>>>>>>>> supported by the Gradle test runner (see >>>>>>>>>>> https://issues.gradle.org/browse/GRADLE-3290 >>>>>>>>>>> <https://issues.gradle.org/browse/GRADLE-3290> for details). We >>>>>>>>>>> would have to use an ant task as a workaround. >>>>>>>>>>> >>>>>>>>>>> Regards, >>>>>>>>>>> Alexander >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>> >>>>> >> >> =
