Hi Anton,
On 10/16/2013 03:26 PM, anton nashatyrev wrote:
On 15.10.2013 15:02, Anthony Petrov wrote:
The analysis of the problem looks good. Please clarify one point:
"(2) mouse moved to target in a single X11 event"
Is the "single event" the one from (1) or from (3) ? IOW, are the (1)
and (2) a single event, or the (2) and (3) ?
No, each step is a separate X event: (1) ButtonPress, (2) MotionNotify,
(3) ButtonRelease
Please clarify what exactly do you mean by "mouse moved to target in a
single X11 event"? Do I understand correctly that the real problem we're
trying to fix here is the absence of a subsequent MotionNotify before
the final ButtonRelease gets processed?
Anyway, I have two concerns regarding the fix:
1. In startDrag() you now call the setNativeContext() and
setCurrentJVMLocalSourceTransferable() under the awtLock, whereas
previously these calls were preformed w/o the lock. This seems a bit
risky as potentially this may cause some deadlocks. We should possibly
rearrange the calls so that processMouseMove() is called before the
above two calls, and the two calls remain outside the lock.
Alternatively, we could acquire the lock again just for the sake of a
call to processMouseMove().
Agree, this would be safer.
2. It looks suspicious that we call processMouseMove()
unconditionally. The dragGestureMouseMotion could be stale, or
uninitialized yet.
I'm assuming that startDrag may occur only in response to MotionNotify
event, which should initialize the field and additionally we are
synchronizing on awtLock (startDrag() is invoked on the EDT) so it looks
like there is no need to check for initialization. Anyway if you think
this is the right thing there is no problem to add an additional check.
I think this assumption needs to be documented then. Please check all
possible code paths leading to startDrag(). If it's only called in
response to the MotionNotify event, then please add a comment about that
to the code (right before the startDrag() definition seems to be the
best place). In that case, no additional checks are needed.
Also, the event might actually be processed twice because
doProcessEvent() returns false in that case.
Good point... Do you see any potential problem with that?
The field dragGestureMouseMotion is accessed under awtLock in both
places, so it would be either initialized on the first doProcessEvent
call or if the second call occurs it might be rewritten with the same
value (if dnd is still not started).
I don't know if this is a problem or not, but this is a change in the
(long-standing) behavior and some legacy apps may not be ready to handle
that. Perhaps we could suppress handling of the mouse event somehow if
we know startDrag() will process it anyway?
--
best regards,
Anthony
Thanks!
Anton.
--
best regards,
Anthony
On 10/14/2013 06:26 PM, anton nashatyrev wrote:
Hi Artem,
with the fix the 'drag entered' event is actually generated on the
mouse move event (step 2). Currently this first move event is 'consumed'
by the MouseGestureRecognizer and is not treated as the part of a drag
operation, while this move could be important (as in our case).
During the process I've created a couple of fixes which tried to
address the step 3, but all of them seemed to me rather like hacks than
fixes.
BTW, I've run the regression DnD tests (from
test/closed/java/awt/dnd + test/java/awt/dnd, totally about 75 tests) -
all went fine.
Thanks!
Anton.
On 14.10.2013 17:45, Artem Ananiev wrote:
Hi, Anton,
thanks for details description of the bug and suggested fix. It helped
a lot.
My understanding is that the problem is not with step 2, but step 3.
Generating "drag entered" events on mouse press looks incompatible, I
can't predict side effects of such a change. Fixing step 3, so we
don't assume drag entered events already sent, looks more correct.
What do you think?
Thanks,
Artem
On 10/11/2013 7:49 PM, anton nashatyrev wrote:
Hello,
could you please review the following fix:
fix: http://cr.openjdk.java.net/~mcherkas/anton/8024061/webrev.00/
<http://cr.openjdk.java.net/%7Emcherkas/anton/8024061/webrev.00/>
bug: https://bugs.openjdk.java.net/browse/JDK-8024061
Problem: when doing quick drag'n'drop in X11 the incorrect
behavior appears in the different forms. One is the exception when
trying to get 'local Jvm' Transferable from DropTargetListener.
Reason: on quick drag the following sequence of events may
appear:
(1) mouse pressed on source -> (2) mouse moved to target in a single
X11 event -> (3) mouse released. What's happening in that case: on
event
(2) only a drag gesture is recognized and no drag enter is generated
yet. On event (3) the XDragSourceContextPeer assumes that entered
event
(if it happened) was already generated and the 'local JVM'
transferable
had been already captured by SunDropTargetContextPeer from the static
field currentJVMLocalSourceTransferable. Thus on event (3) the
XDragSourceContextPeer posts the additional mouseMove event (which
turns
into DragEnter later) and initiates the cleanup which then resets the
currentJVMLocalSourceTransferable to null. Thus on DragEnter the
currentJVMLocalSourceTransferable is already null and the
SunDropTargetContextPeer appears in the inconsistent state.
Solution: the event (2) from the example above should not only
initiate the DnD operation but also be a part of that operation, i.e.
this event should also appear as a drag motion. For that I propose to
keep a track of the XMotionEvents in the XDragSourceContextPeer to
catch
the mouse event which initiated the DnD and when a startDrag() is
called
process this event as the first drag motion event.
Thanks!
Anton.
