[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-openjfx-dev
Subject: Re: [11] RFR: Enhancement: JDK-8195811:Support FX Swing interop using public API
From: Prasanta Sadhukhan <prasanta.sadhukhan () oracle ! com>
Date: 2018-07-16 5:13:13
Message-ID: 2b6eb6db-37ca-9375-0248-84ff28aa9e91 () oracle ! com
[Download RAW message or body]
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
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic