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
<[email protected] <mailto:[email protected]>>:
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
<[email protected] <mailto:[email protected]>>:
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
=
=
