Hello, Anthony. Thank you for the review.
> Do you think we should file a new bug to redesign this (and possibly other) > helper method(s) (e.g. by introducing a new interface that extends the > PlatformWindow)? I totally agree that it is a good idea to give it a try. With best regards. Petr. On Apr 4, 2013, at 4:10 PM, Anthony Petrov wrote: > Hi Petr, Sergey, > > You're right, and I agree that PlatformWindow shouldn't be polluted with > platform-specific APIs. I also think that having code with many instanceof > checks indicates that the code is ill-designed. Do you think we should file a > new bug to redesign this (and possibly other) helper method(s) (e.g. by > introducing a new interface that extends the PlatformWindow)? If so, I'm fine > with the current fix. > -- > best regards, > Anthony > > On 04/03/13 21:27, Petr Pchelko wrote: >> Hello, Anthony. >> >> Thank you for the review. >> >>> Why not make it a part of the PlatformWindow interface then? There's >>> already the getLayerPtr() which is Mac-specific. >> I agree to Sergey. The idea as I understand is to avoid mac specific methods >> in PlatformWindow, so my vote would be not to add another such method to >> PlatformWindow. >> What so you think about it? >> >> With best regards. Petr. >> >> 03.04.2013, в 19:25, Sergey Bylokhov<[email protected]> написал(а): >> >>> 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. >>> >>
