[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: [PATCH] 8088147:  FXCanvas: implement custom cursors
From:       Alexander Nyssen <alexander () nyssen ! org>
Date:       2016-06-30 10:14:38
Message-ID: BE0C2F27-AF4F-474D-BE9F-9346C4FD519A () nyssen ! org
[Download RAW message or body]

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]