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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > >> >> Reviewed-by: Michael Catanzaro <[email protected]> > >> >> Reviewed-by: Jonas Ådahl <[email protected]> > >> >> Reviewed-by: Mike Blumenkrantz <[email protected]> > >> > > >> > 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 _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
