GNOME Bugzilla – Bug 760805
Implement wl_data_device_manager v3
Last modified: 2016-01-19 13:29:15 UTC
wl_data_device_manager v3 offers full dnd action support, plus other events to let clients properly track the lifetime of the operation. Attaching the necessary patches, these provide full v3 support, plus the necessary xdnd bridges.
Created attachment 319305 [details] [review] wayland: Add "update" vfunc to MetaWaylandDragDestFuncs This will be useful when an update is due but no motion event is to be sent/received (eg. modifier changes during DnD).
Created attachment 319306 [details] [review] wayland: Add MetaWaylandKeyboardGrab and keyboard grab API This will be useful during DnD, where mutter is expected to consume keyboard events for either allowing changes in the selected DnD action, or misc a11y features like keyboard-driven DnD. Currently, the vtable contains 2 functions, key() will be used on every key event we get from Clutter, modifiers() will notify of changes in the keyboard modifiers (mouse buttons will never be set in the modifier mask)
Created attachment 319307 [details] [review] data-device: Refactor data source management by the drag grab Move to a separate meta_wayland_drag_grab_set_source() so we keep the weak pointer management in a single place.
Created attachment 319308 [details] [review] wayland: Implement DnD actions as per wl_data_device v3 changes We now additionally send: - wl_data_offer.source_actions - wl_data_source.action - wl_data_offer.action - wl_data_source.dnd_drop_performed - wl_data_source.dnd_finished The protocol changes allow for compositors to implement different policies when chosing the action, mutter uses this to reimplement the same behavior that GTK+ traditionally had: - Alt/Control/Shift modifiers change the chosen action to ask/copy/move respectively - Drags with middle button start out as "ask" by default As mutter now also grabs the keyboard and unsets the window focus for these purposes, the window focus is restored after the drag operation has finished. The Xdnd bridge code is also modified to cope with actions, so mixed wayland-x11 scenarios are able to convey that information.
Created attachment 319312 [details] [review] wayland: Implement DnD actions as per wl_data_device v3 changes We now additionally send: - wl_data_offer.source_actions - wl_data_source.action - wl_data_offer.action - wl_data_source.dnd_drop_performed - wl_data_source.dnd_finished The protocol changes allow for compositors to implement different policies when chosing the action, mutter uses this to reimplement the same behavior that GTK+ traditionally had: - Alt/Control/Shift modifiers change the chosen action to ask/copy/move respectively - Drags with middle button start out as "ask" by default As mutter now also grabs the keyboard and unsets the window focus for these purposes, the window focus is restored after the drag operation has finished. The Xdnd bridge code is also modified to cope with actions, so mixed wayland-x11 scenarios are able to convey that information.
Review of attachment 319305 [details] [review]: Looks good to me.
Review of attachment 319306 [details] [review]: ::: src/wayland/meta-wayland-keyboard.c @@ +299,2 @@ static void +notify_modifiers (MetaWaylandKeyboard *keyboard) notify_modifiers() calls the grab interface. notify_key() sends the event. I think it'd be better to do: meta_wayland_keyboard_send_key() sends the key event meta_wayland_keyboard_send_modifiers() sends the modifiers event notify_modifiers() calls the grab interface notify_key() calls the grab interface @@ +746,3 @@ + meta_wayland_keyboard_set_focus (keyboard, NULL); + keyboard->grab = grab; + grab->keyboard = keyboard; Is the grab supposed to prevent changing the focus while active? Is it seems to be now, nothing prevents something else in mutter to change the keyboard focus while the grab is active. ::: src/wayland/meta-wayland-keyboard.h @@ +53,3 @@ +{ + gboolean (* key) (MetaWaylandKeyboardGrab *grab, + const ClutterEvent *event); (* key) should be (*key) I suppose.
Review of attachment 319307 [details] [review]: Looks good to me.
Review of attachment 319312 [details] [review]: ::: src/wayland/meta-wayland-data-device.c @@ +48,3 @@ struct wl_listener source_destroy_listener; + uint32_t dnd_actions; + uint32_t preferred_dnd_action; Use the enum? (here and all other places which is not multiple action bitmask) @@ +60,3 @@ + uint32_t current_dnd_action; + MetaWaylandSeat *seat; + guint actions_set : 1; gboolean? @@ +131,3 @@ + if (wl_resource_get_version (offer->resource) >= + WL_DATA_OFFER_ACTION_SINCE_VERSION) + wl_data_offer_send_action (offer->resource, action); We should only set_current_action and offer_send_action only if we haven't entered "ask" mode right? @@ +362,3 @@ + return; + } + When we drop the trailing action events after dropping, and updated from ask to something else, we should send the new action the source here. ::: src/wayland/meta-xwayland-selection.c @@ +1490,3 @@ + action = WL_DATA_DEVICE_MANAGER_DND_ACTION_MOVE; + else if ((Atom) event->data.l[4] == xdnd_atoms[ATOM_DND_ACTION_ASK]) + action = WL_DATA_DEVICE_MANAGER_DND_ACTION_ASK; Should this maybe be moved to a helper, and reused in meta_xwayland_handle_client_message? You also seem to miss ATOM_DND_ACTION_PRIVATE here.
(In reply to Jonas Ådahl from comment #7) > Review of attachment 319306 [details] [review] [review]: > > ::: src/wayland/meta-wayland-keyboard.c > @@ +299,2 @@ > static void > +notify_modifiers (MetaWaylandKeyboard *keyboard) > > notify_modifiers() calls the grab interface. > notify_key() sends the event. > > I think it'd be better to do: > meta_wayland_keyboard_send_key() sends the key event > meta_wayland_keyboard_send_modifiers() sends the modifiers event > notify_modifiers() calls the grab interface > notify_key() calls the grab interface Oh yes indeed, missed the inconsistence here. > > @@ +746,3 @@ > + meta_wayland_keyboard_set_focus (keyboard, NULL); > + keyboard->grab = grab; > + grab->keyboard = keyboard; > > Is the grab supposed to prevent changing the focus while active? Is it seems > to be now, nothing prevents something else in mutter to change the keyboard > focus while the grab is active. Right, that isn't handled explicitly. Although I expected the combined pointer+keyboard grabs to replace all places where that could happen. In the future I'd like to see some futher grab vfuncs so we fully undo the "operation mode" of a grab before we enter the next, instead of doing things manually before the new grab/semantics are in effect. > > ::: src/wayland/meta-wayland-keyboard.h > @@ +53,3 @@ > +{ > + gboolean (* key) (MetaWaylandKeyboardGrab *grab, > + const ClutterEvent *event); > > (* key) should be (*key) I suppose. Indeed. (In reply to Jonas Ådahl from comment #9) > Review of attachment 319312 [details] [review] [review]: > > ::: src/wayland/meta-wayland-data-device.c > @@ +48,3 @@ > struct wl_listener source_destroy_listener; > + uint32_t dnd_actions; > + uint32_t preferred_dnd_action; > > Use the enum? (here and all other places which is not multiple action > bitmask) Sure > > @@ +60,3 @@ > + uint32_t current_dnd_action; > + MetaWaylandSeat *seat; > + guint actions_set : 1; > > gboolean? I tend to pack boolean things in bits in internal structs in order to save some space for possible future fields. Given there's not going to be a lot of this, nor a lot of previsible further state in this struct, I'm more indifferent, Although... > > @@ +131,3 @@ > + if (wl_resource_get_version (offer->resource) >= > + WL_DATA_OFFER_ACTION_SINCE_VERSION) > + wl_data_offer_send_action (offer->resource, action); > > We should only set_current_action and offer_send_action only if we haven't > entered "ask" mode right? Right. I guess this deserves a guint in_ask : 1; :) > > @@ +362,3 @@ > + return; > + } > + > > When we drop the trailing action events after dropping, and updated from ask > to something else, we should send the new action the source here. > > ::: src/wayland/meta-xwayland-selection.c > @@ +1490,3 @@ > + action = WL_DATA_DEVICE_MANAGER_DND_ACTION_MOVE; > + else if ((Atom) event->data.l[4] == > xdnd_atoms[ATOM_DND_ACTION_ASK]) > + action = WL_DATA_DEVICE_MANAGER_DND_ACTION_ASK; > > Should this maybe be moved to a helper, and reused in > meta_xwayland_handle_client_message? You also seem to miss > ATOM_DND_ACTION_PRIVATE here. A helper makes sense indeed, and I guess the best way to handle XdndActionPrivate is to default to WL_DATA_DEVICE_MANAGER_DND_ACTION_COPY, althought I'm somewhat unsure on that one.
Created attachment 319338 [details] [review] wayland: Add MetaWaylandKeyboardGrab and keyboard grab API This will be useful during DnD, where mutter is expected to consume keyboard events for either allowing changes in the selected DnD action, or misc a11y features like keyboard-driven DnD. Currently, the vtable contains 2 functions, key() will be used on every key event we get from Clutter, modifiers() will notify of changes in the keyboard modifiers (mouse buttons will never be set in the modifier mask)
Created attachment 319339 [details] [review] wayland: Implement DnD actions as per wl_data_device v3 changes We now additionally send: - wl_data_offer.source_actions - wl_data_source.action - wl_data_offer.action - wl_data_source.dnd_drop_performed - wl_data_source.dnd_finished The protocol changes allow for compositors to implement different policies when chosing the action, mutter uses this to reimplement the same behavior that GTK+ traditionally had: - Alt/Control/Shift modifiers change the chosen action to ask/copy/move respectively - Drags with middle button start out as "ask" by default As mutter now also grabs the keyboard and unsets the window focus for these purposes, the window focus is restored after the drag operation has finished. The Xdnd bridge code is also modified to cope with actions, so mixed wayland-x11 scenarios are able to convey that information.
Thanks for the reviews. These fix all the issues, but the question around keyboard focus management (which ought to be fixed by improving the grabs interfaces, also meant to change as we introduce wl_tablet and other misc foci/pointers), and the s/guint/gboolean/ thing, which I've kept as bitfields. Pushing these now.
Attachment 319305 [details] pushed as 6b88420 - wayland: Add "update" vfunc to MetaWaylandDragDestFuncs Attachment 319307 [details] pushed as f053c09 - data-device: Refactor data source management by the drag grab Attachment 319338 [details] pushed as ec9abaf - wayland: Add MetaWaylandKeyboardGrab and keyboard grab API Attachment 319339 [details] pushed as 9b26694 - wayland: Implement DnD actions as per wl_data_device v3 changes