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

[Carlos Garnacho]

Hey!,

On Tue, Dec 22, 2015 at 3:26 AM, Jonas Ådahl <jadahl at gmail.com> wrote:
> On Tue, Dec 22, 2015 at 02:33:32AM +0100, 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.dnd_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.dnd_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.
>>
>> Changes since v4:
>>   - Applied rewording suggestions from Jonas Ådahl. Added new
>>     wl_data_offer.finish request to allow explicit finalization on the
>>     destination side.
>>
>> Changes since v3:
>>   - Renamed dnd_performed to a more descriptive dnd_drop_performed,
>>     documented backwards compatible behavior on wl_data_offer.accept and
>>     wl_data_source.cancelled.
>>
>> Changes since v2:
>>   - Minor rewording.
>>
>> Changes since v1:
>>   - Renamed events to have a common "dnd" namespace. Made dnd_performed to
>>     happen invariably, data_device.cancelled may still happen afterwards.
>>
>> Signed-off-by: Carlos Garnacho <carlosg at gnome.org>
>> Reviewed-by: Michael Catanzaro <mcatanzaro at igalia.com>
>> Reviewed-by: Jonas Ådahl <jadahl at gmail.com>
>> Reviewed-by: Mike Blumenkrantz <zmike at samsung.com>
>
> Looks pretty good now. Btw, if you put the "Changes since ..."
> underneath the "---" below after doing git-format-patch you don't have
> to keep the patch change log in the actual commit message.

Ah sure, I thought the kernel style of keeping patch changelog for
posteriority was preferred.

>
> One comment and a couple of nits below.
>
>> ---
>>  protocol/wayland.xml | 75 +++++++++++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 69 insertions(+), 6 deletions(-)
>>
>> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
>> index df8ed19..ae5ef21 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
>> @@ -423,7 +423,17 @@
>>       Indicate that the client can accept the given mime type, or
>>       NULL for not accepted.
>>
>> -     Used for feedback during drag-and-drop.
>> +     For objects of version 2 or older, this request is used by the
>> +     client to give feedback whether the client can receive the given
>> +     mime type, or NULL if none is accepted; the feedback does not
>> +     determine whether drag-and-drop operation succeeds or not.
>> +
>> +     For objects of version 3 or newer, this request determines the
>> +     final result of the drag-and-drop operation. If the end result
>> +     is that no mime types was accepted, the drag-and-drop operation
>> +     will be cancelled and the corresponding drag source will receive
>> +     wl_data_source.cancelled. Clients may still use this event in
>> +     conjunction with wl_data_source.action for feedback.
>>        </description>
>>
>>        <arg name="serial" type="uint"/>
>> @@ -461,9 +471,24 @@
>>
>>        <arg name="mime_type" type="string"/>
>>      </event>
>> +
>> +    <!-- Version 3 additions -->
>> +
>> +    <request name="finish" since="3">
>> +      <description summary="the offer will no longer be used">
>> +     Notifies the compositor that the data offer will no longer be used.
>> +     Upon receiving this request, the compositor will emit
>> +     wl_data_source.drag_finished or wl_data_source.cancelled on the drag
>> +     source client depending on the most recent wl_data_offer.accept and
>> +     wl_data_offer.set_actions requests received from this offer.
>
> Hmm. What I had in mind is that .finish is called after the transfer is
> completed, i.e. the accept and actions (nit: which btw is not introduced
> in this patch so I suppose should not really be referenced yet) have

Oops indeed.

> already been ensured to be compatible. So, I don't see how
> wl_data_source.cancelled could ever be emitted with well behaving
> clients.

Two well behaved clients can disagree on the actions (eg. source only
offering "copy", dest only allowing "move"), the resulting action will
be "none", and wl_data_source.cancelled would be emitted. Same for
dropping places that are not really a drag destination, the dest
client should .set_actions(0) if dropping is not allowed there.

> I would rather expect a client calling .finish after not having
> the actual ability to have finished to be terminated for incorrect
> behaviour.

I agree though that there's times where it's just not convenient for
this to happen, eg. mid drag. I'll add a comment that this is only
allowed after wl_data_device.drop, and add the corresponding error.

>
> It would also be good to add a note in the destructor request about what
> events may be emitted.

Indeed.

>
>> +
>> +     It is a client error to perform other requests than
>> +     wl_data_offer.destroy after this one.
>
> nit: "..after this request is called.".
>
>> +      </description>
>> +    </request>
>>    </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 +535,52 @@
>>
>>      <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 drag-and-drop operation was performed, but the drop destination
>> +       did not accept any of the mimetypes offered through
>> +       wl_data_source.target.
>> +     - The drag-and-drop operation was performed but didn't happen over a
>> +       surface.
>> +     - The compositor cancelled the drag-and-drop operation (e.g. compositor
>> +       dependent timeouts to avoid stale drag-and-drop transfers).

I'll be adding here the extra point, as pointed out in your other email.

Cheers,
  Carlos