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

Kevin Rushforth kevin.rushforth at oracle.com
Fri Jul 13 15:46:44 UTC 2018 

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