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

Tue Dec 22 18:07:55 PST 2015

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.

> 
> If you notice the weston 1/5 patch, on drag_grab_button(), we only
> cancel if there is no offer created (eg. dropping between windows, or
> on a client that didn't create a wl_data_device_manager), otherwise we
> just emit wl_data_device.drop and wl_data_source.dnd_drop_performed,
> so we rely on the dest client indirectly triggering
> wl_data_source.drag_finished/cancelled.

Yes, this makes sense. The only concern I'm talking about is when
the destination calling "finish" would mean that it was operation
cancelled, as I think "finish" should only be called after the whole
destination side of the operation has finished (after mime type and
action negotation has completed).

> 
> (hmm, that makes me thing that my recently added blurb to
> wl_data_offer.action might be better worded to make this clear,
> although it's strictly correct)
> 
> >
> >>
> >> > 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.
> >
> > I think it should rather be after all negotiation is finished, at the
> > same point in time when the client can call .receive. I.e. all mime
> > types match up, the actions match up and is not "ask", and the transfer
> > is ready to begin. Either the destination transfers and calls "finish"
> > when the transfer is "done enough" or without calling .receive, if it
> > for some reason actually don't care about the data.
> 
> IMO we should not involve .receive in negotiation, the data could have
> been already fetched by the client at the time drop happens, so the
> destination might just set the mimetype/action that was chosen in the
> end and use the prefetched data. And similar situations might arise
> after .drop, where more than one mimetype is .receive()d. So we
> basically have to account for 0..n .receive requests after drop.

Fair enough.

Jonas

> 
> >
> > Any time before that the client should just .destroy it if it don't want
> > to continue.
> 
> Agreed.
> 
> Cheers,
>   Carlos