[PATCH weston v6 1/5] data-device: Implement DnD progress notification
Jonas Ådahl
jadahl at gmail.com
Thu Jan 14 21:32:26 PST 2016
On Thu, Jan 14, 2016 at 11:43:39PM +0100, Carlos Garnacho wrote:
> Weston now sends wl_data_source.dnd_drop_performed and .dnd_finished in
> order to notify about the different phases of DnD.
>
> 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 v5:
> - Dissociate source and offer after cancel. Updated to
> apply on top of c9f8f8a7f.
>
> Changes since v4:
> - Make wl_data_offer.finish with the wrong state an error.
>
> Changes since v3:
> - Fixed wl_data_source.dnd_finished vs cancelled emission on
> when interoperating with version < 3 drag destinations.
>
> Changes since v2:
> - Handle wl_data_offer.finish. Fixed commit log inconsistencies.
> Added version checks. Spaces vs tabs fixes. Fixed resource
> versioning.
>
> Changes since v1:
> - Updated to protocol v2.
>
> Signed-off-by: Carlos Garnacho <carlosg at gnome.org>
> Reviewed-by: Michael Catanzaro <mcatanzaro at igalia.com>
Mostly looks good to me. I have a couple of minor requests though. See
inline comments. With those fixed, this is Reviewed-by: Jonas Ådahl
<jadahl at gmail.com>.
> ---
> clients/dnd.c | 39 +++++++++++++++-----
> clients/window.c | 3 +-
> src/compositor.h | 3 ++
> src/data-device.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> 4 files changed, 132 insertions(+), 18 deletions(-)
>
> diff --git a/clients/dnd.c b/clients/dnd.c
> index e6a0c39..48111d9 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)
> +dnd_drag_destroy(struct dnd_drag *dnd_drag)
> {
> - 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. */
> + dnd_drag_destroy(dnd_drag);
> +}
> +
> +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.
> + */
> + dnd_drag_destroy(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,
nit: These should be data_source_dnd_drop_performed and
data_source_dnd_finished.
> };
>
> static cairo_surface_t *
> diff --git a/clients/window.c b/clients/window.c
> index 5d69116..c93edb1 100644
> --- a/clients/window.c
> +++ b/clients/window.c
> @@ -3726,6 +3726,7 @@ offer_io_func(struct task *task, uint32_t events)
>
> if (len == 0) {
> close(offer->fd);
> + wl_data_offer_finish(offer->offer);
> data_offer_destroy(offer);
> }
> }
> @@ -5318,7 +5319,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 130b258..8d6b051 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -317,6 +317,9 @@ struct weston_data_source {
> struct wl_resource *resource;
> struct wl_signal destroy_signal;
> struct wl_array mime_types;
> + struct weston_data_offer *offer;
> + struct weston_seat *seat;
> + 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 54541b3..d2b89bb 100644
> --- a/src/data-device.c
> +++ b/src/data-device.c
> @@ -62,12 +62,16 @@ 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)
> - offer->source->accept(offer->source, serial, mime_type);
> + offer->source->accept(offer->source, serial, mime_type);
> + offer->source->accepted = mime_type != NULL;
> }
>
> static void
> @@ -76,7 +80,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);
> @@ -88,10 +92,48 @@ data_offer_destroy(struct wl_client *client, struct wl_resource *resource)
> wl_resource_destroy(resource);
> }
>
> +static void
> +data_offer_notify_finish(struct weston_data_offer *offer)
> +{
> + /* Protect against untimely calls from older data offers */
> + if (!offer->source || offer != offer->source->offer)
> + return;
> +
> + if (wl_resource_get_version(offer->source->resource) >=
> + WL_DATA_SOURCE_DND_FINISHED_SINCE_VERSION) {
> + wl_data_source_send_dnd_finished(offer->source->resource);
> + }
> +
> + offer->source->offer = NULL;
> +}
> +
> +static void
> +data_offer_finish(struct wl_client *client, struct wl_resource *resource)
> +{
> + struct weston_data_offer *offer = wl_resource_get_user_data(resource);
> +
> + if (!offer->source || offer->source->offer != offer)
> + return;
> +
> + /* Disallow finish while we have a grab driving drag-and-drop, or
> + * if the negotiation is not at the right stage
> + */
> + if (offer->source->seat ||
> + !offer->source->accepted) {
> + wl_resource_post_error(offer->resource,
> + WL_DATA_OFFER_ERROR_INVALID_FINISH,
> + "premature finish request");
> + return;
> + }
> +
> + data_offer_notify_finish(offer);
If you do as my suggestion regarding destroy_data_offer, this line could
be replaced with
if (wl_resource_get_version(offer->source->resource) >=
WL_DATA_SOURCE_DND_FINISHED_SINCE_VERSION) {
wl_data_source_send_dnd_finished(offer->source->resource);
or otherwise remove the offer->source->offer check in
data_offer_notify_finish().
> +}
> +
> static const struct wl_data_offer_interface data_offer_interface = {
> data_offer_accept,
> data_offer_receive,
> data_offer_destroy,
> + data_offer_finish,
> };
>
> static void
> @@ -99,8 +141,21 @@ 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 the drag destination has version < 3, wl_data_offer.finish
> + * won't be called, so do this here as a safety net, because
> + * we still want the version >=3 drag source to be happy.
> + */
> + if (wl_resource_get_version(offer->source->resource) <
> + WL_DATA_SOURCE_DND_FINISHED_SINCE_VERSION) {
> + data_offer_notify_finish(offer);
> + } else {
> + wl_data_source_send_cancelled(offer->source->resource);
> + offer->source->offer = NULL;
> + }
How about changing this function to
static void
destroy_data_offer(struct wl_resource *resource)
{
struct weston_data_offer *offer = wl_resource_get_user_data(resource);
if (!offer->source)
goto out;
wl_list_remove(&offer->source_destroy_listener.link);
if (offer->source->offer != offer)
goto out;
/* Only cancel if the source object supports finishing. */
if (wl_resource_get_version(offer->source->resource) >=
WL_DATA_SOURCE_DND_FINISHED_SINCE_VERSION)
wl_data_source_send_cancelled(offer->source->resource);
offer->source->offer = NULL;
out:
free(offer);
}
This way you have the same semantics, and still clean up
offer->source->offer (which is the only thing data_offer_notify_finish()
does anyway here, without pretending to notify finish.
> +
> wl_list_remove(&offer->source_destroy_listener.link);
> + }
> free(offer);
> }
>
> @@ -147,6 +202,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 = false;
> +
> return offer->resource;
> }
>
> @@ -272,6 +330,7 @@ weston_drag_set_focus(struct weston_drag *drag,
> {
> struct wl_resource *resource, *offer = NULL;
> struct wl_display *display = seat->compositor->wl_display;
> + struct weston_data_offer *data_offer;
nit: You call the weston_data_offer "offer" everywhere else. I think
it'd be better to stay consistent and rename the current "offer" to
"offer_resource".
Jonas
> uint32_t serial;
>
> if (drag->focus && view && drag->focus->surface == view->surface) {
> @@ -293,6 +352,15 @@ weston_drag_set_focus(struct weston_drag *drag,
> wl_resource_get_client(view->surface->resource) != drag->client)
> return;
>
> + if (drag->data_source &&
> + drag->data_source->offer) {
> + /* Unlink the offer from the source */
> + data_offer = drag->data_source->offer;
> + data_offer->source = NULL;
> + drag->data_source->offer = NULL;
> + wl_list_remove(&data_offer->source_destroy_listener.link);
> + }
> +
> resource = wl_resource_find_for_client(&seat->drag_resource_list,
> wl_resource_get_client(view->surface->resource));
> if (!resource)
> @@ -301,6 +369,7 @@ weston_drag_set_focus(struct weston_drag *drag,
> serial = wl_display_next_serial(display);
>
> if (drag->data_source) {
> + drag->data_source->accepted = false;
> offer = weston_data_source_send_offer(drag->data_source,
> resource);
> if (offer == NULL)
> @@ -396,11 +465,24 @@ drag_grab_button(struct weston_pointer_grab *grab,
> container_of(grab, struct weston_pointer_drag, grab);
> struct weston_pointer *pointer = drag->grab.pointer;
> enum wl_pointer_button_state state = state_w;
> + struct weston_data_source *data_source = drag->base.data_source;
>
> - if (drag->base.focus_resource &&
> + if (data_source &&
> pointer->grab_button == button &&
> - state == WL_POINTER_BUTTON_STATE_RELEASED)
> - wl_data_device_send_drop(drag->base.focus_resource);
> + state == WL_POINTER_BUTTON_STATE_RELEASED) {
> + if (drag->base.focus_resource &&
> + data_source->accepted) {
> + wl_data_device_send_drop(drag->base.focus_resource);
> +
> + if (wl_resource_get_version(data_source->resource) >=
> + WL_DATA_SOURCE_DND_DROP_PERFORMED_SINCE_VERSION)
> + wl_data_source_send_dnd_drop_performed(data_source->resource);
> +
> + data_source->seat = NULL;
> + } else {
> + wl_data_source_send_cancelled(data_source->resource);
> + }
> + }
>
> if (pointer->button_count == 0 &&
> state == WL_POINTER_BUTTON_STATE_RELEASED) {
> @@ -712,6 +794,8 @@ data_device_start_drag(struct wl_client *client, struct wl_resource *resource,
>
> if (ret < 0)
> wl_resource_post_no_memory(resource);
> + else
> + source->seat = seat;
> }
>
> static void
> @@ -882,7 +966,8 @@ create_data_source(struct wl_client *client,
> }
>
> source->resource =
> - wl_resource_create(client, &wl_data_source_interface, 1, id);
> + wl_resource_create(client, &wl_data_source_interface,
> + wl_resource_get_version(resource), id);
> if (source->resource == NULL) {
> free(source);
> wl_resource_post_no_memory(resource);
> @@ -893,6 +978,8 @@ 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 = false;
>
> wl_array_init(&source->mime_types);
>
> @@ -971,7 +1058,7 @@ WL_EXPORT int
> wl_data_device_manager_init(struct wl_display *display)
> {
> if (wl_global_create(display,
> - &wl_data_device_manager_interface, 2,
> + &wl_data_device_manager_interface, 3,
> NULL, bind_manager) == NULL)
> return -1;
>
> --
> 2.5.0
>
> _______________________________________________
> 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