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