Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

+1 -phil. On 06/15/2018 12:31 AM, Prasanta Sadhukhan wrote: Webrev to add @since 11 to jdk.swing.interop classes (only difference from .14) http://cr.openjdk.java.net/~psadhukhan/fxswing.15/ I also tried submitting mach5 job from linux but it is failing to download jib (although I

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Great! Thanks Prasanta. Mandy On 6/15/18 5:42 AM, Prasanta Sadhukhan wrote: I confirm jdk.unsupported.desktop is not present in doc build. Regards Prasanta

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

On 6/15/2018 1:01 PM, Prasanta Sadhukhan wrote: Webrev to add @since 11 to jdk.swing.interop classes (only difference from .14) http://cr.openjdk.java.net/~psadhukhan/fxswing.15/ I also tried submitting mach5 job from linux but it is failing to download jib (although I candownload from

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Thanks for confirming that. Mandy On 6/14/18 3:53 PM, Kevin Rushforth wrote: I verified on an earlier version of the patch that it wasn't in the docs, but it would be good for Prasanta to double-check. -- Kevin On 6/14/2018 1:29 PM, Phil Race wrote: you surely mean @since 11 I believe it

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

I verified on an earlier version of the patch that it wasn't in the docs, but it would be good for Prasanta to double-check. -- Kevin On 6/14/2018 1:29 PM, Phil Race wrote: you surely mean @since 11 I believe it has been verified that it is excluded from the docs build .. right Prasanta ?

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

On 6/14/18 1:29 PM, Phil Race wrote: you surely mean @since 11 Oops typo. Yes @since 11 Mandy

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

you surely mean @since 11 I believe it has been verified that it is excluded from the docs build .. right Prasanta ? -phil On 06/14/2018 01:26 PM, mandy chung wrote: I reviewed the module-info.java change. @since 12 is missing in jdk.unsupported.desktop module-info.java Otherwise it's

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

I reviewed the module-info.java change. @since 12 is missing in jdk.unsupported.desktop module-info.java Otherwise it's fine. Does the docs build exclude jdk.unsupported.desktop? You might have confirmed that that I missed. Mandy On 6/13/18 12:48 AM, Prasanta Sadhukhan wrote: Hi Phil,

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

+1 from me for the JDK changes. -phil. On 06/13/2018 06:04 PM, Kevin Rushforth wrote: The JDK changes look good to me, and as far as I have tested them, it works for me. I haven't tried Drag and Drop yet with the latest interface changes. To repeat something I've mentioned before, just so

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

The JDK changes look good to me, and as far as I have tested them, it works for me. I haven't tried Drag and Drop yet with the latest interface changes. To repeat something I've mentioned before, just so there is no confusion, the FX side of the changes are intended to be a proof of concept

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Hi Phil, All Please find modified webrev taking care of streamlining SwingInteropUtils class and handling of native window handle in LightweightFrameWrapper class by using JNI call in FX http://cr.openjdk.java.net/~psadhukhan/fxswing.14/ Corresponding CSR:

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Some follow up comments. On 5/10/18, 5:49 PM, Philip Race wrote: I've looked over the Swing code. No time to actually try it out so I can only comment on what I see. I am not sure what I think about class docs like "This class wraps .." I know no javadoc is generated but this module is

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

PS .. there should be some tests on the JDK side that don't use FX Or at least a reasoned explanation as to why this isn't possible. -phil. On 5/10/18, 5:49 PM, Philip Race wrote: I've looked over the Swing code. No time to actually try it out so I can only comment on what I see. I am not

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

I've looked over the Swing code. No time to actually try it out so I can only comment on what I see. I am not sure what I think about class docs like "This class wraps .." I know no javadoc is generated but this module is exported and probably should say something less specific. In general

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

I confirm that this fixes the issue. The interface change to make the two static methods e instance methods is a good change in any case. I am not a Swing "R"eviewer, but the .13 webrev gets a +1 from me. -- Kevin On 5/10/2018 8:20 AM, Prasanta Sadhukhan wrote: Hi Kevin,All, Please find

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Hi Kevin,All, Please find the modified webrev fixing this #1 issue http://cr.openjdk.java.net/~psadhukhan/fxswing.13/ via change in jdk/swing/interop/DropTargetContextWrapper.java#setDropTargetContext and FXDnD.java#postDropTargetEvent For me, #2 works, #3 doesn't work even now due to

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Hi Prasanta, The API looks good now. All of our automated tests work (except for the ones with a security manager due to JDK-8202451 ). The only functional problem that I see is that Drag and Drop onto a SwingNode doesn't work. We need to

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

I just checked, and it looks like the build doesn't generate docs for the jdk.unsupported.desktop module. So the issue of doclint warnings is probably a moot point. -- Kevin On 5/9/2018 7:42 AM, Philip Race wrote: Yes, they (the methods mentioning Peer) should be renamed. Qn. to Mandy &

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

On 5/9/18 7:48 AM, Alan Bateman wrote: On 09/05/2018 15:42, Philip Race wrote: : Qn. to Mandy & Alan : it seems there is no need to mention this module in make/common/Modules.gmk in order to get it built, but is there any advantage in doing so ? I mean without it, there is no conscious

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

On 09/05/2018 15:42, Philip Race wrote: : Qn. to Mandy & Alan : it seems there is no need to mention this module in make/common/Modules.gmk in order to get it built, but is there any advantage in doing so ? I mean without it, there is no conscious listing of it as a module nor classification

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Yes, they (the methods mentioning Peer) should be renamed. Qn. to Mandy & Alan : it seems there is no need to mention this module in make/common/Modules.gmk in order to get it built, but is there any advantage in doing so ? I mean without it, there is no conscious listing of it as a module nor

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Modified webrev to cater to this http://cr.openjdk.java.net/~psadhukhan/fxswing.12/ Regards Prasanta On 5/9/2018 5:58 PM, Kevin Rushforth wrote: The following can also be abstract: LightweightContentWrapper:   getComponent, createDragGestureRecognizer, createDragSourceContextPeer

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

The following can also be abstract: LightweightContentWrapper:   getComponent, createDragGestureRecognizer, createDragSourceContextPeer DropTargetContextWrapper:   getTargetActions, getDropTarget, getTransferDataFlavors, getTransferable, isTransferableJVMLocal DispatcherWrapper:  

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Modified webrev to cater to these 3 observations http://cr.openjdk.java.net/~psadhukhan/fxswing.11/ Regards Prasanta On 5/9/2018 5:03 AM, Kevin Rushforth wrote: The module definition for jdk.unsupported.desktop and the changes to java.desktop look fine. In reviewing the jdk.swing.interop

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

The module definition for jdk.unsupported.desktop and the changes to java.desktop look fine. In reviewing the jdk.swing.interop API, I have the following suggestions / observations: 1. DispatcherWrapper, DragSourceContextWrapper, DropTargetContextWrapper, and LightweightContentWrapper can

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Exactly. The current naming was recommended by the jigsaw team, and I fully agree. -- Kevin On 5/8/2018 12:53 AM, Prasanta Sadhukhan wrote: We have discussed this internally and we believe that "unsupportedness" is the critical property here so it justifies grouping them in the same

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

On 08/05/2018 06:51, Prasanta Sadhukhan wrote: Modified webrev to rename to InteropProviderImpl http://cr.openjdk.java.net/~psadhukhan/fxswing.10/ This looks okay to me. -Alan

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

We have discussed this internally and we believe that "unsupportedness" is the critical property here so it justifies grouping them in the same "unsupported" namespace rather than desktop namespace. Regards Prasanta On 5/8/2018 12:56 PM, Ali Ebrahimi wrote: Hi, What about "

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Hi, What about " jdk.desktop.unsupported" for new module name? On Tue, May 8, 2018 at 10:21 AM, Prasanta Sadhukhan < prasanta.sadhuk...@oracle.com> wrote: > Modified webrev to rename to InteropProviderImpl > > http://cr.openjdk.java.net/~psadhukhan/fxswing.10/ > > Regards > Prasanta > > On

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Modified webrev to rename to InteropProviderImpl http://cr.openjdk.java.net/~psadhukhan/fxswing.10/ Regards Prasanta On 5/7/2018 8:35 PM, Kevin Rushforth wrote: That name seems good to me. -- Kevin On 5/7/2018 8:01 AM, Prasanta Sadhukhan wrote: Would InteropProviderImpl sound good?

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

On 5/7/18 2:26 AM, Prasanta Sadhukhan wrote: Modified webrev to use InteropProvider http://cr.openjdk.java.net/~psadhukhan/fxswing.9/ This version looks good.   InteropProviderImpl name is okay with me. Mandy

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

That name seems good to me. -- Kevin On 5/7/2018 8:01 AM, Prasanta Sadhukhan wrote: Would InteropProviderImpl sound good? Regards Prasanta On 5/7/2018 8:27 PM, Alan Bateman wrote: On 07/05/2018 10:26, Prasanta Sadhukhan wrote: : Modified webrev to use InteropProvider

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Would InteropProviderImpl sound good? Regards Prasanta On 5/7/2018 8:27 PM, Alan Bateman wrote: On 07/05/2018 10:26, Prasanta Sadhukhan wrote: : Modified webrev to use InteropProvider http://cr.openjdk.java.net/~psadhukhan/fxswing.9/ This looks okay although for consistent then InteropImpl

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

On 07/05/2018 10:26, Prasanta Sadhukhan wrote: : Modified webrev to use InteropProvider http://cr.openjdk.java.net/~psadhukhan/fxswing.9/ This looks okay although for consistent then InteropImpl could be renamed too. -Alan

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

On 5/6/2018 1:15 PM, Alan Bateman wrote: On 05/05/2018 16:20, Prasanta Sadhukhan wrote: Updated webrev to modify java.desktop module-info.java (only difference between webrev7 and this) to remove the duplicate exports of sun.awt so we will have now exports sun.awt to    

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

On 05/05/2018 16:20, Prasanta Sadhukhan wrote: Updated webrev to modify java.desktop module-info.java (only difference between webrev7 and this) to remove the duplicate exports of sun.awt so we will have now exports sun.awt to     jdk.accessibility,     jdk.unsupported.desktop;

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Hi Prasanta, If @override is not added, then it will cause recursion between > LightweightContent.java:130 and LightweightContent.java:187 I don't understand how the existence or lack thereof of an @Override can cause or prevent recursion (bar reflection). Since all the methods of the interface

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Updated webrev to modify java.desktop module-info.java (only difference between webrev7 and this) to remove the duplicate exports of sun.awt so we will have now exports sun.awt to     jdk.accessibility,     jdk.unsupported.desktop; http://cr.openjdk.java.net/~psadhukhan/fxswing.8/

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Hi All, I have tried to address all comments from Nir, Phil, Mandy and Kevin got so far in this webrev http://cr.openjdk.java.net/~psadhukhan/fxswing.7 >>In SwingNode, lines 870 -883, is there a reason that only the first method uncommented @Override? I don't understand the comment that

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

My quick comments: 1. The changes to java.desktop module-info.java won't compile when applied to jdk-client, since there is already a qualified export of the sun.swing package to another internal class. Can you update your patch to be based off of jdk-client? I note that it requires a small

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

jdk.unsupported.desktop is defined to the application class loader which I think it's fine as FX modules are defined to the same class loader. I noticed this, too, during my testing this morning. It still fails when the security manager is enabled. We already have a bug filed for this:

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

I skimmed through the sources.  It's good to see that this patch is straight forward.  A couple of comments: jdk.unsupported.desktop is defined to the application class loader which I think it's fine as FX modules are defined to the same class loader. I expect

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Two quick comments. 1) I'd like "Peer" removed from all the exported API. 2) It is probably stable enough now that you can start to add some javadoc. -phil. On 05/04/2018 05:00 AM, Prasanta Sadhukhan wrote: Hi All, Please review an enhancement to remove the tight coupling of JDK internal

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Hi Prasanta, In SwingNode, lines 870 -883, is there a reason that only the first method uncommented @Override? I don't understand the comment that wants to skip @Override. Some minor code comments: * LightweightFrameWrapper#isCompEqual: you can skip the 'if' and just 'return c !=

Re: [11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Thanks, Prasanta. As an additional note to reviewers: The JDK portion of this change (JDK-8202199) is the subject for this review. The FX webrev is enough to be able to test the JDK side, but will need additional refactoring (to allow it to continue to build / run with JDK 10) before being

[11] RFR JDK-8202199 : Provide public, unsupported API for FX Swing interop

Hi All, Please review an enhancement to remove the tight coupling of JDK internal class from FX so that when javafx.* modules are removed from Openjdk build in jdk11, FX in general, and fx swing interop, in particular, can still build and function. Right now, FX uses 6 jdk internal packages