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

[Phil Race]

+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 candownload from browser and ping 
java.se.oracle.com)
~/Downloads/mach5-1.0.1865-distribution/bin/mach5 --remote-build 
--email prasanta.sadhuk...@oracle.com

Mach 5 Health State: green

Creating job description... done
Creating build ID... 2018-06-15-0626444.prasanta.sadhukhan.open
Publishing source using JIB... 
[2018-06-15T06:26:42,870Z][INFO][main][c.o.j.s.c.SparkyClient] 
Downloading JIB bootstrap script
Failed to download 
https://java.se.oracle.com/artifactory/jdk-virtual/com/oracle/java/jib/jib/3.0-SNAPSHOT/jib-3.0-SNAPSHOT.jib.sh.gz


I am trying from windows but Phil told windows build does not probably 
built docs.

Are there any easier alternative to verify the doc build?

Regards
Prasanta
On 6/15/2018 1:56 AM, mandy chung wrote:
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, 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: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

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

[mandy chung]

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

[Prasanta Sadhukhan]

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 browser and ping 
java.se.oracle.com)
~/Downloads/mach5-1.0.1865-distribution/bin/mach5 --remote-build 
--email prasanta.sadhuk...@oracle.com

Mach 5 Health State: green

Creating job description... done
Creating build ID... 2018-06-15-0626444.prasanta.sadhukhan.open
Publishing source using JIB... 
[2018-06-15T06:26:42,870Z][INFO][main][c.o.j.s.c.SparkyClient] 
Downloading JIB bootstrap script



I am trying from windows but Phil told windows build does not probably 
built docs.

Are there any easier alternative to verify the doc build?


I confirm jdk.unsupported.desktop is not present in doc build.

Regards
Prasanta

Regards
Prasanta
On 6/15/2018 1:56 AM, mandy chung wrote:
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, 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: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

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

[mandy chung]

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 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 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, 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: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

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

[Kevin Rushforth]

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 ?


-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 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, 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: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

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

[mandy chung]

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

[Phil Race]

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 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, 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: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

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

[mandy chung]

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, 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: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

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

[Phil Race]

+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 there is no 
confusion, the FX side of the changes are intended to be a proof of 
concept to test the JDK side at this point. They will need refactoring 
before they can go in, so that FX will still be buildable and runnable 
with JDK 10.


-- Kevin


On 6/13/2018 12:48 AM, Prasanta Sadhukhan wrote:


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: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta
On 5/30/2018 4:13 AM, Philip Race wrote:

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 exported and 
probably should say something

less specific.

In general I really have to hold my nose when looking at this whole 
module and I

find it distasteful to endorse it.

I really don't like all the things SwingInterOpUtils.java does.


Specific things we should look at in this file
- getDefaultScaleX/Y .. JDK 9 will set a transform on the graphics that
is passed to JComponent.paintComponent() .. so I wonder if we really
need this internal API. I doubt any other code is updating the transform
in a way that would make it a problem so the information you get 
should be valid.


The code to get the data array for the raster and associated information
can be extracted via standard API like this :

DataBuffer  db = BufferedImage.getRaster().getDataBuffer();
if (db instanceof DataBufferInt) {
 data = (DataBufferInt)db.getData();
}

So all of those can be fixed.

The code that accepts a native window handle maybe should require it 
is accessed via JNI ...
It could still be unsupported, but it would be package private or 
similar.

If you have a native ID, then you are already using native code.
This sidesteps any questions about the stability of such an API which
accepts a native resource.

The code that finds a component by location is probably harmless ...

I'm less sure about the event posting and focus grabbing support.
I'd like to have Sergey comment on those.

the DnD code is too much for me to examine in detail.

I worry we are creating something we'll come to regret here.
The "unsupportedness" needs to be backed up by 
deprecated-for-removal being included

today so we can get rid of it the next release if we want to.


I've looked at jdk.unsupported and they don't seem to have 
deprecation tags so

maybe that isn't an issue.

-phil


No @since tags anywhere

-phil.

On 5/10/18, 8:32 AM, Kevin Rushforth wrote:
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 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 JDK-8141391 
 and #4 works 
for me.


Regards
Prasanta
On 5/9/2018 11:29 PM, Kevin Rushforth wrote:

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 make sure that we test 
the following four cases:


1. Drag / drop onto a Swing component in a SwingNode
2. Drag / drop from a Swing component in a SwingNode
3. Drag / drop onto a JavaFX control in a JFXPanel
4. Drag / drop from a JavaFX control in a JFXPanel

So far I only tried the first one; the others still need to be 
validated.


-- Kevin



On 5/9/2018 7:14 AM, Prasanta Sadhukhan wrote:

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


DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, 

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

[Kevin Rushforth]

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 to test the JDK side at this point. They will need refactoring 
before they can go in, so that FX will still be buildable and runnable 
with JDK 10.


-- Kevin


On 6/13/2018 12:48 AM, Prasanta Sadhukhan wrote:


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: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta
On 5/30/2018 4:13 AM, Philip Race wrote:

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 exported and 
probably should say something

less specific.

In general I really have to hold my nose when looking at this whole 
module and I

find it distasteful to endorse it.

I really don't like all the things SwingInterOpUtils.java does.


Specific things we should look at in this file
- getDefaultScaleX/Y .. JDK 9 will set a transform on the graphics that
is passed to JComponent.paintComponent() .. so I wonder if we really
need this internal API. I doubt any other code is updating the transform
in a way that would make it a problem so the information you get 
should be valid.


The code to get the data array for the raster and associated information
can be extracted via standard API like this :

DataBuffer  db = BufferedImage.getRaster().getDataBuffer();
if (db instanceof DataBufferInt) {
 data = (DataBufferInt)db.getData();
}

So all of those can be fixed.

The code that accepts a native window handle maybe should require it 
is accessed via JNI ...
It could still be unsupported, but it would be package private or 
similar.

If you have a native ID, then you are already using native code.
This sidesteps any questions about the stability of such an API which
accepts a native resource.

The code that finds a component by location is probably harmless ...

I'm less sure about the event posting and focus grabbing support.
I'd like to have Sergey comment on those.

the DnD code is too much for me to examine in detail.

I worry we are creating something we'll come to regret here.
The "unsupportedness" needs to be backed up by 
deprecated-for-removal being included

today so we can get rid of it the next release if we want to.


I've looked at jdk.unsupported and they don't seem to have 
deprecation tags so

maybe that isn't an issue.

-phil


No @since tags anywhere

-phil.

On 5/10/18, 8:32 AM, Kevin Rushforth wrote:
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 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 JDK-8141391 
 and #4 works 
for me.


Regards
Prasanta
On 5/9/2018 11:29 PM, Kevin Rushforth wrote:

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 make sure that we test the 
following four cases:


1. Drag / drop onto a Swing component in a SwingNode
2. Drag / drop from a Swing component in a SwingNode
3. Drag / drop onto a JavaFX control in a JFXPanel
4. Drag / drop from a JavaFX control in a JFXPanel

So far I only tried the first one; the others still need to be 
validated.


-- Kevin



On 5/9/2018 7:14 AM, Prasanta Sadhukhan wrote:

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


DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, createSecondaryLoop

The rest looks good to me (although I still see two public 
methods with "Peer" in 

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

[Prasanta Sadhukhan]

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: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta
On 5/30/2018 4:13 AM, Philip Race wrote:

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 exported and 
probably should say something

less specific.

In general I really have to hold my nose when looking at this whole 
module and I

find it distasteful to endorse it.

I really don't like all the things SwingInterOpUtils.java does.


Specific things we should look at in this file
- getDefaultScaleX/Y .. JDK 9 will set a transform on the graphics that
is passed to JComponent.paintComponent() .. so I wonder if we really
need this internal API. I doubt any other code is updating the transform
in a way that would make it a problem so the information you get 
should be valid.


The code to get the data array for the raster and associated information
can be extracted via standard API like this :

DataBuffer  db = BufferedImage.getRaster().getDataBuffer();
if (db instanceof DataBufferInt) {
 data = (DataBufferInt)db.getData();
}

So all of those can be fixed.

The code that accepts a native window handle maybe should require it 
is accessed via JNI ...

It could still be unsupported, but it would be package private or similar.
If you have a native ID, then you are already using native code.
This sidesteps any questions about the stability of such an API which
accepts a native resource.

The code that finds a component by location is probably harmless ...

I'm less sure about the event posting and focus grabbing support.
I'd like to have Sergey comment on those.

the DnD code is too much for me to examine in detail.

I worry we are creating something we'll come to regret here.
The "unsupportedness" needs to be backed up by deprecated-for-removal 
being included

today so we can get rid of it the next release if we want to.


I've looked at jdk.unsupported and they don't seem to have deprecation 
tags so

maybe that isn't an issue.

-phil


No @since tags anywhere

-phil.

On 5/10/18, 8:32 AM, Kevin Rushforth wrote:
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 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 JDK-8141391 
 and #4 works for me.


Regards
Prasanta
On 5/9/2018 11:29 PM, Kevin Rushforth wrote:

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 make sure that we test the 
following four cases:


1. Drag / drop onto a Swing component in a SwingNode
2. Drag / drop from a Swing component in a SwingNode
3. Drag / drop onto a JavaFX control in a JFXPanel
4. Drag / drop from a JavaFX control in a JFXPanel

So far I only tried the first one; the others still need to be 
validated.


-- Kevin



On 5/9/2018 7:14 AM, Prasanta Sadhukhan wrote:

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


DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, createSecondaryLoop

The rest looks good to me (although I still see two public 
methods with "Peer" in the name, so Phil may want those renamed).


-- Kevin


On 5/9/2018 2:14 AM, Prasanta Sadhukhan wrote:

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 API, I have the following 
suggestions / observations:


1. DispatcherWrapper, DragSourceContextWrapper, 

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

[Philip Race]

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 exported and 
probably should say something

less specific.

In general I really have to hold my nose when looking at this whole 
module and I

find it distasteful to endorse it.

I really don't like all the things SwingInterOpUtils.java does.


Specific things we should look at in this file
- getDefaultScaleX/Y .. JDK 9 will set a transform on the graphics that
is passed to JComponent.paintComponent() .. so I wonder if we really
need this internal API. I doubt any other code is updating the transform
in a way that would make it a problem so the information you get should 
be valid.


The code to get the data array for the raster and associated information
can be extracted via standard API like this :

DataBuffer  db = BufferedImage.getRaster().getDataBuffer();
if (db instanceof DataBufferInt) {
 data = (DataBufferInt)db.getData();
}

So all of those can be fixed.

The code that accepts a native window handle maybe should require it is 
accessed via JNI ...

It could still be unsupported, but it would be package private or similar.
If you have a native ID, then you are already using native code.
This sidesteps any questions about the stability of such an API which
accepts a native resource.

The code that finds a component by location is probably harmless ...

I'm less sure about the event posting and focus grabbing support.
I'd like to have Sergey comment on those.

the DnD code is too much for me to examine in detail.

I worry we are creating something we'll come to regret here.
The "unsupportedness" needs to be backed up by deprecated-for-removal 
being included

today so we can get rid of it the next release if we want to.


I've looked at jdk.unsupported and they don't seem to have deprecation 
tags so

maybe that isn't an issue.

-phil


No @since tags anywhere

-phil.

On 5/10/18, 8:32 AM, Kevin Rushforth wrote:
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 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 JDK-8141391 
 and #4 works for me.


Regards
Prasanta
On 5/9/2018 11:29 PM, Kevin Rushforth wrote:

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 make sure that we test the 
following four cases:


1. Drag / drop onto a Swing component in a SwingNode
2. Drag / drop from a Swing component in a SwingNode
3. Drag / drop onto a JavaFX control in a JFXPanel
4. Drag / drop from a JavaFX control in a JFXPanel

So far I only tried the first one; the others still need to be 
validated.


-- Kevin



On 5/9/2018 7:14 AM, Prasanta Sadhukhan wrote:

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


DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, createSecondaryLoop

The rest looks good to me (although I still see two public 
methods with "Peer" in the name, so Phil may want those renamed).


-- Kevin


On 5/9/2018 2:14 AM, Prasanta Sadhukhan wrote:

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 API, I have the following 
suggestions / observations:


1. DispatcherWrapper, DragSourceContextWrapper, 
DropTargetContextWrapper, and LightweightContentWrapper can all 
be abstract, along with most of the methods (rather than having 
an empty body return value that is never used).


2. The addNotify method in LightweightFrameWrapper is unused. 
Should be used somewhere? If not, then it can be removed.


The implementation of the new wrapper classes looks OK to me 
with one 

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

[Philip Race]

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 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 I really have to hold my nose when looking at this whole 
module and I

find it distasteful to endorse it.

I really don't like all the things SwingInterOpUtils.java does.

I worry we are creating something we'll come to regret here.
The "unsupportedness" needs to be backed up by deprecated-for-removal 
being included

today so we can get rid of it the next release if we want to.

No @since tags anywhere

-phil.

On 5/10/18, 8:32 AM, Kevin Rushforth wrote:
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 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 JDK-8141391 
 and #4 works for me.


Regards
Prasanta
On 5/9/2018 11:29 PM, Kevin Rushforth wrote:

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 make sure that we test the 
following four cases:


1. Drag / drop onto a Swing component in a SwingNode
2. Drag / drop from a Swing component in a SwingNode
3. Drag / drop onto a JavaFX control in a JFXPanel
4. Drag / drop from a JavaFX control in a JFXPanel

So far I only tried the first one; the others still need to be 
validated.


-- Kevin



On 5/9/2018 7:14 AM, Prasanta Sadhukhan wrote:

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


DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, createSecondaryLoop

The rest looks good to me (although I still see two public 
methods with "Peer" in the name, so Phil may want those renamed).


-- Kevin


On 5/9/2018 2:14 AM, Prasanta Sadhukhan wrote:

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 API, I have the following 
suggestions / observations:


1. DispatcherWrapper, DragSourceContextWrapper, 
DropTargetContextWrapper, and LightweightContentWrapper can all 
be abstract, along with most of the methods (rather than having 
an empty body return value that is never used).


2. The addNotify method in LightweightFrameWrapper is unused. 
Should be used somewhere? If not, then it can be removed.


The implementation of the new wrapper classes looks OK to me 
with one observation that might or might not matter:


3. The behavior of getDefaultScaleX/Y (which is now in 
SwingInteropUtils) has changed in the case where the Graphics 
is not an instance of SunGraphics2D. The former behavior was to 
leave the instance variables X and Y unchanged. The new 
behavior will set them back to 1.0. Maybe this can't happen in 
practice, but it is something to consider.


-- Kevin


On 5/8/2018 3:31 AM, Alan Bateman wrote:

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

[Philip Race]

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 I really have to hold my nose when looking at this whole 
module and I

find it distasteful to endorse it.

I really don't like all the things SwingInterOpUtils.java does.

I worry we are creating something we'll come to regret here.
The "unsupportedness" needs to be backed up by deprecated-for-removal 
being included

today so we can get rid of it the next release if we want to.

No @since tags anywhere

-phil.

On 5/10/18, 8:32 AM, Kevin Rushforth wrote:
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 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 JDK-8141391 
 and #4 works for me.


Regards
Prasanta
On 5/9/2018 11:29 PM, Kevin Rushforth wrote:

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 make sure that we test the 
following four cases:


1. Drag / drop onto a Swing component in a SwingNode
2. Drag / drop from a Swing component in a SwingNode
3. Drag / drop onto a JavaFX control in a JFXPanel
4. Drag / drop from a JavaFX control in a JFXPanel

So far I only tried the first one; the others still need to be 
validated.


-- Kevin



On 5/9/2018 7:14 AM, Prasanta Sadhukhan wrote:

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


DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, createSecondaryLoop

The rest looks good to me (although I still see two public methods 
with "Peer" in the name, so Phil may want those renamed).


-- Kevin


On 5/9/2018 2:14 AM, Prasanta Sadhukhan wrote:

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 API, I have the following 
suggestions / observations:


1. DispatcherWrapper, DragSourceContextWrapper, 
DropTargetContextWrapper, and LightweightContentWrapper can all 
be abstract, along with most of the methods (rather than having 
an empty body return value that is never used).


2. The addNotify method in LightweightFrameWrapper is unused. 
Should be used somewhere? If not, then it can be removed.


The implementation of the new wrapper classes looks OK to me 
with one observation that might or might not matter:


3. The behavior of getDefaultScaleX/Y (which is now in 
SwingInteropUtils) has changed in the case where the Graphics is 
not an instance of SunGraphics2D. The former behavior was to 
leave the instance variables X and Y unchanged. The new behavior 
will set them back to 1.0. Maybe this can't happen in practice, 
but it is something to consider.


-- Kevin


On 5/8/2018 3:31 AM, Alan Bateman wrote:

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

[Kevin Rushforth]

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 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 JDK-8141391 
 and #4 works for me.


Regards
Prasanta
On 5/9/2018 11:29 PM, Kevin Rushforth wrote:

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 make sure that we test the 
following four cases:


1. Drag / drop onto a Swing component in a SwingNode
2. Drag / drop from a Swing component in a SwingNode
3. Drag / drop onto a JavaFX control in a JFXPanel
4. Drag / drop from a JavaFX control in a JFXPanel

So far I only tried the first one; the others still need to be validated.

-- Kevin



On 5/9/2018 7:14 AM, Prasanta Sadhukhan wrote:

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


DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, createSecondaryLoop

The rest looks good to me (although I still see two public methods 
with "Peer" in the name, so Phil may want those renamed).


-- Kevin


On 5/9/2018 2:14 AM, Prasanta Sadhukhan wrote:

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 API, I have the following 
suggestions / observations:


1. DispatcherWrapper, DragSourceContextWrapper, 
DropTargetContextWrapper, and LightweightContentWrapper can all 
be abstract, along with most of the methods (rather than having 
an empty body return value that is never used).


2. The addNotify method in LightweightFrameWrapper is unused. 
Should be used somewhere? If not, then it can be removed.


The implementation of the new wrapper classes looks OK to me with 
one observation that might or might not matter:


3. The behavior of getDefaultScaleX/Y (which is now in 
SwingInteropUtils) has changed in the case where the Graphics is 
not an instance of SunGraphics2D. The former behavior was to 
leave the instance variables X and Y unchanged. The new behavior 
will set them back to 1.0. Maybe this can't happen in practice, 
but it is something to consider.


-- Kevin


On 5/8/2018 3:31 AM, Alan Bateman wrote:

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

[Prasanta Sadhukhan]

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 JDK-8141391 
 and #4 works for me.


Regards
Prasanta
On 5/9/2018 11:29 PM, Kevin Rushforth wrote:

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 make sure that we test the 
following four cases:


1. Drag / drop onto a Swing component in a SwingNode
2. Drag / drop from a Swing component in a SwingNode
3. Drag / drop onto a JavaFX control in a JFXPanel
4. Drag / drop from a JavaFX control in a JFXPanel

So far I only tried the first one; the others still need to be validated.

-- Kevin



On 5/9/2018 7:14 AM, Prasanta Sadhukhan wrote:

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


DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, createSecondaryLoop

The rest looks good to me (although I still see two public methods 
with "Peer" in the name, so Phil may want those renamed).


-- Kevin


On 5/9/2018 2:14 AM, Prasanta Sadhukhan wrote:

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 API, I have the following 
suggestions / observations:


1. DispatcherWrapper, DragSourceContextWrapper, 
DropTargetContextWrapper, and LightweightContentWrapper can all be 
abstract, along with most of the methods (rather than having an 
empty body return value that is never used).


2. The addNotify method in LightweightFrameWrapper is unused. 
Should be used somewhere? If not, then it can be removed.


The implementation of the new wrapper classes looks OK to me with 
one observation that might or might not matter:


3. The behavior of getDefaultScaleX/Y (which is now in 
SwingInteropUtils) has changed in the case where the Graphics is 
not an instance of SunGraphics2D. The former behavior was to leave 
the instance variables X and Y unchanged. The new behavior will 
set them back to 1.0. Maybe this can't happen in practice, but it 
is something to consider.


-- Kevin


On 5/8/2018 3:31 AM, Alan Bateman wrote:

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

[Kevin Rushforth]

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 make sure that we test the following 
four cases:


1. Drag / drop onto a Swing component in a SwingNode
2. Drag / drop from a Swing component in a SwingNode
3. Drag / drop onto a JavaFX control in a JFXPanel
4. Drag / drop from a JavaFX control in a JFXPanel

So far I only tried the first one; the others still need to be validated.

-- Kevin



On 5/9/2018 7:14 AM, Prasanta Sadhukhan wrote:

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

DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, createSecondaryLoop

The rest looks good to me (although I still see two public methods 
with "Peer" in the name, so Phil may want those renamed).


-- Kevin


On 5/9/2018 2:14 AM, Prasanta Sadhukhan wrote:

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 API, I have the following 
suggestions / observations:


1. DispatcherWrapper, DragSourceContextWrapper, 
DropTargetContextWrapper, and LightweightContentWrapper can all be 
abstract, along with most of the methods (rather than having an 
empty body return value that is never used).


2. The addNotify method in LightweightFrameWrapper is unused. 
Should be used somewhere? If not, then it can be removed.


The implementation of the new wrapper classes looks OK to me with 
one observation that might or might not matter:


3. The behavior of getDefaultScaleX/Y (which is now in 
SwingInteropUtils) has changed in the case where the Graphics is 
not an instance of SunGraphics2D. The former behavior was to leave 
the instance variables X and Y unchanged. The new behavior will set 
them back to 1.0. Maybe this can't happen in practice, but it is 
something to consider.


-- Kevin


On 5/8/2018 3:31 AM, Alan Bateman wrote:

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

[Kevin Rushforth]

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 & 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 of it.

Another thing that Kevin & I touched on in conversation is that it seems
doclint fail on warning must be disabled by default .. and I suppose that
is what we want here since documentation is minimal.

-phil.

On 5/9/18, 5:28 AM, Kevin Rushforth wrote:

The following can also be abstract:

LightweightContentWrapper:
  getComponent, createDragGestureRecognizer, createDragSourceContextPeer

DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, createSecondaryLoop

The rest looks good to me (although I still see two public methods 
with "Peer" in the name, so Phil may want those renamed).


-- Kevin


On 5/9/2018 2:14 AM, Prasanta Sadhukhan wrote:

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 API, I have the following 
suggestions / observations:


1. DispatcherWrapper, DragSourceContextWrapper, 
DropTargetContextWrapper, and LightweightContentWrapper can all be 
abstract, along with most of the methods (rather than having an 
empty body return value that is never used).


2. The addNotify method in LightweightFrameWrapper is unused. 
Should be used somewhere? If not, then it can be removed.


The implementation of the new wrapper classes looks OK to me with 
one observation that might or might not matter:


3. The behavior of getDefaultScaleX/Y (which is now in 
SwingInteropUtils) has changed in the case where the Graphics is 
not an instance of SunGraphics2D. The former behavior was to leave 
the instance variables X and Y unchanged. The new behavior will set 
them back to 1.0. Maybe this can't happen in practice, but it is 
something to consider.


-- Kevin


On 5/8/2018 3:31 AM, Alan Bateman wrote:

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

[mandy chung]

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

it as a module nor classification of it.
Right, it's not necessary to list modules that are defined to the 
application class loader and are only in the JDK image. There's a 
comment at the top of the make file on this but it probably could be a 
bit clearer.


The build determines the modules to be compiled from the source 
directory per the modular source layout.  make/common/Modules.gmk serves 
a few purpose and the relevant one here is the module to class loader 
mapping.  It lists the modules defined to the boot loader and platform 
boot loader.


Mandy

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

[Alan Bateman]

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 of it.
Right, it's not necessary to list modules that are defined to the 
application class loader and are only in the JDK image. There's a 
comment at the top of the make file on this but it probably could be a 
bit clearer.


-Alan

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

[Philip Race]

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 classification of it.

Another thing that Kevin & I touched on in conversation is that it seems
doclint fail on warning must be disabled by default .. and I suppose that
is what we want here since documentation is minimal.

-phil.

On 5/9/18, 5:28 AM, Kevin Rushforth wrote:

The following can also be abstract:

LightweightContentWrapper:
  getComponent, createDragGestureRecognizer, createDragSourceContextPeer

DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, createSecondaryLoop

The rest looks good to me (although I still see two public methods 
with "Peer" in the name, so Phil may want those renamed).


-- Kevin


On 5/9/2018 2:14 AM, Prasanta Sadhukhan wrote:

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 API, I have the following 
suggestions / observations:


1. DispatcherWrapper, DragSourceContextWrapper, 
DropTargetContextWrapper, and LightweightContentWrapper can all be 
abstract, along with most of the methods (rather than having an 
empty body return value that is never used).


2. The addNotify method in LightweightFrameWrapper is unused. Should 
be used somewhere? If not, then it can be removed.


The implementation of the new wrapper classes looks OK to me with 
one observation that might or might not matter:


3. The behavior of getDefaultScaleX/Y (which is now in 
SwingInteropUtils) has changed in the case where the Graphics is not 
an instance of SunGraphics2D. The former behavior was to leave the 
instance variables X and Y unchanged. The new behavior will set them 
back to 1.0. Maybe this can't happen in practice, but it is 
something to consider.


-- Kevin


On 5/8/2018 3:31 AM, Alan Bateman wrote:

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

[Prasanta Sadhukhan]

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

DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, createSecondaryLoop

The rest looks good to me (although I still see two public methods 
with "Peer" in the name, so Phil may want those renamed).


-- Kevin


On 5/9/2018 2:14 AM, Prasanta Sadhukhan wrote:

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 API, I have the following 
suggestions / observations:


1. DispatcherWrapper, DragSourceContextWrapper, 
DropTargetContextWrapper, and LightweightContentWrapper can all be 
abstract, along with most of the methods (rather than having an 
empty body return value that is never used).


2. The addNotify method in LightweightFrameWrapper is unused. Should 
be used somewhere? If not, then it can be removed.


The implementation of the new wrapper classes looks OK to me with 
one observation that might or might not matter:


3. The behavior of getDefaultScaleX/Y (which is now in 
SwingInteropUtils) has changed in the case where the Graphics is not 
an instance of SunGraphics2D. The former behavior was to leave the 
instance variables X and Y unchanged. The new behavior will set them 
back to 1.0. Maybe this can't happen in practice, but it is 
something to consider.


-- Kevin


On 5/8/2018 3:31 AM, Alan Bateman wrote:

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

[Kevin Rushforth]

The following can also be abstract:

LightweightContentWrapper:
  getComponent, createDragGestureRecognizer, createDragSourceContextPeer

DropTargetContextWrapper:
  getTargetActions, getDropTarget, getTransferDataFlavors, 
getTransferable, isTransferableJVMLocal


DispatcherWrapper:
  isDispatchThread, createSecondaryLoop

The rest looks good to me (although I still see two public methods with 
"Peer" in the name, so Phil may want those renamed).


-- Kevin


On 5/9/2018 2:14 AM, Prasanta Sadhukhan wrote:

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 API, I have the following 
suggestions / observations:


1. DispatcherWrapper, DragSourceContextWrapper, 
DropTargetContextWrapper, and LightweightContentWrapper can all be 
abstract, along with most of the methods (rather than having an empty 
body return value that is never used).


2. The addNotify method in LightweightFrameWrapper is unused. Should 
be used somewhere? If not, then it can be removed.


The implementation of the new wrapper classes looks OK to me with one 
observation that might or might not matter:


3. The behavior of getDefaultScaleX/Y (which is now in 
SwingInteropUtils) has changed in the case where the Graphics is not 
an instance of SunGraphics2D. The former behavior was to leave the 
instance variables X and Y unchanged. The new behavior will set them 
back to 1.0. Maybe this can't happen in practice, but it is something 
to consider.


-- Kevin


On 5/8/2018 3:31 AM, Alan Bateman wrote:

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

[Prasanta Sadhukhan]

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 API, I have the following 
suggestions / observations:


1. DispatcherWrapper, DragSourceContextWrapper, 
DropTargetContextWrapper, and LightweightContentWrapper can all be 
abstract, along with most of the methods (rather than having an empty 
body return value that is never used).


2. The addNotify method in LightweightFrameWrapper is unused. Should 
be used somewhere? If not, then it can be removed.


The implementation of the new wrapper classes looks OK to me with one 
observation that might or might not matter:


3. The behavior of getDefaultScaleX/Y (which is now in 
SwingInteropUtils) has changed in the case where the Graphics is not 
an instance of SunGraphics2D. The former behavior was to leave the 
instance variables X and Y unchanged. The new behavior will set them 
back to 1.0. Maybe this can't happen in practice, but it is something 
to consider.


-- Kevin


On 5/8/2018 3:31 AM, Alan Bateman wrote:

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

[Kevin Rushforth]

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 all be 
abstract, along with most of the methods (rather than having an empty 
body return value that is never used).


2. The addNotify method in LightweightFrameWrapper is unused. Should be 
used somewhere? If not, then it can be removed.


The implementation of the new wrapper classes looks OK to me with one 
observation that might or might not matter:


3. The behavior of getDefaultScaleX/Y (which is now in 
SwingInteropUtils) has changed in the case where the Graphics is not an 
instance of SunGraphics2D. The former behavior was to leave the instance 
variables X and Y unchanged. The new behavior will set them back to 1.0. 
Maybe this can't happen in practice, but it is something to consider.


-- Kevin


On 5/8/2018 3:31 AM, Alan Bateman wrote:

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

[Kevin Rushforth]

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 "unsupported" namespace rather than desktop 
namespace.


Regards
Prasanta
On 5/8/2018 12:56 PM, Ali Ebrahimi wrote:

Hi,
What about " jdk.desktop.unsupported" for new module name?

On Tue, May 8, 2018 at 10:21 AM, Prasanta Sadhukhan 
> wrote:


    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?

    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 could be renamed too.

    -Alan







--

Best Regards,
Ali Ebrahimi

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

[Alan Bateman]

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

[Prasanta Sadhukhan]

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 " jdk.desktop.unsupported" for new module name?

On Tue, May 8, 2018 at 10:21 AM, Prasanta Sadhukhan 
> 
wrote:


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?

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 could be renamed too.

-Alan







--

Best Regards,
Ali Ebrahimi

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

[Ali Ebrahimi]

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 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?
>>>
>>> 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 could be
 renamed too.

 -Alan

>>>
>>>
>>
>


-- 

Best Regards,
Ali Ebrahimi

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

[Prasanta Sadhukhan]

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?

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 could be 
renamed too.


-Alan

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

[mandy chung]

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

[Kevin Rushforth]

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

[Prasanta Sadhukhan]

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 could be 
renamed too.


-Alan

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

[Alan Bateman]

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

[Prasanta Sadhukhan]

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
    jdk.accessibility,
    jdk.unsupported.desktop;

http://cr.openjdk.java.net/~psadhukhan/fxswing.8/


At a high-level, the module declarations look good and using service 
binding to ensure that the jdk.unsupported.desktop module is resolved 
looks good. The only thing is the name of the service provider 
interface, it might be clearer if you use InteropProvider rather than 
InteropInterface.



Modified webrev to use InteropProvider
http://cr.openjdk.java.net/~psadhukhan/fxswing.9/
I assume dependencies/java.desktop/module-info.java.extra will be 
removed once the remaining dependency on sun.print goes away.



Yes, that's the plan.

Regards
Prasanta

-Alan

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

[Alan Bateman]

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;

http://cr.openjdk.java.net/~psadhukhan/fxswing.8/


At a high-level, the module declarations look good and using service 
binding to ensure that the jdk.unsupported.desktop module is resolved 
looks good. The only thing is the name of the service provider 
interface, it might be clearer if you use InteropProvider rather than 
InteropInterface.


I assume dependencies/java.desktop/module-info.java.extra will be 
removed once the remaining dependency on sun.print goes away.


-Alan

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

[Nir Lisker]

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 are overriden in SwingNode (regardless of the annotation) their
default implementation is not used, and SwingNode is recursion-free.
What is more worrisome is that if the interface methods are not overriden
in some other implementing class, there will be infinite recursion, but
this a fault in the design of the interface (I'd say), and out of scope for
this issue. On the up side, one of the methods in the interface is
deprecated, so it didn't go completely under the radar.

Personally, I force the use of @Override where applicable since it can only
help and I only brought this up since you uncommented 1 of the annotations
and left 2 others.

Thanks,
Nir

On Sat, May 5, 2018 at 6:20 PM, Prasanta Sadhukhan <
prasanta.sadhuk...@oracle.com> 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;
>
> http://cr.openjdk.java.net/~psadhukhan/fxswing.8/
>
> Regards
> Prasanta
>
> On 5/5/2018 2:28 PM, Prasanta Sadhukhan wrote:
>
>> 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 wants to
>> skip @Override.
>> If @override is not added, then it will cause recursion between
>> LightweightContent.java:130 and LightweightContent.java:187 thereby causing
>> StackOverflowError. I am also not sure about the comment of skipping
>> @Override but I have removed it from this webrev.
>>
>> Regards
>> Prasanta
>> On 5/5/2018 4:32 AM, Kevin Rushforth wrote:
>>
>>> 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 patch to be
>>> able to build FX with JDK 11 (it's a known gradle issue), but it's not hard
>>> to do.
>>>
>>> 2. As I mentioned in an off-line email, the final version of the
>>> javafx.swing patch will need to use 'requires static
>>> jdk.unsupported.desktop' in its module-info.java, so that it will still run
>>> on JDK 10 EA. This means that you will need to ensure that
>>> jdk.unsupported.desktop is loaded via a service loader as discussed. This
>>> will not affect the public API at all, so that can proceed in parallel, but
>>> this needs to be fixed.
>>>
>>> 3. As I mentioned in my earlier reply to Mandy's comment, Swing interop
>>> apps will fail if a security manager is installed, because the
>>> jdk.unsupported.desktop classes are loaded by the app class loader. This
>>> can be sorted out later as a follow-on bug, with permissions added to the
>>> default policy file, etc.
>>>
>>> 4. As Phil mentioned, some docs would be good. They don't need to be all
>>> that detailed, since it isn't intended for use by applications, and will
>>> not be included in the published API docs bundle (i.e., it should be
>>> excluded from "make docs").
>>>
>>> I will do a more detailed review early next week.
>>>
>>> -- Kevin
>>>
>>> On 5/4/2018 1:53 PM, Phil Race wrote:
>>>
 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 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
> sun.swing, sun.awt, java.awt.dnd.peer, sun.awt.dnd, sun.awt.image and
> sun.java2d.
>
> However, the goal is not to use qualified exports of these internal
> packages once FX is removed to be built along with JDK,
>
> The solution will define a new "jdk.unsupported.desktop" module that
> exports public API
> that is intended to be used by the javafx.swing module (but it does so
> with public exports and not qualified exports).
> The idea is the same as jdk.unsupported, in that it is documented as
> being unsupported.
> Further, since it is only intended to be used by javafx.swing, it need
> not be in the 

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

[Prasanta Sadhukhan]

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/

Regards
Prasanta
On 5/5/2018 2:28 PM, Prasanta Sadhukhan wrote:

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 
wants to skip @Override.
If @override is not added, then it will cause recursion between 
LightweightContent.java:130 and LightweightContent.java:187 thereby 
causing StackOverflowError. I am also not sure about the comment of 
skipping @Override but I have removed it from this webrev.


Regards
Prasanta
On 5/5/2018 4:32 AM, Kevin Rushforth wrote:

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 
patch to be able to build FX with JDK 11 (it's a known gradle issue), 
but it's not hard to do.


2. As I mentioned in an off-line email, the final version of the 
javafx.swing patch will need to use 'requires static 
jdk.unsupported.desktop' in its module-info.java, so that it will 
still run on JDK 10 EA. This means that you will need to ensure that 
jdk.unsupported.desktop is loaded via a service loader as discussed. 
This will not affect the public API at all, so that can proceed in 
parallel, but this needs to be fixed.


3. As I mentioned in my earlier reply to Mandy's comment, Swing 
interop apps will fail if a security manager is installed, because 
the jdk.unsupported.desktop classes are loaded by the app class 
loader. This can be sorted out later as a follow-on bug, with 
permissions added to the default policy file, etc.


4. As Phil mentioned, some docs would be good. They don't need to be 
all that detailed, since it isn't intended for use by applications, 
and will not be included in the published API docs bundle (i.e., it 
should be excluded from "make docs").


I will do a more detailed review early next week.

-- Kevin

On 5/4/2018 1:53 PM, Phil Race wrote:

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 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
sun.swing, sun.awt, java.awt.dnd.peer, sun.awt.dnd, sun.awt.image 
and sun.java2d.


However, the goal is not to use qualified exports of these internal 
packages once FX is removed to be built along with JDK,


The solution will define a new "jdk.unsupported.desktop" module 
that exports public API
that is intended to be used by the javafx.swing module (but it does 
so with public exports and not qualified exports).
The idea is the same as jdk.unsupported, in that it is documented 
as being unsupported.
Further, since it is only intended to be used by javafx.swing, it 
need not be in the default module graph.


The module-info.java will look like this:

|module jdk.unsupported.desktop { requires transitive java.desktop; 
exports jdk.swing.interop; } |||


Enhancement: https://bugs.openjdk.java.net/browse/JDK-8202199, 
https://bugs.openjdk.java.net/browse/JDK-8195811

webrev: cr.openjdk.java.net/~psadhukhan/fxswing.6/
CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

||

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

[Prasanta Sadhukhan]

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 wants 
to skip @Override.
If @override is not added, then it will cause recursion between 
LightweightContent.java:130 and LightweightContent.java:187 thereby 
causing StackOverflowError. I am also not sure about the comment of 
skipping @Override but I have removed it from this webrev.


Regards
Prasanta
On 5/5/2018 4:32 AM, Kevin Rushforth wrote:

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 
patch to be able to build FX with JDK 11 (it's a known gradle issue), 
but it's not hard to do.


2. As I mentioned in an off-line email, the final version of the 
javafx.swing patch will need to use 'requires static 
jdk.unsupported.desktop' in its module-info.java, so that it will 
still run on JDK 10 EA. This means that you will need to ensure that 
jdk.unsupported.desktop is loaded via a service loader as discussed. 
This will not affect the public API at all, so that can proceed in 
parallel, but this needs to be fixed.


3. As I mentioned in my earlier reply to Mandy's comment, Swing 
interop apps will fail if a security manager is installed, because the 
jdk.unsupported.desktop classes are loaded by the app class loader. 
This can be sorted out later as a follow-on bug, with permissions 
added to the default policy file, etc.


4. As Phil mentioned, some docs would be good. They don't need to be 
all that detailed, since it isn't intended for use by applications, 
and will not be included in the published API docs bundle (i.e., it 
should be excluded from "make docs").


I will do a more detailed review early next week.

-- Kevin

On 5/4/2018 1:53 PM, Phil Race wrote:

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 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
sun.swing, sun.awt, java.awt.dnd.peer, sun.awt.dnd, sun.awt.image 
and sun.java2d.


However, the goal is not to use qualified exports of these internal 
packages once FX is removed to be built along with JDK,


The solution will define a new "jdk.unsupported.desktop" module that 
exports public API
that is intended to be used by the javafx.swing module (but it does 
so with public exports and not qualified exports).
The idea is the same as jdk.unsupported, in that it is documented as 
being unsupported.
Further, since it is only intended to be used by javafx.swing, it 
need not be in the default module graph.


The module-info.java will look like this:

|module jdk.unsupported.desktop { requires transitive java.desktop; 
exports jdk.swing.interop; } |||


Enhancement: https://bugs.openjdk.java.net/browse/JDK-8202199, 
https://bugs.openjdk.java.net/browse/JDK-8195811

webrev: cr.openjdk.java.net/~psadhukhan/fxswing.6/
CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

||

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

[Kevin Rushforth]

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 patch to 
be able to build FX with JDK 11 (it's a known gradle issue), but it's 
not hard to do.


2. As I mentioned in an off-line email, the final version of the 
javafx.swing patch will need to use 'requires static 
jdk.unsupported.desktop' in its module-info.java, so that it will still 
run on JDK 10 EA. This means that you will need to ensure that 
jdk.unsupported.desktop is loaded via a service loader as discussed. 
This will not affect the public API at all, so that can proceed in 
parallel, but this needs to be fixed.


3. As I mentioned in my earlier reply to Mandy's comment, Swing interop 
apps will fail if a security manager is installed, because the 
jdk.unsupported.desktop classes are loaded by the app class loader. This 
can be sorted out later as a follow-on bug, with permissions added to 
the default policy file, etc.


4. As Phil mentioned, some docs would be good. They don't need to be all 
that detailed, since it isn't intended for use by applications, and will 
not be included in the published API docs bundle (i.e., it should be 
excluded from "make docs").


I will do a more detailed review early next week.

-- Kevin

On 5/4/2018 1:53 PM, Phil Race wrote:

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 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
sun.swing, sun.awt, java.awt.dnd.peer, sun.awt.dnd, sun.awt.image and 
sun.java2d.


However, the goal is not to use qualified exports of these internal 
packages once FX is removed to be built along with JDK,


The solution will define a new "jdk.unsupported.desktop" module that 
exports public API
that is intended to be used by the javafx.swing module (but it does 
so with public exports and not qualified exports).
The idea is the same as jdk.unsupported, in that it is documented as 
being unsupported.
Further, since it is only intended to be used by javafx.swing, it 
need not be in the default module graph.


The module-info.java will look like this:

|module jdk.unsupported.desktop { requires transitive java.desktop; 
exports jdk.swing.interop; } |||


Enhancement: https://bugs.openjdk.java.net/browse/JDK-8202199, 
https://bugs.openjdk.java.net/browse/JDK-8195811

webrev: cr.openjdk.java.net/~psadhukhan/fxswing.6/
CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

||

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

[Kevin Rushforth]

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:


https://bugs.openjdk.java.net/browse/JDK-8202451

I had hoped would be fixed along with the new unsupported API, but I 
agree that this could be fixed later.


-- Kevin


On 5/4/2018 2:08 PM, mandy chung wrote:

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 src/java.base/share/lib/security/default.security will
need to be modified and grant permissions to jdk.unsupported.desktop.
Kevin and I talked about potential issues with running with security
manager enabled.  Maybe it is a separate task to follow up and it
may reveal if any operation needs doPrivileged - that's fine with me.

The docs build starts with a known set of root modules for javadoc
generation.  I believe jdk.unsupported.desktop won't be included
in the docs build which is intended (as it's unsupported).  Can
you verify it?

Nit: jdk.swing.interop.* source files
it'd be good to keep import statements sorted by the names.

Mandy

On 5/4/18 5:00 AM, Prasanta Sadhukhan wrote:

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
sun.swing, sun.awt, java.awt.dnd.peer, sun.awt.dnd, sun.awt.image and 
sun.java2d.


However, the goal is not to use qualified exports of these internal 
packages once FX is removed to be built along with JDK,


The solution will define a new "jdk.unsupported.desktop" module that 
exports public API
that is intended to be used by the javafx.swing module (but it does 
so with public exports and not qualified exports).
The idea is the same as jdk.unsupported, in that it is documented as 
being unsupported.
Further, since it is only intended to be used by javafx.swing, it 
need not be in the default module graph.


The module-info.java will look like this:
|module jdk.unsupported.desktop { requires transitive java.desktop; 
exports jdk.swing.interop; } |||
Enhancement: https://bugs.openjdk.java.net/browse/JDK-8202199, 
https://bugs.openjdk.java.net/browse/JDK-8195811

webrev: cr.openjdk.java.net/~psadhukhan/fxswing.6/
CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta
||

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

[mandy chung]

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 src/java.base/share/lib/security/default.security will
need to be modified and grant permissions to jdk.unsupported.desktop.
Kevin and I talked about potential issues with running with security
manager enabled.  Maybe it is a separate task to follow up and it
may reveal if any operation needs doPrivileged - that's fine with me.

The docs build starts with a known set of root modules for javadoc
generation.  I believe jdk.unsupported.desktop won't be included
in the docs build which is intended (as it's unsupported).  Can
you verify it?

Nit: jdk.swing.interop.* source files
it'd be good to keep import statements sorted by the names.

Mandy

On 5/4/18 5:00 AM, Prasanta Sadhukhan wrote:

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
sun.swing, sun.awt, java.awt.dnd.peer, sun.awt.dnd, sun.awt.image and 
sun.java2d.


However, the goal is not to use qualified exports of these internal 
packages once FX is removed to be built along with JDK,


The solution will define a new "jdk.unsupported.desktop" module that 
exports public API
that is intended to be used by the javafx.swing module (but it does so 
with public exports and not qualified exports).
The idea is the same as jdk.unsupported, in that it is documented as 
being unsupported.
Further, since it is only intended to be used by javafx.swing, it need 
not be in the default module graph.


The module-info.java will look like this:
|module jdk.unsupported.desktop { requires transitive java.desktop; 
exports jdk.swing.interop; } |||
Enhancement: https://bugs.openjdk.java.net/browse/JDK-8202199, 
https://bugs.openjdk.java.net/browse/JDK-8195811

webrev: cr.openjdk.java.net/~psadhukhan/fxswing.6/
CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta
||

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

[Phil Race]

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 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
sun.swing, sun.awt, java.awt.dnd.peer, sun.awt.dnd, sun.awt.image and 
sun.java2d.


However, the goal is not to use qualified exports of these internal 
packages once FX is removed to be built along with JDK,


The solution will define a new "jdk.unsupported.desktop" module that 
exports public API
that is intended to be used by the javafx.swing module (but it does so 
with public exports and not qualified exports).
The idea is the same as jdk.unsupported, in that it is documented as 
being unsupported.
Further, since it is only intended to be used by javafx.swing, it need 
not be in the default module graph.


The module-info.java will look like this:

|module jdk.unsupported.desktop { requires transitive java.desktop; 
exports jdk.swing.interop; } |||


Enhancement: https://bugs.openjdk.java.net/browse/JDK-8202199, 
https://bugs.openjdk.java.net/browse/JDK-8195811

webrev: cr.openjdk.java.net/~psadhukhan/fxswing.6/
CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

||

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

[Nir Lisker]

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 != lwFrame.getLightweightFrame();'.
* Same with SwingInterOpUtils#isUngrabEvent.

* LightweightFrameWrapper#createUngrabEvent: you can directly 'return new
...' (I wouldn't comment about this had you not done this to the other
methods).

* LightweightContentWrapper: there're no new lines between non-empty
methods, especially in LightweightContentProxy. Is this an acceptable code
style?
* Same for DragSourceContextPeerWrapper and the proxy,
DragTargetContextPeerWrapper.
and DispatcherWrapper.

* Extra empty line(s) at the end of DropTargetContextPeerWrapper.

- Nir

On Fri, May 4, 2018 at 3:10 PM, Kevin Rushforth 
wrote:

> 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 ready for review. If you spot something of concern in the FX
> webrev, please feel free to bring it up, but don't spend time doing a
> thorough review just yet.
>
> -- Kevin
>
>
>
> On 5/4/2018 5:00 AM, Prasanta Sadhukhan wrote:
>
>> 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
>> sun.swing, sun.awt, java.awt.dnd.peer, sun.awt.dnd, sun.awt.image and
>> sun.java2d.
>>
>> However, the goal is not to use qualified exports of these internal
>> packages once FX is removed to be built along with JDK,
>>
>> The solution will define a new "jdk.unsupported.desktop" module that
>> exports public API
>> that is intended to be used by the javafx.swing module (but it does so
>> with public exports and not qualified exports).
>> The idea is the same as jdk.unsupported, in that it is documented as
>> being unsupported.
>> Further, since it is only intended to be used by javafx.swing, it need
>> not be in the default module graph.
>>
>> The module-info.java will look like this:
>> |module jdk.unsupported.desktop { requires transitive java.desktop;
>> exports jdk.swing.interop; } |||
>> Enhancement: https://bugs.openjdk.java.net/browse/JDK-8202199,
>> https://bugs.openjdk.java.net/browse/JDK-8195811
>> webrev: cr.openjdk.java.net/~psadhukhan/fxswing.6/
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8202175
>>
>> Regards
>> Prasanta
>> ||
>>
>>
>

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

[Kevin Rushforth]

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 ready for review. If you spot something of concern in 
the FX webrev, please feel free to bring it up, but don't spend time 
doing a thorough review just yet.


-- Kevin


On 5/4/2018 5:00 AM, Prasanta Sadhukhan wrote:

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
sun.swing, sun.awt, java.awt.dnd.peer, sun.awt.dnd, sun.awt.image and 
sun.java2d.


However, the goal is not to use qualified exports of these internal 
packages once FX is removed to be built along with JDK,


The solution will define a new "jdk.unsupported.desktop" module that 
exports public API
that is intended to be used by the javafx.swing module (but it does so 
with public exports and not qualified exports).
The idea is the same as jdk.unsupported, in that it is documented as 
being unsupported.
Further, since it is only intended to be used by javafx.swing, it need 
not be in the default module graph.


The module-info.java will look like this:
|module jdk.unsupported.desktop { requires transitive java.desktop; 
exports jdk.swing.interop; } |||
Enhancement: https://bugs.openjdk.java.net/browse/JDK-8202199, 
https://bugs.openjdk.java.net/browse/JDK-8195811

webrev: cr.openjdk.java.net/~psadhukhan/fxswing.6/
CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta
||

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

[Prasanta Sadhukhan]

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
sun.swing, sun.awt, java.awt.dnd.peer, sun.awt.dnd, sun.awt.image and 
sun.java2d.


However, the goal is not to use qualified exports of these internal 
packages once FX is removed to be built along with JDK,


The solution will define a new "jdk.unsupported.desktop" module that 
exports public API
that is intended to be used by the javafx.swing module (but it does so 
with public exports and not qualified exports).
The idea is the same as jdk.unsupported, in that it is documented as 
being unsupported.
Further, since it is only intended to be used by javafx.swing, it need 
not be in the default module graph.


The module-info.java will look like this:

|module jdk.unsupported.desktop { requires transitive java.desktop; 
exports jdk.swing.interop; } |||


Enhancement: https://bugs.openjdk.java.net/browse/JDK-8202199, 
https://bugs.openjdk.java.net/browse/JDK-8195811

webrev: cr.openjdk.java.net/~psadhukhan/fxswing.6/
CSR: https://bugs.openjdk.java.net/browse/JDK-8202175

Regards
Prasanta

||