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

[Jonas Ådahl]

On Mon, Jan 11, 2016 at 05:39:20PM +0100, Carlos Garnacho wrote:
> Hi Pekka!,
> 
> On Tue, Jan 5, 2016 at 9:50 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > On Wed, 23 Dec 2015 12:31:16 +0100
> > Carlos Garnacho <carlosg at gnome.org> wrote:
> >
> >> Hey,
> >>
> >> On Wed, Dec 23, 2015 at 5:47 AM, Jonas Ådahl <jadahl at gmail.com> wrote:
> >> > Hi again,
> >> >
> >> > I was reading an E-mail in another thread that brought up different
> >> > types of backward compatibility promises, and it made me think of a
> >> > potential issue. I'm commenting inline close to the relevant change this
> >> > patch introduces.
> >> >
> >> > 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>
> >> >> ---
> >> >>  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.
> >> >
> >> > As Pekka (CC:ed) wrote in a reply to the proxy version tracking thread
> >> > [0], the kind of change that is introduced above is not really backward
> >> > compatible. The possible issue would be, if I understand things
> >> > correctly, that if we have a client split up into two separate parts (an
> >> > application and a third party library); if the application creates a
> >> > wl_data_device of version 3 and then passes it (or a wl_data_offer) to
> >> > the library that only support version 1 or 2, the client will break
> >> > because according to version < 3 there is no requirement to reason to
> >> > reply with "accept" other than wishing to provide optional feedback, and
> >> > the library may rely on this.
> >>
> >> Uhm, I see no way out of these misbehaviors. If an app manages a v3
> >> wl_data_offer (or source) with v2-style, it will simply not honor
> >> negotiation, we'll get a 0 action mask and drag-and-drop will fail. I
> >> see more trouble though when adding listeners, if the one doing it
> >> thinks this is v2....
> >
> > Listeners must always match or exceed the negotiated object version,
> > and we are getting the version query soon.
> >
> > However, the case with wl_surface in [0] is slightly different: the
> > third party library can "drive" a wl_surface without having any
> > listeners hooked on the wl_surface itself. I get the feeling that is
> > not the case here, so if these DnD objects are passed between sw
> > components, everyone must explicitly support the exact version that was
> > negotiated. Otherwise there is just no way it can work. And even then,
> > if to "drive" the object you must handle events on it, who should own
> > the listeners becomes an issue.
> 
> Agreed, I somehow expect this to be fully wrapped by one of the
> players, rather than shared across. As first-hand examples I would
> think GtkClutterEmbed or WebKitWebView would rather interact with
> GdkDragContext than hole-punching their way to wayland protocols.
> 
> >
> > Now I wonder if this above makes my comments below moot...
> >
> > OTOH, keeping backward compatibility rules clear and unbroken as far as
> > possible is a valuable policy, I think. Trying too carefully to analyze
> > every case whether a "break" is actually workable or not will probably
> > lead to a mistake eventually.
> >
> > If you really need to change the semantics, add a new request or event
> > with the new semantics. That way it will be clear also on the other end
> > whether someone is using the object according to v2 or v3 and it might
> > be manageable, or the compositor could complain rather than silently
> > cause misbehavior. Or you could even make it a well-defined protocol
> > error, if there is *really* no good way to have v3 work as v2.
> 
> Hmm, the problem I see with newly added requests that become mandatory
> in the workflow is that the failure to comply (i.e. not emitting
> those) can't be easily detected, is the client non-compliant or simply
> busy? FWIW, the expected (mis)behavior would be that DnD fails and
> wl_data_source.cancelled is emitted on the source (if version >=3),
> just the same that happens if the client sets actions=0.
> 
> I think there is an easy way out of here, but not too pretty IMO... We
> could default to "copy" on both sides, until the first .set_actions()
> call overwrites that result (which should happen as soon as the client
> is let know of the wl_data_source/offer). That'd make clients free to
> fail to comply, but such default is IMO hardly excusable in the long
> term. Would it be ok to mention in docs that we only recommend this
> behavior as long as version <=2 is supported by the compositor? OTOH,
> this is just postponing arbitrarily the fact that it is recommended to
> perform those requests.

We'd also need to make .finish() non-mandatory, i.e. useless, which
would break the fail-gracefully part of the additions. While I think
backward compatibility as in when a client uses a v3 object as a v2 is
generally a good idea, it should not be made a strict requirement. In
the DND case, we do need to add new requirements to fix certain issues,
and ditching the whole protocol because of this doesn't seem like the
better option.

A client also needs to actively choose to get a new version and update
its implementation either way, so it's not like an old client will
suddenly stop working.

> 
> (To the casual reader, this is all about clients that request v3 but
> behave like v2 in practical terms, interoperation between v3 and v2
> clients is observed by the protocol/weston patches.)
> 
> >
> >> It would fail even if we take mimetype acknowledgement out of .accept,
> >> because the caller wouldn't be calling the new request either. In all
> >> honesty, I consider v2 broken and something we should hide under the
> >> rug soon, there is no way we can stay both backward compatible and
> >> sane at the same time. If such library is more than a proof of
> >> concept, I'd urge it to update to v3 ASAP.
> >
> > So if I understand right, you say that v2 should not be used and
> > everyone should migrate to the new thing ASAP.
> 
> It has some inherent brokenness (see eg. fdo bugs #91943 and #91944).
> My main complain is that, while v2 is fine for weston-dnd and proves
> sufficient for intra-app DnD or simple cases, it quickly falls short
> once you try to get a real world toolkit using this (and the
> applications behind these).
> 
> I was perhaps too harsh saying v2 is broken, but I could say "if you
> want to do anything serious, you likely want v3" at best. Seeing that
> the audience is mostly toolkits, the end message might not be too
> different :).
> 
> >
> > In that case, is there any reason to keep building on top of v2 if we
> > cannot keep it backward-compatible in any case?
> >
> > Why not start from scratch with everything on new names and improved
> > specs, and let this old v2 be slowly abandoned?
> >
> > Is there any reason to keep v2 limping along as legacy in the spec?
> >
> > I think you have learned a lot in trying to make v2 work and you have
> > seen its problems, so you should be able to do better by designing a
> > replacement extension for DnD. But, you should also be careful to just
> > fix what you found broken and not go redesign everything just because
> > you can, since that might raise new issues the old design didn't have.
> >
> > After all, and not intending to depreciate krh's efforts in any way,
> > the original design was perhaps a bootstrap: something that works to
> > show how things could work on Wayland, with caveats unknown at the
> > time. We've gained years of experience since then, and the DnD protocol
> > has been quite unloved for a long time.
> 
> That is my theory too, and IMO it was a brilliant bootstrap. I
> honestly have no issues with the current DnD API and workflow, it's
> just a bit incomplete in things that surely weren't thought out at
> that time.
> 
> That'd actually be my counterargument for making this a separate
> extension/interfaces, it'd be 95% the same we have on v2, I would just
> use the opportunity to fold a few requests together maybe.

I don't think dropping the DND part of the wl_data_device protocol and
replacing it with a new wp_ protocol is a good idea. We can't remove it,
because the selection part still needs to be there. If we'd to deprecate
the whole wl_data_device protocol, could we ever really remove it
anyway? It'd break libwayland-client API.

We'd also get awkward undocumented compatibility bridges between
wl_data_device and "wp_dnd", as while they would have to be made to work
together, we could not document that in wl_data_device.


Jonas

> 
> >
> > There is also the question of how much of the new DnD requirements are
> > specific to desktops - does DnD protocol even have a place in the
> > Wayland core?
> 
> Although I might accept this argument from you. It really your call if
> you want to outcast this into wayland-protocols/unstable land... IMO
> it's a core experience, at least in the desktop world, other
> interfaces might be more selection centric.
> 
> Cheers,
>   Carlos