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

Thu Jun 14 01:04:51 UTC 2018

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/20180613/84703bef/attachment.html>