[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