[PATCH weston v6 1/5] data-device: Implement DnD progress notification
Jonas Ådahl
jadahl at gmail.com
Fri Jan 15 18:37:14 PST 2016
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 <jadahl at 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 <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.
>
> 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
More information about the wayland-devel
mailing list