[PATCH weston v7 2/5] data-device: Implement DnD actions
Jonas Ådahl
jadahl at gmail.com
Thu Jan 14 22:10:40 PST 2016
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.
> + 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.
> +
> + 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.
Jonas
> +}
> +
> static struct wl_data_source_interface data_source_interface = {
> data_source_offer,
> - data_source_destroy
> + data_source_destroy,
> + data_source_set_actions
> };
>
> static void
> @@ -471,7 +616,8 @@ drag_grab_button(struct weston_pointer_grab *grab,
> pointer->grab_button == button &&
> state == WL_POINTER_BUTTON_STATE_RELEASED) {
> if (drag->base.focus_resource &&
> - data_source->accepted) {
> + data_source->accepted &&
> + data_source->current_dnd_action) {
> wl_data_device_send_drop(drag->base.focus_resource);
>
> if (wl_resource_get_version(data_source->resource) >=
> @@ -895,13 +1041,23 @@ data_device_set_selection(struct wl_client *client,
> struct wl_resource *resource,
> struct wl_resource *source_resource, uint32_t serial)
> {
> + struct weston_data_source *data_source;
> +
> if (!source_resource)
> return;
>
> + data_source = wl_resource_get_user_data(source_resource);
> +
> + if (data_source->actions_set) {
> + wl_resource_post_error(source_resource,
> + WL_DATA_SOURCE_ERROR_INVALID_SOURCE,
> + "cannot set drag-and-drop source as selection");
> + return;
> + }
> +
> /* FIXME: Store serial and check against incoming serial here. */
> weston_seat_set_selection(wl_resource_get_user_data(resource),
> - wl_resource_get_user_data(source_resource),
> - serial);
> + data_source, serial);
> }
> static void
> data_device_release(struct wl_client *client, struct wl_resource *resource)
> @@ -980,6 +1136,9 @@ create_data_source(struct wl_client *client,
> source->cancel = client_source_cancel;
> source->offer = NULL;
> source->accepted = false;
> + source->actions_set = false;
> + source->dnd_actions = 0;
> + source->current_dnd_action = WL_DATA_DEVICE_MANAGER_DND_ACTION_NONE;
>
> wl_array_init(&source->mime_types);
>
> --
> 2.5.0
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list