[PATCH] 8088147: FXCanvas: implement custom cursors (revised)
Kevin Rushforth
kevin.rushforth at oracle.com
Tue Jul 26 23:15:48 UTC 2016
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 <alexander at nyssen.org>:
>>>
>>> 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
>>>> <alexander at nyssen.org>:
>>>>
>>>> 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
>>>>> <alexander at nyssen.org>:
>>>>>
>>>>> Hi Kevin,
>>>>>
>>>>>
>>>>>> Am 30.06.2016 um 01:20 schrieb Kevin Rushforth
>>>>>> <kevin.rushforth at oracle.com>:
>>>>>>
>>>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>
>>
More information about the openjfx-dev
mailing list