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

Thu Jun 14 20:13:40 UTC 2018

+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 
>>>> <names internal interface> .."
>>>> 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 
>>>>>> <https://bugs.openjdk.java.net/browse/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 
>>>>>>> <https://bugs.openjdk.java.net/browse/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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20180614/d604a0aa/attachment.html>