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.

Reply via email to