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. 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