[11] RFR: Enhancement: JDK-8195811:Support FX Swing interop using public API

[Kevin Rushforth]

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
>>>>
>>>
>>
>