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