[PATCH weston 1/5] data-device: Implement DnD progress notification
Jonas Ådahl
jadahl at gmail.com
Wed Dec 16 17:18:43 PST 2015
On Wed, Dec 16, 2015 at 04:03:14PM +0100, Carlos Garnacho wrote:
> Hey :),
>
> On Wed, Dec 16, 2015 at 3:21 AM, Jonas Ådahl <jadahl at gmail.com> wrote:
> > On Tue, Dec 15, 2015 at 06:56:21PM +0100, Carlos Garnacho wrote:
> >> Weston now sends wl_data_source.drop_performed and .drag_finished in
> >> order to notify about the different phases of DnD.
> >
> > s/drop_performed/dnd_drop_performed, and s/drag_finished/dnd_finished/.
>
> Oops, missed the renames in the commit logs.
>
> >
> >>
> >> wl_data_source.cancelled is also used as mentioned in the docs, being
> >> emitted also on DnD when the operation is meant to fail (eg. source
> >> and dest didn't agree on a mimetype).
> >>
> >> The dnd demo is also fixed so the struct dnd_drag isn't leaked.
> >>
> >> https://bugs.freedesktop.org/show_bug.cgi?id=91943
> >> https://bugs.freedesktop.org/show_bug.cgi?id=91944
> >>
> >> Changes since v1:
> >> - Updated to protocol v2.
> >>
> >> Signed-off-by: Carlos Garnacho <carlosg at gnome.org>
> >> Reviewed-by: Michael Catanzaro <mcatanzaro at igalia.com>
> >> ---
> >> clients/dnd.c | 39 +++++++++++++++++++++++++++++++--------
> >> clients/window.c | 2 +-
> >> src/compositor.h | 2 ++
> >> src/data-device.c | 37 +++++++++++++++++++++++++++++++------
> >> 4 files changed, 65 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/clients/dnd.c b/clients/dnd.c
> >> index e6a0c39..6c2ed57 100644
> >> --- a/clients/dnd.c
> >> +++ b/clients/dnd.c
> >> @@ -318,14 +318,8 @@ data_source_send(void *data, struct wl_data_source *source,
> >> }
> >>
> >> static void
> >> -data_source_cancelled(void *data, struct wl_data_source *source)
> >> +destroy_dnd_drag(struct dnd_drag *dnd_drag)
> >
> > naming nit: dnd_drag_destroy().
> >
> >> {
> >> - struct dnd_drag *dnd_drag = data;
> >> -
> >> - /* The 'cancelled' event means that the source is no longer in
> >> - * use by the drag (or current selection). We need to clean
> >> - * up the drag object created and the local state. */
> >> -
> >> wl_data_source_destroy(dnd_drag->data_source);
> >>
> >> /* Destroy the item that has been dragged out */
> >> @@ -339,10 +333,39 @@ data_source_cancelled(void *data, struct wl_data_source *source)
> >> free(dnd_drag);
> >> }
> >>
> >> +static void
> >> +data_source_cancelled(void *data, struct wl_data_source *source)
> >> +{
> >> + struct dnd_drag *dnd_drag = data;
> >> +
> >> + /* The 'cancelled' event means that the source is no longer in
> >> + * use by the drag (or current selection). We need to clean
> >> + * up the drag object created and the local state. */
> >> + destroy_dnd_drag(dnd_drag);
> >
> > Tab instead of spaces.
> >
> >> +}
> >> +
> >> +static void
> >> +data_source_drop_performed(void *data, struct wl_data_source *source)
> >> +{
> >> +}
> >> +
> >> +static void
> >> +data_source_drag_finished(void *data, struct wl_data_source *source)
> >> +{
> >> + struct dnd_drag *dnd_drag = data;
> >> +
> >> + /* The operation is already finished, we can destroy all
> >> + * related data.
> >> + */
> >
> > Tab instead of spaces.
> >
> >> + destroy_dnd_drag(dnd_drag);
> >> +}
> >> +
> >> static const struct wl_data_source_listener data_source_listener = {
> >> data_source_target,
> >> data_source_send,
> >> - data_source_cancelled
> >> + data_source_cancelled,
> >> + data_source_drop_performed,
> >> + data_source_drag_finished,
> >> };
> >>
> >> static cairo_surface_t *
> >> diff --git a/clients/window.c b/clients/window.c
> >> index 47628de..24aa517 100644
> >> --- a/clients/window.c
> >> +++ b/clients/window.c
> >> @@ -5376,7 +5376,7 @@ registry_handle_global(void *data, struct wl_registry *registry, uint32_t id,
> >> d->shm = wl_registry_bind(registry, id, &wl_shm_interface, 1);
> >> wl_shm_add_listener(d->shm, &shm_listener, d);
> >> } else if (strcmp(interface, "wl_data_device_manager") == 0) {
> >> - d->data_device_manager_version = MIN(version, 2);
> >> + d->data_device_manager_version = MIN(version, 3);
> >> d->data_device_manager =
> >> wl_registry_bind(registry, id,
> >> &wl_data_device_manager_interface,
> >> diff --git a/src/compositor.h b/src/compositor.h
> >> index 4443c72..96a7b0d 100644
> >> --- a/src/compositor.h
> >> +++ b/src/compositor.h
> >> @@ -304,6 +304,8 @@ struct weston_data_source {
> >> struct wl_resource *resource;
> >> struct wl_signal destroy_signal;
> >> struct wl_array mime_types;
> >> + struct weston_data_offer *offer;
> >> + bool accepted;
> >>
> >> void (*accept)(struct weston_data_source *source,
> >> uint32_t serial, const char *mime_type);
> >> diff --git a/src/data-device.c b/src/data-device.c
> >> index 1612091..2b55f7b 100644
> >> --- a/src/data-device.c
> >> +++ b/src/data-device.c
> >> @@ -62,12 +62,18 @@ data_offer_accept(struct wl_client *client, struct wl_resource *resource,
> >> {
> >> struct weston_data_offer *offer = wl_resource_get_user_data(resource);
> >>
> >> + /* Protect against untimely calls from older data offers */
> >> + if (!offer->source || offer != offer->source->offer)
> >> + return;
> >> +
> >> /* FIXME: Check that client is currently focused by the input
> >> * device that is currently dragging this data source. Should
> >> * this be a wl_data_device request? */
> >>
> >> - if (offer->source)
> >> + if (offer->source) {
> >> offer->source->accept(offer->source, serial, mime_type);
> >> + offer->source->accepted = mime_type != NULL;
> >> + }
> >> }
> >>
> >> static void
> >> @@ -76,7 +82,7 @@ data_offer_receive(struct wl_client *client, struct wl_resource *resource,
> >> {
> >> struct weston_data_offer *offer = wl_resource_get_user_data(resource);
> >>
> >> - if (offer->source)
> >> + if (offer->source && offer == offer->source->offer)
> >> offer->source->send(offer->source, mime_type, fd);
> >> else
> >> close(fd);
> >> @@ -99,8 +105,18 @@ destroy_data_offer(struct wl_resource *resource)
> >> {
> >> struct weston_data_offer *offer = wl_resource_get_user_data(resource);
> >>
> >> - if (offer->source)
> >> + if (offer->source) {
> >> + if (offer->source->offer == offer) {
> >> + if (offer->source->accepted)
> >> + wl_data_source_send_dnd_finished(offer->source->resource);
> >> + else
> >> + wl_data_source_send_cancelled(offer->source->resource);
> >
> > First, this needs to be inside a version checks. It should only send
> > wl_data_source_cancelled if the offer version is >= 3, and it should
> > only send the dnd_finished if the source version is >= 3.
>
> sure, the patches altogether didn't add version checks because git
> grep SINCE_VERSION didn't bring many hits, will add those all across
> now.
>
> >
> > This made me think of an issue with the progress notification. How will
> > a failed operation be communicated? For example, lets pretend we have
> > the actions and the progress protocol in place.
> >
> > source.offer("abc")
> > source.set_actions(move)
> > data_device.enter
> > offer.accept("abc")
> > offer.set_actions(move)
> > source.action(move)
> > offer.action(move)
> > data_device.drop
> > *fails to perform move*
> > offer.destroy
> > source.dnd_finished
> > *removes data*
> >
> > Shouldn't we not send source.dnd_finished here? How would the offer
> > communicate that, while it accepted the action and mime type? Looking
> > for the .receive request I suspect is not enough, because it can still
> > fail to write the file due to the filesystem beeing full etc.
>
> Hmm, is this in the context of file managers? As was partly implied in
> my previous reply to wayland 2/2, I think the confirmation/denial of
> DnD success should exclusively stick to the data being sent through
> the fd. Under that light there's no failure to move, there's a data
> transfer and an ack of a move operation, any other failure in between
> (eg. fd prematurely closed) is a client error, and the actions
> resulting from this (move or copy+delete the URI list in the file
> manager case) are domain of the app.
>
> Keep in mind that file managers are kind of an strange case, most of
> the times DnD is exclusively about the content being transferred
> through it, although traditionally DnD has been deliberately flexible,
> turning a drop operation into a list of actions to perform is
> certainly amongst the possibilities.
File managers was what I thought of first, but I think to have a "move"
in anyway needs a way to fail grasefully without data loss. Think of DND
move of some text between two word processors that are separate
processes. If the destination word processor is editing a remote
document and it fails to insert the text because of some network error,
it has no way to cancel an already initiated move. This example might
not make much sense, but my point is that a "move" can fail mid-"move"
and shouldn't we make it at least possible to fail gracefully?
Really any time the data is not just an URI where the source side
"delete" is not a no-op.
A way of doing this would be to add a "finish" request to wl_data_source
that is sent by the destination before destroying the wl_data_source.
That way the compositor can send a "cancel" if no finish was sent, or
finished if a finish was sent.
Jonas
>
> >
> >> +
> >> + offer->source->offer = NULL;
> >> + }
> >> +
> >> wl_list_remove(&offer->source_destroy_listener.link);
> >> + }
> >> free(offer);
> >> }
> >>
> >> @@ -147,6 +163,9 @@ weston_data_source_send_offer(struct weston_data_source *source,
> >> wl_array_for_each(p, &source->mime_types)
> >> wl_data_offer_send_offer(offer->resource, *p);
> >>
> >> + source->offer = offer;
> >> + source->accepted = 0;
> >
> > s/0/false/.
> >
> >> +
> >> return offer->resource;
> >> }
> >>
> >> @@ -301,6 +320,7 @@ weston_drag_set_focus(struct weston_drag *drag,
> >> serial = wl_display_next_serial(display);
> >>
> >> if (drag->data_source) {
> >> + drag->data_source->accepted = 0;
> >> offer = weston_data_source_send_offer(drag->data_source,
> >> resource);
> >> if (offer == NULL)
> >> @@ -398,9 +418,12 @@ drag_grab_button(struct weston_pointer_grab *grab,
> >> enum wl_pointer_button_state state = state_w;
> >>
> >> if (drag->base.focus_resource &&
> >> + drag->base.data_source &&
> >> pointer->grab_button == button &&
> >> - state == WL_POINTER_BUTTON_STATE_RELEASED)
> >> + state == WL_POINTER_BUTTON_STATE_RELEASED) {
> >> wl_data_device_send_drop(drag->base.focus_resource);
> >> + wl_data_source_send_dnd_drop_performed(drag->base.data_source->resource);
> >
> > Needs to be inside a data_source version >= 3 check.
> >
> >> + }
> >>
> >> if (pointer->button_count == 0 &&
> >> state == WL_POINTER_BUTTON_STATE_RELEASED) {
> >> @@ -878,11 +901,13 @@ create_data_source(struct wl_client *client,
> >> source->accept = client_source_accept;
> >> source->send = client_source_send;
> >> source->cancel = client_source_cancel;
> >> + source->offer = NULL;
> >> + source->accepted = 0;
> >
> > s/0/false/.
> >
> >>
> >> wl_array_init(&source->mime_types);
> >>
> >> source->resource =
> >> - wl_resource_create(client, &wl_data_source_interface, 1, id);
> >> + wl_resource_create(client, &wl_data_source_interface, 3, id);
> >
> > The 1 here was incorrect, so lets fix this while at it. It should be
> >
> > source->resource =
> > wl_resource_create(client, &wl_data_source_interface,
> > wl_resource_get_version(resource), id);
> >
> > Seems weston_data_source_send_offer has the same bug when creating the
> > offer object, so it's probably needed to fix that place as well in order
> > to be able to do the version checking.
>
> Indeed, will fix those.
>
> Cheers,
> Carlos
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list