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

[Carlos Garnacho]

On Fri, Oct 30, 2015 at 5:51 AM, Jonas Ådahl <jadahl at gmail.com> wrote:
> 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).

Yeah, I came to think along the same lines. The intention was making
it so you receive either drop_performed or cancelled, XDND gets rid of
this race condition by making the  messaging extremely synchronous
(Each XdndPosition must be replied with a XdndStatus, so does the last
one sent when you release the button, only afterwards the transfer is
started/cancelled).

It may also work if drag sources get either drag_finished or
cancelled, and make drop_performed something more invariant. It could
still be possible for the compositor to cancel dnd before button
release (eg. an "esc" keybinding), but those cases are rare and can be
mentioned in docs.

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

I see your point, changed locally.

Cheers,
  Carlos