[11] RFR: Enhancement: JDK-8195811:Support FX Swing interop using public API
Prasanta Sadhukhan
prasanta.sadhukhan at oracle.com
Mon Jul 16 05:01:13 UTC 2018
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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the openjfx-dev
mailing list