[PATCH wayland 1/2] protocol: Improve data source notification around DnD progress
Jonas Ådahl
jadahl at gmail.com
Wed Dec 23 03:37:41 PST 2015
On Wed, Dec 23, 2015 at 12:05:35PM +0100, Carlos Garnacho wrote:
> 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.
It's not so much about the implementation but about the following text:
> + 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.
It, to me, makes "finish" to be more like a "I did my best with what I
know right now" instead of "I have now completed any data transfer and
action related operation given given the known meta type and final
action" which I thought it would be. I don't see how it can possibly
"finish" anything without knowing the final meta type and action.
If you prefer that "finish" should be possible to call no matter the
final action and mime type, letting the compositor figure out if it
actually finished or not (given the final mime type and action), then I
think that should be added to the description.
Jonas
>
> Cheers,
> Carlos
More information about the wayland-devel
mailing list