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

[Carlos Garnacho]

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.

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.

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

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

>
> Thats all I have now.

Thanks :),
  Carlos