[PATCH weston 2/5] data-device: Implement DnD actions
Carlos Garnacho
carlosg at gnome.org
Tue Dec 22 06:17:17 PST 2015
Hey!,
On Tue, Dec 22, 2015 at 5:25 AM, Jonas Ådahl <jadahl at gmail.com> wrote:
> On Tue, Dec 22, 2015 at 02:33:28AM +0100, Carlos Garnacho wrote:
>> The policy in weston in order to determine the chosen DnD action is
>> deliberately simple, and is probably the minimals that any compositor
>> should be doing here.
>>
>> Besides honoring the set_actions requests on both wl_data_source and
>> wl_data_offer, weston now will emit the newly added "action" events
>> notifying both source and dest of the chosen action.
>>
>> The "dnd" client has been updated too (although minimally), so it
>> notifies the compositor of a "move" action on both sides.
>
> dnd.c needs to be updated to require version 3 to function, since you
> now depend on it.
Right
>
>>
>> Changes since v4:
>> - Added compositor-side version checks. Spaces vs tabs fixes.
>> Fixed resource versioning. Initialized new weston_data_source/offer
>> fields.
>>
>> Changes since v3:
>> - Put data_source.action to use in the dnd client, now updates
>> the dnd surface like data_source.target events do.
>>
>> Changes since v2:
>> - Split from DnD progress notification changes.
>>
>> Changes since v1:
>> - Updated to v2 of DnD actions protocol changes, implement
>> wl_data_offer.source_actions.
>> - Fixed coding style issues.
>>
>> Signed-off-by: Carlos Garnacho <carlosg at gnome.org>
>> Reviewed-by: Michael Catanzaro <mcatanzaro at igalia.com>
>
> Mostly looks good, with a few nits below and one above. I suspect some
> changes are needed depending on how the "ask" action should be
> interpeted, so I'll give my RB when that has been cleared out.
>
>> ---
>> clients/dnd.c | 32 ++++++++++++++---
>> clients/window.c | 25 +++++++++++++
>> src/compositor.h | 4 +++
>> src/data-device.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 4 files changed, 157 insertions(+), 8 deletions(-)
>>
>> diff --git a/clients/dnd.c b/clients/dnd.c
>> index 48111d9..ddf3fcc 100644
>> --- a/clients/dnd.c
>> +++ b/clients/dnd.c
>> @@ -72,6 +72,7 @@ struct dnd_drag {
>> struct item *item;
>> int x_offset, y_offset;
>> int width, height;
>> + uint32_t dnd_action;
>> const char *mime_type;
>>
>> struct wl_surface *drag_surface;
>> @@ -266,16 +267,13 @@ dnd_get_item(struct dnd *dnd, int32_t x, int32_t y)
>> }
>>
>> static void
>> -data_source_target(void *data,
>> - struct wl_data_source *source, const char *mime_type)
>> +dnd_drag_update_surface(struct dnd_drag *dnd_drag)
>> {
>> - struct dnd_drag *dnd_drag = data;
>> struct dnd *dnd = dnd_drag->dnd;
>> cairo_surface_t *surface;
>> struct wl_buffer *buffer;
>>
>> - dnd_drag->mime_type = mime_type;
>> - if (mime_type)
>> + if (dnd_drag->mime_type && dnd_drag->dnd_action)
>> surface = dnd_drag->opaque;
>> else
>> surface = dnd_drag->translucent;
>> @@ -288,6 +286,16 @@ data_source_target(void *data,
>> }
>>
>> static void
>> +data_source_target(void *data,
>> + struct wl_data_source *source, const char *mime_type)
>> +{
>> + struct dnd_drag *dnd_drag = data;
>> +
>> + dnd_drag->mime_type = mime_type;
>> + dnd_drag_update_surface(dnd_drag);
>> +}
>> +
>> +static void
>> data_source_send(void *data, struct wl_data_source *source,
>> const char *mime_type, int32_t fd)
>> {
>> @@ -360,12 +368,22 @@ data_source_drag_finished(void *data, struct wl_data_source *source)
>> dnd_drag_destroy(dnd_drag);
>> }
>>
>> +static void
>> +data_source_action(void *data, struct wl_data_source *source, uint32_t dnd_action)
>> +{
>> + struct dnd_drag *dnd_drag = data;
>> +
>> + dnd_drag->dnd_action = dnd_action;
>> + dnd_drag_update_surface(dnd_drag);
>> +}
>> +
>> static const struct wl_data_source_listener data_source_listener = {
>> data_source_target,
>> data_source_send,
>> data_source_cancelled,
>> data_source_drop_performed,
>> data_source_drag_finished,
>> + data_source_action,
>> };
>>
>> static cairo_surface_t *
>> @@ -428,6 +446,8 @@ create_drag_source(struct dnd *dnd,
>> dnd_drag->item = item;
>> dnd_drag->x_offset = x - item->x;
>> dnd_drag->y_offset = y - item->y;
>> + dnd_drag->dnd_action = WL_DATA_DEVICE_MANAGER_DND_ACTION_MOVE;
>> + dnd_drag->mime_type = NULL;
>>
>> for (i = 0; i < ARRAY_LENGTH(dnd->items); i++) {
>> if (item == dnd->items[i]){
>> @@ -461,6 +481,8 @@ create_drag_source(struct dnd *dnd,
>> window_get_wl_surface(dnd->window),
>> dnd_drag->drag_surface,
>> serial);
>> + wl_data_source_set_actions(dnd_drag->data_source,
>> + WL_DATA_DEVICE_MANAGER_DND_ACTION_MOVE);
>>
>> dnd_drag->opaque =
>> create_drag_icon(dnd_drag, item, x, y, 1);
>> diff --git a/clients/window.c b/clients/window.c
>> index db96185..9c67b91 100644
>> --- a/clients/window.c
>> +++ b/clients/window.c
>> @@ -3362,6 +3362,8 @@ struct data_offer {
>> int fd;
>> data_func_t func;
>> int32_t x, y;
>> + uint32_t dnd_action;
>> + uint32_t source_actions;
>> void *user_data;
>> };
>>
>> @@ -3375,8 +3377,26 @@ data_offer_offer(void *data, struct wl_data_offer *wl_data_offer, const char *ty
>> *p = strdup(type);
>> }
>>
>> +static void
>> +data_offer_source_actions(void *data, struct wl_data_offer *wl_data_offer, uint32_t source_actions)
>> +{
>> + struct data_offer *offer = data;
>> +
>> + offer->source_actions = source_actions;
>> +}
>> +
>> +static void
>> +data_offer_action(void *data, struct wl_data_offer *wl_data_offer, uint32_t dnd_action)
>> +{
>> + struct data_offer *offer = data;
>> +
>> + offer->dnd_action = dnd_action;
>> +}
>> +
>> static const struct wl_data_offer_listener data_offer_listener = {
>> data_offer_offer,
>> + data_offer_source_actions,
>> + data_offer_action
>> };
>>
>> static void
>> @@ -3440,6 +3460,11 @@ data_device_enter(void *data, struct wl_data_device *data_device,
>> *p = NULL;
>>
>> types_data = input->drag_offer->types.data;
>> + wl_data_offer_set_actions(offer,
>> + WL_DATA_DEVICE_MANAGER_DND_ACTION_COPY |
>> + WL_DATA_DEVICE_MANAGER_DND_ACTION_MOVE,
>> + WL_DATA_DEVICE_MANAGER_DND_ACTION_MOVE);
>> +
>> } else {
>> input->drag_offer = NULL;
>> types_data = NULL;
>> diff --git a/src/compositor.h b/src/compositor.h
>> index 96a7b0d..9aa0150 100644
>> --- a/src/compositor.h
>> +++ b/src/compositor.h
>> @@ -298,6 +298,8 @@ struct weston_data_offer {
>> struct wl_resource *resource;
>> struct weston_data_source *source;
>> struct wl_listener source_destroy_listener;
>> + uint32_t dnd_actions;
>> + uint32_t preferred_dnd_action;
>
> Shouldn't "preferred_dnd_action" be a enum wl_data_device_action, since
> it's not a mask?
Right, changed locally.
>
>> };
>>
>> struct weston_data_source {
>> @@ -306,6 +308,8 @@ struct weston_data_source {
>> struct wl_array mime_types;
>> struct weston_data_offer *offer;
>> bool accepted;
>> + uint32_t dnd_actions;
>> + uint32_t current_dnd_action;
>
> Same here regarding "current_dnd_action".
>
>>
>> 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 d5b8f02..045cef5 100644
>> --- a/src/data-device.c
>> +++ b/src/data-device.c
>> @@ -101,7 +101,8 @@ data_offer_notify_finish(struct weston_data_offer *offer)
>> offer->source->offer == offer &&
>> wl_resource_get_version(offer->source->resource) >=
>> WL_DATA_SOURCE_DND_FINISHED_SINCE_VERSION) {
>> - if (offer->source->accepted)
>> + if (offer->source->accepted &&
>> + offer->source->current_dnd_action)
>> wl_data_source_send_dnd_finished(offer->source->resource);
>> else
>> wl_data_source_send_cancelled(offer->source->resource);
>> @@ -110,6 +111,72 @@ data_offer_notify_finish(struct weston_data_offer *offer)
>> }
>> }
>>
>> +static uint32_t
>> +data_offer_choose_action(struct weston_data_offer *offer)
>> +{
>> + uint32_t available_actions, preferred_action = 0;
>> + uint32_t source_actions, offer_actions;
>> +
>> + if (wl_resource_get_version (offer->resource) >=
>
> No space between function name and (.
>
>> + WL_DATA_OFFER_ACTION_SINCE_VERSION) {
>> + offer_actions = offer->dnd_actions;
>> + preferred_action = offer->preferred_dnd_action;
>> + } else {
>> + offer_actions = WL_DATA_DEVICE_MANAGER_DND_ACTION_COPY;
>> + }
>> +
>> + if (wl_resource_get_version (offer->source->resource) >=
>
> No space between function name and (.
>
>> + WL_DATA_SOURCE_ACTION_SINCE_VERSION)
>> + source_actions = offer->source->dnd_actions;
>> + else
>> + source_actions = WL_DATA_DEVICE_MANAGER_DND_ACTION_COPY;
>> +
>> + available_actions = offer_actions & source_actions;
>> +
>> + if (!available_actions)
>> + return 0;
>
> WL_DATA_DEVICE_ACTION_NONE.
>
>> +
>> + /* If the dest side has a preferred DnD action, use it */
>> + if ((preferred_action & available_actions) != 0)
>> + return preferred_action;
>> +
>> + /* Use the first found action, in bit order */
>> + return 1 << (ffs(available_actions) - 1);
>> +}
>> +
>> +static void
>> +data_offer_update_action(struct weston_data_offer *offer)
>> +{
>> + uint32_t action;
>> +
>> + action = data_offer_choose_action(offer);
>> +
>> + if (offer->source->current_dnd_action == action)
>> + return;
>> +
>> + offer->source->current_dnd_action = action;
>> +
>> + if (wl_resource_get_version (offer->source->resource) >=
>
> No space between function name and (.
>
>> + WL_DATA_SOURCE_ACTION_SINCE_VERSION)
>> + wl_data_source_send_action(offer->source->resource, action);
>> +
>> + if (wl_resource_get_version (offer->resource) >=
>
> No space between function name and (.
>
>> + WL_DATA_OFFER_ACTION_SINCE_VERSION)
>> + wl_data_offer_send_action(offer->resource, action);
>> +}
>> +
>> +static void
>> +data_offer_set_actions(struct wl_client *client,
>> + struct wl_resource *resource,
>> + uint32_t dnd_actions, uint32_t preferred_action)
>> +{
>> + struct weston_data_offer *offer = wl_resource_get_user_data(resource);
>
> Should we teminate the client if this is called after a receive or
> finish?
After receive it's still acceptable (eg. the drop destination may
preemptively fetch all data mid-drag, before even taking a decision,
actually this is key to webkit2), it is a good idea though to make it
an error if called after finish. Actually, this should also be
mentioned in wl_data_offer.receive docs, I'm adding the following
blurb to wayland 1/2:
This request may happen multiple times for different mimetypes, both
before and after wl_data_device.drop. Drag-and-drop destination clients
may preemptively fetch data or examine it more closely to determine
acceptance.
>
>> +
>> + offer->dnd_actions = dnd_actions;
>> + offer->preferred_dnd_action = preferred_action;
>
> Should we do any input validation? I.e. trying to set invalid actions or
> setting a preferred action that is actually multiple actions, should it
> disconnect the client?
Yeah probably... I'll add that error too. The bitmasks were originally
untouched in order to cater for extra action bits, that makes less
sense now.
Cheers,
Carlos
More information about the wayland-devel
mailing list