[PATCH] 8088147: FXCanvas: implement custom cursors (revised)
Kevin Rushforth
kevin.rushforth at oracle.com
Wed Jul 27 16:51:35 UTC 2016
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:
>
>> 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
>> <kevin.rushforth at oracle.com <mailto:kevin.rushforth at oracle.com>>:
>>
>> 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