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

[Carlos Garnacho]

Hey,

On Wed, Dec 23, 2015 at 3:07 AM, Jonas Ådahl <jadahl at gmail.com> wrote:
> On Tue, Dec 22, 2015 at 04:46:37PM +0100, Carlos Garnacho wrote:
>> Hey,
>>
>> On Tue, Dec 22, 2015 at 2:55 PM, Jonas Ådahl <jadahl at gmail.com> wrote:
>> > On Tue, Dec 22, 2015 at 01:33:08PM +0100, Carlos Garnacho wrote:
>> >> 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.
>> >
>> > Actually I don't know what is preferred. Personally I put the changelog
>> > outside of the commit message and thought that was what was preferred
>> > but I might be wrong.
>> >
>> >>
>> >> >
>> >> > 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.
>> >
>> > Yes, but if the resulting action is "none", the destination side would
>> > never ever call "finish" to begin with since it has nothing that it can
>> > finish. I thought the point of "finish" was that an already successfully
>> > negotiated data transfer had finished transferring.
>>
>> The resulting action may end up as none after wl_data_device.drop (eg.
>> cancelling a menu popped up on "ask"), and the dest client would need
>> to wl_data_offer.finish() it.
>
> Why would it wl_data_offer.finish() it and not wl_data_offer.destroy()
> it? It didn't finish anything, since the action is "none". Or do you
> want to special case "none" where "finish" is the same as "destroy"?
>
>>
>> There's also the small race condition pointed out in
>> http://lists.freedesktop.org/archives/wayland-devel/2015-October/025107.html,
>> which means that button release may not be a proper time to decide
>> about wl_drag_source.cancelled vs .dnd_drop_performed (and eventually
>> .dnd_finished), which means drop sites can still potentially do a last
>> .set_actions() change soon after .drop, and that means they have to
>> .finish() too.
>
> Yes, but I thought wl_data_source.cancelled would either come before or
> after .dnd_drop_performed, and may very well come as a result of
> wl_data_offer.destroy much later than .dnd_drop_performed.

Hmm, right. If it's "I disallow DnD" what the drag dest wants,
destroying would be just as effective. In my local changes I have in
data_offer_destroy (pseudocode):

if (offer->source) {
       if (offer_version < 3)
              data_offer_notify_finish(offer);
       else if (source_version >= 3)
              wl_data_source_send_cancelled(offer->source->resource);
}

That should cater for what you mention here.

Cheers,
  Carlos