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