On Fri, Jan 15, 2016 at 09:09:46PM +0100, Carlos Garnacho wrote: > Hey Jonas, > > On Fri, Jan 15, 2016 at 6:32 AM, Jonas Ådahl <jad...@gmail.com> wrote: > > 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 <carl...@gnome.org> > >> Reviewed-by: Michael Catanzaro <mcatanz...@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 > > <jad...@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. > > Indeed. > > > > >> }; > >> > >> 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(). > > I've gone for the latter (the check can indeed be removed), more > explanations below. > > > > >> +} > >> + > >> 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. > > Oh, I see what confused you, actually a bug: we should be checking the > offer version before notify_finish(), not the source. > > If we drag from a v3 data source to a v2 data offer, we must signal > wl_data_source.dnd_finished at some point, and the destructor seems > the most natural place since the offer wont get .finish() at any > point.
Ah right, indeed, thanks for clearing that out. Jonas > > I renamed that function to be data_source_notify_finish(), since we > only manipulate the source there, and makes this a bit more clear. > I've however adapted the destroy_data_offer() code flow to be as you > suggest. > > > > >> + > >> 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". > > Sure. > > Cheers, > Carlos _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel