http://cr.openjdk.java.net/~psadhukhan/fxswinterop/webrev.9/
with sorting and whitespace fixed, hopefully.
Regards
Prasanta
On 7/13/2018 9:16 PM, Kevin Rushforth wrote:
Hi Prasanta,
> I had done "gradle checkrepo" followed by "checkWhiteSpace -F" to
get rid of whitespace problem.
It looks like you either didn't refresh your patch, or you
subsequently introduced additional white space problems. Here is the
output of applying your .8 patch and running 'gradle checkrepo' :
> Task :checkrepo FAILED
modules/javafx.swing/src/main/java/com/sun/javafx/embed/swing/FXDnD.java
:tabs:trailingWhitespace:
modules/javafx.swing/src/main/java/com/sun/javafx/embed/swing/FXDnDInterop.java
:tabs:trailingWhitespace:
modules/javafx.swing/src/main/java/com/sun/javafx/embed/swing/InteropFactory.java
:tabs:
modules/javafx.swing/src/main/java/com/sun/javafx/embed/swing/JFXPanelInterop.java
:trailingWhitespace:
modules/javafx.swing/src/main/java/com/sun/javafx/embed/swing/SwingNodeInterop.java
:tabs:trailingWhitespace:
Found 5 whitespace or executable issues
To correct, use
bash tools/scripts/checkWhiteSpace -F -x
I'll continue testing anyway, assuming that the only changes at this
point will be white space changes or similar, which won't invalidate
testing.
-- Kevin
On 7/13/2018 2:05 AM, Prasanta Sadhukhan wrote:
Hi Kevin,Ambarish,
Please find modified webrev taking care of review comments.
http://cr.openjdk.java.net/~psadhukhan/fxswinterop/webrev.8/
I had done "gradle checkrepo" followed by "checkWhiteSpace -F" to get
rid of whitespace problem.
Regards
Prasanta
On 7/13/2018 5:29 AM, Kevin Rushforth wrote:
Hi Prasanta,
I confirm that the test failures are gone now. I need to do some
final testing (e.g., a clean build using both JDK 11 and JDK 10 on
all three platforms). I also want a second look at a couple of the
implementation files, but overall it's close I think. Here are my
review comments so far:
General review comments:
* White space problems
- trailing whitespace and TABS, which must be fixed
- InteropFactoryO constructor missing a space: 'Exception{'
* Several of the files have unused imports -- also can you please
sort the imports?
build.gradle:
* MINOR: naming suggestion:
> 2344 task checkJDKUnsupportedModule(...)
Maybe a more descrptive name for the task would be "copyModuleInfo"?
javafx.swing/src/main/module-info/module-info.java:
* Ordering of requires directives
> 41 requires static jdk.unsupported.desktop;
Can you move this before the 'requires transitive' statements -- it
should group with the previous block (the 'requires' statements)
InteropFactory:
* MINOR: maybe use a lambda for the following?
37 AccessController.doPrivileged(new
PrivilegedAction<Object>() {
38 public Object run() {
39 verbose = Boolean.valueOf(
40 System.getProperty("javafx.embed.swing.verbose"));
41 return null;
42 }
43 });
JFXPanelInterop
38 public abstract long setMask();
Would ‘getMask’ be a better name? (it’s a getter not a setter)
FOLLOW-ON BUGS:
* We should file a follow-on bug to remove the runtime qualified
exports (e.g., from testrun.args) when running a JDK that
HAS_UNSUPPORTED_DESKTOP; this isn't high priority
-- Kevin
On 7/11/2018 8:48 AM, Prasanta Sadhukhan wrote:
Hi Kevin,
Please find modified webrev fixing the other test failures by
modifying the overrideNativeWindowHandle JNI method
http://cr.openjdk.java.net/~psadhukhan/fxswinterop/webrev.7/
Regards
Prasanta
On 7/10/2018 3:58 PM, Prasanta Sadhukhan wrote:
Hi Kevin,
Please find modified webrev with some more refactoring to move
more common code to SwingNode
and also this solves SwingNodeMemoryLeakTest failure
http://cr.openjdk.java.net/~psadhukhan/fxswinterop/webrev.6/
Regards
Prasanta
On 7/10/2018 7:13 AM, Kevin Rushforth wrote:
Hi Prasanta,
The public API looks fine now. I sent you an offline note about
one of the test failures (SwingNodeMemoryLeakTest).
There are still whitespace problems that will cause a failure in
'gradle checkrepo' (and 'hg jcheck').
I'll do a more thorough review in the next day or so.
-- Kevin
On 7/9/2018 4:12 AM, Prasanta Sadhukhan wrote:
Hi Kevin,
Modified webrev to address the "public" leakage
http://cr.openjdk.java.net/~psadhukhan/fxswinterop/webrev.5/
I am looking into the test failures.
Regards
Prasanta
On 7/7/2018 4:30 AM, Kevin Rushforth wrote:
Most things are working with either mode (JDK 10 with qualified
exports or JDK 11 without), although I do get a few test failures.
There is a serious issue with leakage into the public API that
needs to be addressed before worrying about the failures, but
I'll list the test failures at the end.
SwingNode.java:
This public class is part of the API. You cannot make any of
the following fields public as this would leak implementation
into public API:
+ public int swingPrefWidth;
+ public int swingPrefHeight;
+ public int swingMaxWidth;
+ public int swingMaxHeight;
+ public int swingMinWidth;
+ public int swingMinHeight;
+ public final Object getLightweightFrame() { return lwFrame; }
+ public final ReentrantLock paintLock = new ReentrantLock();
+ public boolean grabbed; // lwframe initiated grab
+ public void setImageBuffer(...)
+ public void setImageBounds(...);
+ public void repaintDirtyRegion(...)
+ public void ungrabFocus(boolean postUngrabEvent)
If you need to access them from other packages, you can either
use the accessor pattern (this might be easiest) or else
refactor it further to move more of this down to the
implementation. I note that even though SwingNodeInterop is
abstract, it can still have non-abstract methods if that helps
in your refactoring.
SwingFXUtils.java
Same problem as SwingNode, although to a lesser extent. The
following must not be public:
+ public static void runOnFxThread(Runnable runnable)
+ public static void runOnEDT(final Runnable r)
+ public static void runOnEDTAndWait(Object nestedLoopKey,
Runnable r)
+ public static void leaveFXNestedLoop(Object nestedLoopKey)
JFXPanel is fine.
-----------------
* System tests failures on Linux:
test.robot.javafx.embed.swing.SwingNodeJDialogTest >
testJDialogAbove FAILED
java.lang.AssertionError: JDialog is not above JavaFX stage
expected:<java.awt.Color[r=0,g=0,b=255]> but
was:<java.awt.Color[r=0,g=128,b=0]>
test.robot.javafx.embed.swing.SwingNodeJDialogTest >
testNodeRemovalAfterShow FAILED
java.lang.AssertionError: JDialog is not above JavaFX stage
expected:<java.awt.Color[r=0,g=0,b=255]> but
was:<java.awt.Color[r=0,g=128,b=0]>
test.robot.javafx.embed.swing.SwingNodeJDialogTest >
testStageCloseAfterShow FAILED
java.lang.AssertionError: JDialog is not above JavaFX stage
expected:<java.awt.Color[r=0,g=0,b=255]> but
was:<java.awt.Color[r=0,g=128,b=0]>
test.javafx.embed.swing.SwingNodeMemoryLeakTest >
testSwingNodeMemoryLeak FAILED
java.lang.AssertionError: expected:<10> but was:<9>
at org.junit.Assert.fail(Assert.java:91)
at org.junit.Assert.failNotEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:126)
at org.junit.Assert.assertEquals(Assert.java:470)
at org.junit.Assert.assertEquals(Assert.java:454)
at
test.javafx.embed.swing.SwingNodeMemoryLeakTest.testSwingNodeMemoryLeak(SwingNodeMemoryLeakTest.java:97)
Two of these, SwingNodeJDialogTest.testNodeRemovalAfterShow and
SwingNodeJDialogTest.testStageCloseAfterShow, also fail on Mac
-- Kevin
On 7/5/2018 11:29 PM, Prasanta Sadhukhan wrote:
My Bad. Please find modified webrev restoring the filter
http://cr.openjdk.java.net/~psadhukhan/fxswinterop/webrev.4/
Regards
Prasanta
On 7/6/2018 6:47 AM, Kevin Rushforth wrote:
One quick comment:
This no longer compiles with OpenJDK10. It looks like the
logic to optionally filter out jdk.unsupported.desktop from
javafx.swing's module-info.java got lost between the .2 and
.3 versions.
-- Kevin
On 7/4/2018 4:35 AM, Prasanta Sadhukhan wrote:
Hi All,
Please review an enhancement to support openjfx swing
interoperability once the dependancy of internal jdk classes
are removed.
JDK-8202199
<https://bugs.openjdk.java.net/browse/JDK-8202199> provided
a new "jdk.unsupported.desktop" module in JDK 11 that
exports public API that is intended to be used by the
javafx.swing module
and unbundled OpenJFX is now made to depend on these APIs to
support interoperation
between Swing and JavaFX components to replace previous use
of internal APIs when it was part of Oracle JDK.
Bug: https://bugs.openjdk.java.net/browse/JDK-8195811
webrev:
http://cr.openjdk.java.net/~psadhukhan/fxswinterop/webrev.3/
Regards
Prasanta
