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 <[email protected]>:
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
<[email protected]>:
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
<[email protected]>:
Hi Kevin,
Am 30.06.2016 um 01:20 schrieb Kevin Rushforth
<[email protected]>:
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
