[11] RFR: Enhancement: JDK-8195811:Support FX Swing interop using public API
Prasanta Sadhukhan
prasanta.sadhukhan at oracle.com
Fri Jul 13 09:05:09 UTC 2018
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