Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8088147

[SWT] FXCanvas: implement custom cursors

    Details

      Description

      Custom cursors are currently not supported in FXCanvas. Corresponding code is commented out in SWTCursors.
      1. JDK-8088147.patch
        18 kB
        Kevin Rushforth
      2. JDK-8088147.revised.27-07-16.patch
        18 kB
        Kevin Rushforth
      3. JDK-8088147.revised.27-07-16.patch
        18 kB
        Kevin Rushforth
      4. JDK-8088147.revised.patch
        18 kB
        Guru Hb

        Activity

        art Artem Ananiev created issue -
        art Artem Ananiev made changes -
        Field Original Value New Value
        Status New [ 10000 ] Open [ 1 ]
        snorthov Steve Northover (Inactive) made changes -
        Assignee Artem Ananiev [ art ] Steve Northover [ snorthov ]
        snorthov Steve Northover (Inactive) made changes -
        Fix Version/s Van Ness [ 10750 ]
        snorthov Steve Northover (Inactive) made changes -
        Summary FXCanvas: implement custom cursors [SWT] FXCanvas: implement custom cursors
        kcr Kevin Rushforth made changes -
        Assignee Steve Northover [ snorthov ]
        duke J. Duke (Inactive) made changes -
        Component/s Swing [ 10060 ]
        Component/s javafx [ 12520 ]
        duke J. Duke (Inactive) made changes -
        Workflow jfxc-workflow-v2 [ 82394 ] JBS Workflow [ 124382 ]
        squierts Tony Squier made changes -
        Project Import Fri Jun 12 21:05:53 UTC 2015 [ 1434143153521 ]
        squierts Tony Squier made changes -
        Project Runtime [ 11000 ] JDK [ 10100 ]
        Key RT-27939 JDK-8088147
        Fix Version/s 9 [ 14949 ]
        Fix Version/s 9 [ 17733 ]
        Affects Version/s 8 [ 11815 ]
        Affects Version/s 8 [ 17730 ]
        Imported 12/Jun/15 3:28 PM
        Component/s javafx [ 11900 ]
        Component/s javafx [ 12124 ]
        squierts Tony Squier made changes -
        Subcomponent swing [ 1510 ] swing [ 1388 ]
        jgodinez Jennifer Godinez (Inactive) made changes -
        Fix Version/s tbd_major [ 11972 ]
        Fix Version/s 9 [ 14949 ]
        kcr Kevin Rushforth made changes -
        Fix Version/s 9 [ 14949 ]
        Fix Version/s tbd_major [ 11972 ]
        Labels cursor fxcanvas cursor fxcanvas nicetohave
        Subcomponent swing [ 1388 ] other [ 1385 ]
        kcr Kevin Rushforth made changes -
        Assignee Kevin Rushforth [ kcr ]
        Hide
        kcr Kevin Rushforth added a comment -
        Attached patch provided by Alexander Nyssen (alexander at nyssen.org)
        Show
        kcr Kevin Rushforth added a comment - Attached patch provided by Alexander Nyssen (alexander at nyssen.org)
        kcr Kevin Rushforth made changes -
        Attachment JDK-8088147.patch.zip [ 60801 ]
        kcr Kevin Rushforth made changes -
        Attachment JDK-8088147.patch [ 60802 ]
        kcr Kevin Rushforth made changes -
        Attachment JDK-8088147.patch.zip [ 60801 ]
        Hide
        ghb Guru Hb added a comment -
        Updated Patch provided by Alexander Nyssen (alexander at nyssen.org)
        Show
        ghb Guru Hb added a comment - Updated Patch provided by Alexander Nyssen (alexander at nyssen.org)
        ghb Guru Hb made changes -
        Attachment JDK-8088147.revised.patch [ 61359 ]
        kcr Kevin Rushforth made changes -
        Priority P4 [ 4 ] P3 [ 3 ]
        kcr Kevin Rushforth made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Understanding Fix Understood [ 10001 ]
        Hide
        kcr Kevin Rushforth added a comment -
        Review comments for JDK-8088147.revised.patch:

        The new version looks good. I have tested on Linux and will test on Mac and Windows. It will need one other reviewer, since I am sponsoring the change.

        Here are a few things that need to be addressed.

        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.


        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) {


        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.


        SWTCursorsTest.java

        4. Need a blank line before the package declaration:

        + */
        +package test.javafx.embed.swt;
        +

        5. Can you sort the imports alphabetically?


        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 {


        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);
        + }
        + });
        Show
        kcr Kevin Rushforth added a comment - Review comments for JDK-8088147 .revised.patch: The new version looks good. I have tested on Linux and will test on Mac and Windows. It will need one other reviewer, since I am sponsoring the change. Here are a few things that need to be addressed. 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. 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) { 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. SWTCursorsTest.java 4. Need a blank line before the package declaration: + */ +package test.javafx.embed.swt; + 5. Can you sort the imports alphabetically? 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 { 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); + } + });
        Hide
        kcr Kevin Rushforth added a comment -
        Attached latest version of the patch.
        Show
        kcr Kevin Rushforth added a comment - Attached latest version of the patch.
        kcr Kevin Rushforth made changes -
        Attachment JDK-8088147.revised.27-07-16.patch [ 61771 ]
        Hide
        kcr Kevin Rushforth added a comment -
        Attached an updated version of this morning's patch: JDK-8088147.revised.27-07-16.patch
        Show
        kcr Kevin Rushforth added a comment - Attached an updated version of this morning's patch: JDK-8088147 .revised.27-07-16.patch
        kcr Kevin Rushforth made changes -
        Attachment JDK-8088147.revised.27-07-16.patch [ 61772 ]
        Hide
        kcr Kevin Rushforth added a comment -
        The following description of the fix was copied from the latest (27-07-16) attached patch:

        - Augmented implementation of SWTCursorsTest to handle image cursors properly.
        - Ensured PlatformImpl exposes com.sun.javafx.tk to swt, as this is required to convert the cursor frame platform image to an javafx image.
        - Ensured cursor frame is updated when synchronizing scene properties, so it does not get overwritten.
        - Added an automated JUnit test (SWTCursorsTest), which is currently not executed on Mac (because -XstartOnFirstThread is not supported by Gradle test runner) and disabled for jigsaw tests (as SWT is no named module yet).
        - Added SWT_TEST option to build.gradle, which is only evaluated when FULL_TEST is enabled.
        - Enhanced gradle.properties.template to provide SWT_TEST property.
        - Added a manual test class (SWTImageCursorTest) that can be used to validate the cursor change.
        Show
        kcr Kevin Rushforth added a comment - The following description of the fix was copied from the latest (27-07-16) attached patch: - Augmented implementation of SWTCursorsTest to handle image cursors properly. - Ensured PlatformImpl exposes com.sun.javafx.tk to swt, as this is required to convert the cursor frame platform image to an javafx image. - Ensured cursor frame is updated when synchronizing scene properties, so it does not get overwritten. - Added an automated JUnit test (SWTCursorsTest), which is currently not executed on Mac (because -XstartOnFirstThread is not supported by Gradle test runner) and disabled for jigsaw tests (as SWT is no named module yet). - Added SWT_TEST option to build.gradle, which is only evaluated when FULL_TEST is enabled. - Enhanced gradle.properties.template to provide SWT_TEST property. - Added a manual test class (SWTImageCursorTest) that can be used to validate the cursor change.
        Hide
        kcr Kevin Rushforth added a comment -
        All my testing looks good. The latest patch, 27-07-16, looks good.

        +1
        Show
        kcr Kevin Rushforth added a comment - All my testing looks good. The latest patch, 27-07-16, looks good. +1
        Hide
        kcr Kevin Rushforth added a comment -
        The following is just FYI for the future, since I will format the changeset comment per the OpenJDK requirements listed here:

        http://openjdk.java.net/guide/producingChangeset.html

        1. The first line must start with the 7-digit bug ID without the 'JDK-' prefix, followed by a colon and a space. The rest of the text should match the title of the bug.

        2. There is an optional single-line summary line with the prefix 'Summary: ' Any longer description would go into the bug ID as a comment (which I did above for your description of the patch)

        3. I got an error applying your patch, since it used characters not in the Ascii character set in the changset comment:

        $ hg qpush
        applying JDK-8088147.revised.27-07-16.patch
        transaction abort!
        rollback completed
        cleaning up working directory...done
        abort: decoding near 'exander Nyßen <alex': 'ascii' codec can't decode byte 0xc3 in position 12: ordinal not in range(128)!

        In any case, the following is the changeset comment I will use, with the appropriate reviewer filled in once approved by a second reviewer.

        8088147: [SWT] FXCanvas: implement custom cursors
        Summary: Augmented implementation of SWTCursors to handle image cursors properly. Added unit test and manual test.
        Reviewed-by: XXXXX
        Contributed-by: alexander@nyssen.org
        Show
        kcr Kevin Rushforth added a comment - The following is just FYI for the future, since I will format the changeset comment per the OpenJDK requirements listed here: http://openjdk.java.net/guide/producingChangeset.html 1. The first line must start with the 7-digit bug ID without the 'JDK-' prefix, followed by a colon and a space. The rest of the text should match the title of the bug. 2. There is an optional single-line summary line with the prefix 'Summary: ' Any longer description would go into the bug ID as a comment (which I did above for your description of the patch) 3. I got an error applying your patch, since it used characters not in the Ascii character set in the changset comment: $ hg qpush applying JDK-8088147 .revised.27-07-16.patch transaction abort! rollback completed cleaning up working directory...done abort: decoding near 'exander Nyßen <alex': 'ascii' codec can't decode byte 0xc3 in position 12: ordinal not in range(128)! In any case, the following is the changeset comment I will use, with the appropriate reviewer filled in once approved by a second reviewer. 8088147: [SWT] FXCanvas: implement custom cursors Summary: Augmented implementation of SWTCursors to handle image cursors properly. Added unit test and manual test. Reviewed-by: XXXXX Contributed-by: alexander@nyssen.org
        Hide
        azvegint Alexander Zvegintsev (Inactive) added a comment -
        +1
        Show
        azvegint Alexander Zvegintsev (Inactive) added a comment - +1
        Hide
        kcr Kevin Rushforth added a comment -
        Changeset: 286fab2d6958
        Author: kcr
        Date: 2016-07-27 13:04 -0700
        URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/286fab2d6958

        8088147: [SWT] FXCanvas: implement custom cursors
        Summary: Augmented implementation of SWTCursors to handle image cursors properly. Added unit test and manual test.
        Reviewed-by: azvegint
        Contributed-by: alexander@nyssen.org
        Show
        kcr Kevin Rushforth added a comment - Changeset: 286fab2d6958 Author: kcr Date: 2016-07-27 13:04 -0700 URL: http://hg.openjdk.java.net/openjfx/9-dev/rt/rev/286fab2d6958 8088147: [SWT] FXCanvas: implement custom cursors Summary: Augmented implementation of SWTCursors to handle image cursors properly. Added unit test and manual test. Reviewed-by: azvegint Contributed-by: alexander@nyssen.org
        kcr Kevin Rushforth made changes -
        Status In Progress [ 3 ] Resolved [ 5 ]
        Understanding Fix Understood [ 10001 ]
        Resolution Fixed [ 1 ]

          People

          • Assignee:
            kcr Kevin Rushforth
            Reporter:
            art Artem Ananiev
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Imported: