[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