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

Kevin Rushforth kevin.rushforth at oracle.com
Thu Jul 12 23:59:02 UTC 2018


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