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:
> build.gradle
>
> 1. In the following:
>
> + if (IS_JIGSAW_TEST) {
> + enabled = false // FIXME: JIGSAW -- support this with modules
> + 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]>:
>
> Picking this back up again, I have just added my comments to JBS issue.
>
> 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]>:
>>>>
>>>> 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]>:
>>>>>
>>>>> 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]>:
>>>>>>
>>>>>> Hi Kevin,
>>>>>>
>>>>>>
>>>>>>> Am 30.06.2016 um 01:20 schrieb Kevin Rushforth
>>>>>>> <[email protected]>:
>>>>>>>
>>>>>>> Hi Alexander,
>>>>>>>
>>>>>>> I attached the patch to the bug:
>>>>>>>
>>>>>>> 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
>>>>>>>>> for details). We would have to use an ant task as a workaround.
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Alexander
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>
>>>
