<AWT Dev> <AWT dev> Review Request for 8006941 : [macosx] Deadlock in drag and drop & 7199783 : Setting cursor on DragSourceContext does not work on OSX
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Wed Apr 3 08:25:01 PDT 2013
On 4/3/13 7:11 PM, Anthony Petrov wrote:
>
> Why not make it a part of the PlatformWindow interface then? There's
> already the getLayerPtr() which is Mac-specific.
It was added by the mistake. I hope it will be removed in the future.
> So adding something like getContentPtr() wouldn't make it any worse.
> And it would still look better than a bunch of instanceof checks in
> the helper method.
>
> Otherwise the fix looks good to me.
>
> --
> best regards,
> Anthony
>
> On 4/3/2013 18:41, Petr Pchelko wrote:
>> Hello, Anthony.
>>
>> Thank you for the review. I have updated the fix according to your
>> comments. The new version is here:
>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.06/
>>
>>> Can this condition ever be not true? I see that only
>>> LWComponentPeer.java calls this code. Why are the argument types for
>>> CDropTarget so generic?
>> This is left here from the Apple times. Deleted.
>>
>>> There's the same code in CDropTarget.java. I suggest to extract it
>>> in a common utility method somewhere in the macosx.* classes.
>> Moved to the helper method in CPlatformWindow
>>
>>> A general question: you mention 7199783 (implement a missing
>>> functionality), 8006941 (resolve a deadlock), and also, the test
>>> mentions a crash. Looks like these three are completely separate
>>> issues and should better be fixed with separate patches for clarity
>>> and easier maintainability. Why are you combining all this into one
>>> fix? I'd vote for fixing them separately if it is possible.
>> I know thats bad and the problems seem completely separate and
>> unrelated.
>> But when the deadlock was fixed a lot of code related to cursor could
>> be cleaned up. This code was added as a workaround for the fixed
>> deadlock. Cleaning up this code simultaneously resolves the issue
>> 7199783. As for the crash - it was discovered while I was making the
>> fix and prevented from testing some corner cases. But the crash
>> itself could not be fixed without fixing the deadlock. So all these
>> issues are so interconnected that the only possibility left was to
>> merge all changes into one fix.
>>
>> With best regards. Petr.
>>
>> On Apr 3, 2013, at 2:50 PM, Anthony Petrov wrote:
>>
>>> Hi Petr,
>>>
>>> A general question: you mention 7199783 (implement a missing
>>> functionality), 8006941 (resolve a deadlock), and also, the test
>>> mentions a crash. Looks like these three are completely separate
>>> issues and should better be fixed with separate patches for clarity
>>> and easier maintainability. Why are you combining all this into one
>>> fix? I'd vote for fixing them separately if it is possible.
>>>
>>> Here's some comments regarding the code changes:
>>>
>>> src/macosx/classes/sun/lwawt/macosx/CDropTarget.java
>>>> 54 // Make sure the drop target is a LWWindowPeer:
>>>> 55 if (!(peer instanceof LWComponentPeer))
>>> Can this condition ever be not true? I see that only
>>> LWComponentPeer.java calls this code. Why are the argument types for
>>> CDropTarget so generic?
>>>
>>>
>>> src/macosx/classes/sun/lwawt/macosx/CDragSourceContextPeer.java
>>>> 111 PlatformWindow platformWindow =
>>>> ((LWComponentPeer)rootComponent.getPeer()).getPlatformWindow();
>>>> 112 long nativeViewPtr;
>>>> 113 if (platformWindow instanceof CPlatformWindow) {
>>>> 114 nativeViewPtr = ((CPlatformWindow)
>>>> platformWindow).getContentView().getAWTView();
>>>> 115 } else if (platformWindow instanceof
>>>> CViewPlatformEmbeddedFrame) {
>>>> 116 nativeViewPtr = ((CViewPlatformEmbeddedFrame)
>>>> platformWindow).getNSViewPtr();
>>>> 117 } else {
>>>> 118 throw new InvalidDnDOperationException("Unsupported
>>>> PlatformWindow implementation");
>>>> 119 }
>>> There's the same code in CDropTarget.java. I suggest to extract it
>>> in a common utility method somewhere in the macosx.* classes.
>>>
>>> The rest of the fix looks good I guess, although I'm not an expert
>>> in DnD.
>>>
>>> --
>>> best regards,
>>> Anthony
>>>
>>> On 4/1/2013 16:14, Petr Pchelko wrote:
>>>> Hello, AWT Team.
>>>> This is a reminder. Could I please get a second review on this?
>>>> For your convenience:
>>>> Bug:
>>>> http://bugs.sun.com/view_bug.do?bug_id=7199783 Setting cursor on
>>>> DragSourceContext does not work on OSX
>>>> http://bugs.sun.com/view_bug.do?bug_id=8006941 Deadlock in drag
>>>> and drop
>>>> Fix:
>>>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.05/
>>>> With best regards. Petr.
>>>> On Mar 11, 2013, at 12:30 PM, Petr Pchelko wrote:
>>>>> Hello, AWT Team.
>>>>>
>>>>> Could I please get a second review on this?
>>>>>
>>>>> For your convenience:
>>>>> Bug:
>>>>> http://bugs.sun.com/view_bug.do?bug_id=7199783 Setting cursor on
>>>>> DragSourceContext does not work on OSX
>>>>> http://bugs.sun.com/view_bug.do?bug_id=8006941 Deadlock in drag
>>>>> and drop
>>>>> Fix:
>>>>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.05/
>>>>>
>>>>> Thank you.
>>>>> With best regards. Petr.
>>>>>
>>>>> On Mar 4, 2013, at 4:01 PM, Sergey Bylokhov wrote:
>>>>>
>>>>>> Hi, Petr.
>>>>>> Fix looks good. Comment about the test: Swing classes should be
>>>>>> created and used on the EDT. Change it before the push.
>>>>>>
>>>>>> 04.03.2013 15:15, Petr Pchelko wrote:
>>>>>>> Hello, Sergey.
>>>>>>>
>>>>>>> Please have a look on the updated webrev with a test:
>>>>>>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.05/
>>>>>>>
>>>>>>> All the rest is unchanged.
>>>>>>>
>>>>>>> With best regards. Petr.
>>>>>>>
>>>>>>> On Mar 1, 2013, at 1:21 PM, Sergey Bylokhov wrote:
>>>>>>>
>>>>>>>> Hi, Petr.
>>>>>>>> If the crash can be reproduced easily I suggest to add a new test.
>>>>>>>>
>>>>>>>> 28.02.2013 20:21, Petr Pchelko wrote:
>>>>>>>>> Thank you for the reviews.
>>>>>>>>>
>>>>>>>>> Please have a look on the updated version here:
>>>>>>>>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.04/
>>>>>>>>>
>>>>>>>>> The CDropTarget is now also ready to work with a view-based
>>>>>>>>> embedded frame.
>>>>>>>>> Also A native crash was discovered, it will be fixed with the
>>>>>>>>> current fix.
>>>>>>>>>
>>>>>>>>> I have discovered 2 additional DnD issues while testing, I
>>>>>>>>> would file separate issues.
>>>>>>>>>
>>>>>>>>> With best regards. Petr.
>>>>>>>>>
>>>>>>>>> On Feb 28, 2013, at 4:47 PM, Denis S. Fokin wrote:
>>>>>>>>>
>>>>>>>>>> Hi Petr,
>>>>>>>>>>
>>>>>>>>>> I would make these lines shorter...
>>>>>>>>>> CDropTarget.m:495
>>>>>>>>>> CDropTarget.m:609
>>>>>>>>>>
>>>>>>>>>> Ok, I see that in the next line I did not noticed that the
>>>>>>>>>> parentheses
>>>>>>>>>> are related to the whole statement.
>>>>>>>>>> 457 if (!(root.contains(x, y) && root.isEnabled() &&
>>>>>>>>>> root.isVisible())) {
>>>>>>>>>>
>>>>>>>>>> Therefor, I would prefer to see || instead of &&. But it is
>>>>>>>>>> just my personal observation.
>>>>>>>>>>
>>>>>>>>>> Thank you,
>>>>>>>>>> Denis.
>>>>>>>>>>
>>>>>>>>>> On 2/28/2013 4:36 PM, Petr Pchelko wrote:
>>>>>>>>>>> Hello, Denis, Sergey. Thank you for the reviews.
>>>>>>>>>>>
>>>>>>>>>>> The updated version of the fix is available here:
>>>>>>>>>>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.03/
>>>>>>>>>>>
>>>>>>>>>>> Sergey wrote:
>>>>>>>>>>>> getViewPtr() is not a good method in PlatformWindow
>>>>>>>>>>>> interface because pointer to NSView is macosx specific
>>>>>>>>>>>> implementation.
>>>>>>>>>>> Removed it, replace with if's with instanceof
>>>>>>>>>>>
>>>>>>>>>>>> What does it mean "Returns the first " in the
>>>>>>>>>>>> getDropTargetAt. Should it be hw/lw?
>>>>>>>>>>> It should return any component which would be a valid drop
>>>>>>>>>>> target for the drag. It does not matter, lightweight or
>>>>>>>>>>> heavyweight. I have updated a comment a bit.
>>>>>>>>>>>
>>>>>>>>>>>> Check your fix with the example from the CR:8007155. Just
>>>>>>>>>>>> modify for dnd.
>>>>>>>>>>> Works fine.
>>>>>>>>>>>
>>>>>>>>>>> Denis wrote:
>>>>>>>>>>>> Where we are posting dragExit and dragEnter messages now?
>>>>>>>>>>>> Is it the event dispatching issue that you have mentioned?
>>>>>>>>>>>> CDropTarget.m:516,614
>>>>>>>>>>> The DragSource Enter/Exit messages are posted in Java now.
>>>>>>>>>>> The issue is that native dropTarget bounds are not the same
>>>>>>>>>>> as in Java because a native peer for DnD is a contentView of
>>>>>>>>>>> the window. So these events are posted in the wrong places
>>>>>>>>>>> or were not posted when they should have been. For
>>>>>>>>>>> DropTarget events this issue also exist and the Enter/Exit
>>>>>>>>>>> events are modified on the Java level. But for the
>>>>>>>>>>> dragSource I've decided not to use native events at all,
>>>>>>>>>>> because they are not very useful, it's easier to synthesize
>>>>>>>>>>> them in Java.
>>>>>>>>>>>
>>>>>>>>>>>> I would say that visibility and isEnabled() value do not
>>>>>>>>>>>> make sense here.
>>>>>>>>>>>> CDragSourceContextPeer.java
>>>>>>>>>>>> 449 if (!(root.contains(x, y) && root.isVisible()
>>>>>>>>>>>> && root.isEnabled())) {
>>>>>>>>>>> We should find only the dropTarget which would be a valid
>>>>>>>>>>> target for the current drag. This implies that the component
>>>>>>>>>>> should be enabled, visible, and should have a valid drop
>>>>>>>>>>> target. This would produce the same behavior as on Windows.
>>>>>>>>>>>
>>>>>>>>>>> With best regards. Petr.
>>>>>>>>>>>
>>>>>>>>>>> On Feb 28, 2013, at 1:24 PM, Sergey Bylokhov wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi, Petr.
>>>>>>>>>>>> getViewPtr() is not a good method in PlatformWindow
>>>>>>>>>>>> interface because pointer to NSView is macosx specific
>>>>>>>>>>>> implementation.
>>>>>>>>>>>> Could you please split VERY long line in the CDropTarget.m
>>>>>>>>>>>> What does it mean "Returns the first " in the
>>>>>>>>>>>> getDropTargetAt. Should it be hw/lw?
>>>>>>>>>>>> Check your fix with the example from the CR:8007155. Just
>>>>>>>>>>>> modify for dnd.
>>>>>>>>>>>>
>>>>>>>>>>>> 27.02.2013 17:43, Petr Pchelko wrote:
>>>>>>>>>>>>> Hello, AWT Team.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Please, review the fix for 2 issues:
>>>>>>>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=7199783 Setting
>>>>>>>>>>>>> cursor on DragSourceContext does not work on OSX
>>>>>>>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=8006941 Deadlock in
>>>>>>>>>>>>> drag and drop
>>>>>>>>>>>>> The fix is available at:
>>>>>>>>>>>>> http://cr.openjdk.java.net/~pchelko/8006941/webrev.02/
>>>>>>>>>>>>>
>>>>>>>>>>>>> I have made a single fix for these 2 issues, because they
>>>>>>>>>>>>> are quite closely related, and the same methods need to be
>>>>>>>>>>>>> changed. And they depend on one another quite a bit.
>>>>>>>>>>>>>
>>>>>>>>>>>>> 1. The deadlock occurred because in
>>>>>>>>>>>>> CDragSourceContextPeer.dragMouseMoved methods on
>>>>>>>>>>>>> components were invoke on the Appkit thread. They blocked
>>>>>>>>>>>>> on an AWTTreeLock if EDT had already took it. EDT trying
>>>>>>>>>>>>> to perform a sync selector on the Appkit thread lead to a
>>>>>>>>>>>>> deadlock. So the logic of working with components are
>>>>>>>>>>>>> moved to the EDT now.
>>>>>>>>>>>>> 2. DragSource events were dispatched absolutely
>>>>>>>>>>>>> incorrectly. Now we dispatch them the same way as on other
>>>>>>>>>>>>> platforms.
>>>>>>>>>>>>> 3. CCursorManager contained a workaround for the issue
>>>>>>>>>>>>> that we were not able to perform sync selectors during the
>>>>>>>>>>>>> dnd. Now sync selectors are processed during drag, so this
>>>>>>>>>>>>> workaround is not needed any more.
>>>>>>>>>>>>> 4. The functionality to set a cursor in
>>>>>>>>>>>>> DragSourceListeners is implemented. It works, however it
>>>>>>>>>>>>> still has a couple of issues:
>>>>>>>>>>>>> a. Sometimes mouse events are dispatched during the dnd,
>>>>>>>>>>>>> which might reset a cursor. That is wrong, mouse events
>>>>>>>>>>>>> should not be dispatched. It is a separate issue, so this
>>>>>>>>>>>>> problem will disappear as mouseEvent dispatching would be
>>>>>>>>>>>>> fixed.
>>>>>>>>>>>>> b. If the DropTarget supports NSDragOperationCopy cocoa
>>>>>>>>>>>>> sets an NSDragCopyCursor. There is no API to disable this
>>>>>>>>>>>>> and I have found no suitable workaround.
>>>>>>>>>>>>> c. On the first drag in the lifetime of the application
>>>>>>>>>>>>> cursor is not set. This is due to a bug in cocoa. They
>>>>>>>>>>>>> reset a cursor in some setter which looks absolutely
>>>>>>>>>>>>> unrelated and called when the first dragging session is
>>>>>>>>>>>>> initialized. I am thinking about filing a bug against apple.
>>>>>>>>>>>>> 5. Some cleanup: removed unused variables.
>>>>>>>>>>>>>
>>>>>>>>>>>>> With best regards. Petr.
>>>>>>>>>>>> --
>>>>>>>>>>>> Best regards, Sergey.
>>>>>>>>>>>>
>>>>>>>> --
>>>>>>>> Best regards, Sergey.
>>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Best regards, Sergey.
>>>>>>
>>
--
Best regards, Sergey.
More information about the awt-dev
mailing list