[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-openjfx-dev
Subject: Re: [PATCH] 8088147: FXCanvas: implement custom cursors (revised)
From: Alexander Zvegintsev <alexander.zvegintsev () oracle ! com>
Date: 2016-07-27 22:06:07
Message-ID: e877ad22-985d-9451-fd0a-be15a2fe29d0 () oracle ! com
[Download RAW message or body]
Looks good to me too.
--
Thanks,
Alexander.
On 28.07.2016 0:21, Kevin Rushforth wrote:
> I have reviewed and tested the latest patch on Mac, Windows, Linux. I
> added my approval to the JIRA.
>
> Now we just one more reviewer.
>
> Alexander Z or Sergey: are either of you able to review?
>
> -- Kevin
>
>
> Alexander Nyssen wrote:
>> 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
>>> <kevin.rushforth@oracle.com <mailto:kevin.rushforth@oracle.com>>:
>>>
>>> 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@oracle.com <mailto:kevin.rushforth@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@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@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@nyssen.org>:
>>>>>>>>>>
>>>>>>>>>> Hi Kevin,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Am 30.06.2016 um 01:20 schrieb Kevin Rushforth
>>>>>>>>>>> <kevin.rushforth@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
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>
>>>> =
>>
>> =
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic