Hey Jonas, On Fri, Jan 15, 2016 at 7:10 AM, Jonas Ådahl <jad...@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 <carl...@gnome.org> >> Reviewed-by: Michael Catanzaro <mcatanz...@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 _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel