[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: Kevin Rushforth <kevin.rushforth () oracle ! com>
Date: 2016-07-27 21:21:21
Message-ID: 579925D1.4040101 () oracle ! com
[Download RAW message or body]
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