[PATCH weston v7 2/5] data-device: Implement DnD actions
Carlos Garnacho
carlosg at gnome.org
Fri Jan 15 11:57:53 PST 2016
Hey Jonas,
On Fri, Jan 15, 2016 at 7:10 AM, Jonas Ådahl <jadahl at gmail.com> wrote:
> On Thu, Jan 14, 2016 at 11:46:34PM +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.
>>
>> Changes since v6:
>> - Emit errors as defined in DnD actions patch v10.
>>
>> Changes since v5:
>> - Use enum types and values for not-a-bitfield stored values.
>> handle errors when finding unexpected dnd_actions values.
>>
>> 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, except for one part regarding the order of
> wl_data_source.set_actions and wl_data_device.start_drag. I also have a
> question regarding what to do with events during "ask". See the comments
> inline.
>
>> ---
>> clients/dnd.c | 32 +++++++++--
>> clients/window.c | 25 ++++++++
>> src/compositor.h | 5 ++
>> src/data-device.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 4 files changed, 221 insertions(+), 10 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 c93edb1..db1fe57 100644
>> --- a/clients/window.c
>> +++ b/clients/window.c
>> @@ -3323,6 +3323,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;
>> };
>>
>> @@ -3336,8 +3338,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
>> @@ -3401,6 +3421,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 8d6b051..c53949e 100644
>> --- a/src/compositor.h
>> +++ b/src/compositor.h
>> @@ -311,6 +311,8 @@ struct weston_data_offer {
>> struct wl_resource *resource;
>> struct weston_data_source *source;
>> struct wl_listener source_destroy_listener;
>> + uint32_t dnd_actions;
>> + enum wl_data_device_manager_dnd_action preferred_dnd_action;
>> };
>>
>> struct weston_data_source {
>> @@ -320,6 +322,9 @@ struct weston_data_source {
>> struct weston_data_offer *offer;
>> struct weston_seat *seat;
>> bool accepted;
>> + bool actions_set;
>> + uint32_t dnd_actions;
>> + enum wl_data_device_manager_dnd_action 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 d2b89bb..18448bf 100644
>> --- a/src/data-device.c
>> +++ b/src/data-device.c
>> @@ -56,6 +56,10 @@ struct weston_touch_drag {
>> struct weston_touch_grab grab;
>> };
>>
>> +#define ALL_ACTIONS (WL_DATA_DEVICE_MANAGER_DND_ACTION_COPY | \
>> + WL_DATA_DEVICE_MANAGER_DND_ACTION_MOVE | \
>> + WL_DATA_DEVICE_MANAGER_DND_ACTION_ASK)
>> +
>> static void
>> data_offer_accept(struct wl_client *client, struct wl_resource *resource,
>> uint32_t serial, const char *mime_type)
>> @@ -107,6 +111,89 @@ data_offer_notify_finish(struct weston_data_offer *offer)
>> offer->source->offer = NULL;
>> }
>>
>> +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) >=
>> + 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) >=
>> + 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 WL_DATA_DEVICE_MANAGER_DND_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;
>> +
>> + if (!offer->source)
>> + return;
>> +
>> + action = data_offer_choose_action(offer);
>> +
>> + if (offer->source->current_dnd_action == action)
>> + return;
>> +
>> + offer->source->current_dnd_action = action;
>> +
>
> I wonder if we can't avoid sending these, after "ask", and only send a
> final wl_data_source_send_action(..) just before the
> wl_data_source_send_finished(..). This would be done by setting a state
> state on weston_data_offer at the time of
> wl_data_source_send_dnd_drop_performed()/wl_data_device_drop() that the
> offer is now in "ask" mode. If it is, we'd not send any of these actions
> (since they might not represent the correct actual action).
>
> Later in the notify_finish() function, we'd do if
> (offer->is_in_ask_state) wl_data_offer_send_action(...). Any use doing
> this, or should we just let all the events through, even after "ask" was
> the action at the drop? I'm not sure the current spec clarifies whether
> this happens or not.
Right, this was left out. Observed in my next batch of patches.
>
>> + if (wl_resource_get_version(offer->source->resource) >=
>> + WL_DATA_SOURCE_ACTION_SINCE_VERSION)
>> + wl_data_source_send_action(offer->source->resource, action);
>> +
>> + if (wl_resource_get_version(offer->resource) >=
>> + 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);
>> +
>> + if (dnd_actions & ~ALL_ACTIONS) {
>> + wl_resource_post_error(offer->resource,
>> + WL_DATA_OFFER_ERROR_INVALID_ACTION_MASK,
>> + "invalid action mask %x", dnd_actions);
>> + return;
>> + }
>> +
>> + if (__builtin_popcount(preferred_action) > 1) {
>> + wl_resource_post_error(offer->resource,
>> + WL_DATA_OFFER_ERROR_INVALID_ACTION,
>> + "invalid action %x", preferred_action);
>> + return;
>> + }
>> +
>> + offer->dnd_actions = dnd_actions;
>> + offer->preferred_dnd_action = preferred_action;
>> + data_offer_update_action(offer);
>> +}
>> +
>> static void
>> data_offer_finish(struct wl_client *client, struct wl_resource *resource)
>> {
>> @@ -126,6 +213,15 @@ data_offer_finish(struct wl_client *client, struct wl_resource *resource)
>> return;
>> }
>>
>> + if (!offer->source->current_dnd_action ||
>> + offer->source->current_dnd_action ==
>> + WL_DATA_DEVICE_MANAGER_DND_ACTION_ASK) {
>> + wl_resource_post_error(offer->resource,
>> + WL_DATA_OFFER_ERROR_INVALID_OFFER,
>> + "offer finished with an invalid action");
>> + return;
>> + }
>> +
>> data_offer_notify_finish(offer);
>> }
>>
>> @@ -134,6 +230,7 @@ static const struct wl_data_offer_interface data_offer_interface = {
>> data_offer_receive,
>> data_offer_destroy,
>> data_offer_finish,
>> + data_offer_set_actions,
>> };
>>
>> static void
>> @@ -183,7 +280,8 @@ weston_data_source_send_offer(struct weston_data_source *source,
>>
>> offer->resource =
>> wl_resource_create(wl_resource_get_client(target),
>> - &wl_data_offer_interface, 1, 0);
>> + &wl_data_offer_interface,
>> + wl_resource_get_version(source->resource), 0);
>> if (offer->resource == NULL) {
>> free(offer);
>> return NULL;
>> @@ -192,6 +290,8 @@ weston_data_source_send_offer(struct weston_data_source *source,
>> wl_resource_set_implementation(offer->resource, &data_offer_interface,
>> offer, destroy_data_offer);
>>
>> + offer->dnd_actions = 0;
>> + offer->preferred_dnd_action = WL_DATA_DEVICE_MANAGER_DND_ACTION_NONE;
>> offer->source = source;
>> offer->source_destroy_listener.notify = destroy_offer_data_source;
>> wl_signal_add(&source->destroy_signal,
>> @@ -204,6 +304,7 @@ weston_data_source_send_offer(struct weston_data_source *source,
>>
>> source->offer = offer;
>> source->accepted = false;
>> + data_offer_update_action(offer);
>>
>> return offer->resource;
>> }
>> @@ -230,9 +331,53 @@ data_source_destroy(struct wl_client *client, struct wl_resource *resource)
>> wl_resource_destroy(resource);
>> }
>>
>> +static void
>> +data_source_set_actions(struct wl_client *client,
>> + struct wl_resource *resource,
>> + uint32_t dnd_actions)
>> +{
>> + struct weston_data_source *source =
>> + wl_resource_get_user_data(resource);
>> +
>> + if (source->actions_set) {
>> + wl_resource_post_error(source->resource,
>> + WL_DATA_SOURCE_ERROR_INVALID_ACTION_MASK,
>> + "cannot set actions more than once");
>> + return;
>> + }
>> +
>> + if (dnd_actions & ~ALL_ACTIONS) {
>> + wl_resource_post_error(source->resource,
>> + WL_DATA_SOURCE_ERROR_INVALID_ACTION_MASK,
>> + "invalid action mask %x", dnd_actions);
>> + return;
>> + }
>> +
>> + if (!source->seat) {
>> + wl_resource_post_error(source->resource,
>> + WL_DATA_SOURCE_ERROR_INVALID_ACTION_MASK,
>> + "invalid action change after "
>> + "wl_data_source.dnd_drop_performed");
>> + return;
>> + }
>
> Shouldn't this be if (source->seat) { error() } ? In the most recent
> protocol version the requirement is that .set_actions() must be called
> before the .start_drag() request, and it's not until .start_drag()
> source->seat is set.
Doh my bad. This means the client needs changing, because it still worked.
>
>> +
>> + source->dnd_actions = dnd_actions;
>> + source->actions_set = true;
>> +
>> + if (source->offer) {
>> + if (wl_resource_get_version(source->offer->resource) >=
>> + WL_DATA_OFFER_SOURCE_ACTIONS_SINCE_VERSION) {
>> + wl_data_offer_send_source_actions(source->offer->resource,
>> + dnd_actions);
>> + }
>> + data_offer_update_action(source->offer);
>> + }
>
> Should this be dropped now? We haven't started the drag, so I guess no
> offer could possibly exist yet.
Ah yes, indeed.
Cheers,
Carlos
More information about the wayland-devel
mailing list