[PATCH weston 1/5] data-device: Implement DnD progress notification
Carlos Garnacho
carlosg at gnome.org
Wed Dec 16 07:03:14 PST 2015
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.
>
>> +
>> + 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
More information about the wayland-devel
mailing list