[PATCH wayland] protocol: Improve data source notification around DnD progress

[Jonas Ådahl]

On Thu, Oct 29, 2015 at 05:51:04PM +0100, Carlos Garnacho wrote:
> Hey Jonas!,
> 
> On Thu, Oct 29, 2015 at 9:37 AM, Jonas Ådahl <jadahl at gmail.com> wrote:
> > Hey Carlos,
> >
> > Finally had a look at this one.
> 
> Cheers :)
> 
> >
> > On Wed, Sep 30, 2015 at 10:45:39PM +0200, Carlos Garnacho wrote:
> >> Currently, there's no means for the DnD origin to know whether the
> >> destination is actually finished with the DnD transaction, short of
> >> finalizing it after the first transfer finishes, or leaking it forever.
> >>
> >> But this poses other interoperation problems, drag destinations might
> >> be requesting several mimetypes at once, might be just poking to find
> >> out the most suitable format, might want to defer the action to a popup,
> >> might be poking contents early before the selection was dropped...
> >>
> >> In addition, data_source.cancelled is suitable for the situations where
> >> the DnD operation fails (not on a drop target, no matching mimetypes,
> >> etc..), but seems undocumented for that use (and unused in weston's DnD).
> >>
> >> In order to improve the situation, the drag source should be notified
> >> of all stages of DnD. In addition to documenting the "cancelled" event
> >> for DnD purposes, The following 2 events have been added:
> >>
> >> - wl_data_source.drop_performed: Happens when the operation has been
> >>   physically finished (eg. the button is released), it could be the right
> >>   place to reset the pointer cursor back and undo any other state resulting
> >>   from the initial button press.
> >> - wl_data_source.drag_finished: Happens when the destination side destroys
> >>   the wl_data_offer, at this point the source can just forget all data
> >>   related to the DnD selection as well, plus optionally deleting the data
> >>   on move operations.
> >>
> >> Signed-off-by: Carlos Garnacho <carlosg at gnome.org>
> >> ---
> >>  protocol/wayland.xml | 34 +++++++++++++++++++++++++++++-----
> >>  1 file changed, 29 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> >> index 42c9309..1ee143f 100644
> >> --- a/protocol/wayland.xml
> >> +++ b/protocol/wayland.xml
> >> @@ -408,7 +408,7 @@
> >>    </interface>
> >>
> >>
> >> -  <interface name="wl_data_offer" version="1">
> >> +  <interface name="wl_data_offer" version="3">
> >>      <description summary="offer to transfer data">
> >>        A wl_data_offer represents a piece of data offered for transfer
> >>        by another client (the source client).  It is used by the
> >> @@ -463,7 +463,7 @@
> >>      </event>
> >>    </interface>
> >>
> >> -  <interface name="wl_data_source" version="1">
> >> +  <interface name="wl_data_source" version="3">
> >>      <description summary="offer to transfer data">
> >>        The wl_data_source object is the source side of a wl_data_offer.
> >>        It is created by the source client in a data transfer and
> >> @@ -510,14 +510,38 @@
> >>
> >>      <event name="cancelled">
> >>        <description summary="selection was cancelled">
> >> -     This data source has been replaced by another data source.
> >> +     This data source is no longer valid. There are several reasons why
> >> +     this could happen:
> >> +
> >> +     - The data source has been replaced by another data source.
> >> +     - The drop operation finished, but the drop destination did not
> >> +       accept any of the mimetypes offered through data_source.target.
> >> +     - The drop operation finished but didn't happen over a DnD target.
> >
> > That the "operation finished" sounds like the transfer was done as well.
> > "The drop was performed" maybe is better?
> 
> Right, plus the name matches with the event that you'd be expecting otherwise.
> 
> >
> >> +
> >>       The client should clean up and destroy this data source.
> >>        </description>
> >>      </event>
> >>
> >> +    <!-- Version 3 additions -->
> >> +
> >> +    <event name="drop_performed" since="3">
> >> +      <description summary="the drag-and-drop operation physically finished">
> >> +     The user dropped this data onto an accepting destination. Note
> >> +     that the data_source will be used further in the future and should
> >> +     not be destroyed here.
> >> +      </description>
> >> +    </event>
> >
> > I too would like some clarifications why this is needed. The cursor and
> > button state, I assume, could be reset on the next enter or by
> > cancelled/finished, but that the client may want update its UI in some
> > way.
> 
> Yeah, I think that's nice to have, the drag source might enter some
> special UI mode on DnD that has to be reverted after the operation is
> done.
> 
> But this is admittedly non essential information on wayland clients
> done from the ground up. You might consider this similar to how window
> dragging is implemented, where you don't get anything after the button
> press, however there's a difference with DnD face to the traditional
> toolkits: window dragging was already WM territory, toolkits were
> already taught to give up on the implicit grab and let the WM take
> one, but DnD was purely application territory.
> 
> In hindsight (I can just talk for GTK+ tbh) much of the DnD machinery
> should have been hidden at the lowest levels, reality is that DnD is
> driven from a quite high level, more namely the active pointer grab
> used, around which everything revolves for XDND (lifetime included).
> Obviously the longer term goal is to refactor this functionality to a
> lower level, although I'm still scratching my head about how to do in
> a compatible way, and would certainly need assistance to avoid
> breaking other backends than x11/wayland.
> 
> So this x11-style "grab" still happens before start_drag on the
> compositor, all it currently does face to the wayland compositor is
> updating the pointer cursor, but client-side it brings the usual
> grabby effects, input still taken "elsewhere", etc. However, we don't
> have a place to finish this grab, waiting for the next enter event
> strikes me as too late, plus this enter event might be
> undistinguishable from a not-after-dnd one at the windowing layer. It
> is similar for drag_finished, an "ask" action (as per the other patch)
> could trigger a popup, which would mess with this grab state if
> source==dest.

As long as there needs to be no UI/behavioural changes as a result of
ending this grab, ending it at the next 'enter' I guess would be just
fine, but as long as UI needs to react to the drop immediately, it seems
necessary.

> 
> I nonetheless consider this event a nicety to have, not part of the
> basic scaffolding, but IMHO conveys information that may be useful
> besides greedy gtk+ reasons.

Yea, I think that adding protocol for fixable historical toolkit
reasons is undesirable, but I can see why this event may be useful
regardless, so I have no objections.

> 
> >
> > This also, as discussed in another part of this thread, adds
> > requirements on the destination "accepting" an offer. It'd also be good
> > with clarifications about what would happen if a offer accepted an
> > offer at one position, but the drop was performed on another, where the
> > destination do not accept. Would one still get a cancelled then?
> 
> Hmm, I guess there could be a race condition if drag_motion and drop
> happen before the client could accept() in response to drag_motion.
> Emitting cancelled after drop_performed should be in place if we end
> up with accept(null) in the end, this would also be a desirable
> outcome when cancelling popup menus triggered by "ask" actions. I'll
> try to make that clearer in docs.

Should we even always emit the "performed" event, before every cancelled
or finished? Otherwise it'd be a bit undeterministic when to expect it
(depending on the race you described).

> 
> >
> > Maybe also add a paragraph to wl_data_offer.accept?
> 
> Indeed.
> 
> >
> >> +
> >> +    <event name="drag_finished" since="3">
> >> +      <description summary="the drag-and-drop operation finished">
> >> +     The drop destination finished interoperating with this data
> >> +     source, the client is now free to destroy this data source and
> >> +     free all associated data.
> >> +      </description>
> >> +    </event>
> >>    </interface>
> >
> > "drag_" finished sounds a bit odd to me. I guess its meant to namespace
> > the event as a drag-and-drop event?
> 
> Yeah mainly...
> 
> >
> > Correct me if I'm wrong, but this event is meant to be sent after a
> > destination has read all the data it needs, meaning we could effectively
> > send this for regular clipboard sources as well? Then just call it
> > "finished".
> 
> Clipboard offers have a different lifetime, which is "as long as the
> client is focused, or until the focused client replaces the
> clipboard". You could be reusing the same offer multiple times there,
> this is only true to an extent in DnD (which drag_finished precisely
> signals the end for, operations on the DnD offer should only endure
> for a limited time after the drop was performed).

True, so it doesn't make sense because the source is reusable for a
selection, but not for a dnd.

As for nitpicking/bikeshedding, should we just put the drag and drop
specific events behind some "dnd_" prefix, instead of having multiple
prefixes?

dnd_performed I'd read as drag and drop both happened; drop_performed
implicitly means the drag happened as well.

dnd_finished I'd read the same as dnd_performed except that the whole
thing including data transfer has finished, but drag_finished I'd read
as the *drag* finished, whatever that means (finger/pointer stopped?).


Jonas

> 
> >
> > Thats all I have now.
> 
> Thanks :),
>   Carlos