GNOME Bugzilla – Bug 749872
wayland: Port text_input protocol
Last modified: 2021-07-05 13:48:36 UTC
The motivation behind this is to enable IBus on xdg-app. Instead of having a complex and inherently insecure setup (i.e., forwarding the IBus socket/envvar to the application and including IBus libs in the bundle), this patch allows an application to communicate with IBus through the Wayland IM protocol as a transport. The protocol is not ideal, but it is sufficient to wrap the necessary IBus messages.
Created attachment 303966 [details] [review] wayland: Port input-method protocol This is a port of the input-method protocol in Weston. The implementation is based on src/text-backend.c. Some implementation details: - The server merely relays messages between two clients: an input-method and an application. - The input-method should be spawned as a privileged client, which could be implemented using weston_client_launch() in Weston. However, there's no equivalent in mutter yet. - Only one input-method per keyboard is allowed, while the original code seems to allow multiple input-methods per seat (though I'm not sure of the use-case nor how to enable them). - MetaWaylandKeyboard now has a grab mechanism as in MetaWaylandPointer. This is needed to intercept key events to an application and deliver them to an input-method. - The input panel stuff is not implemented. The supposed use-case of it was to render an OSK, but we do that in a different way.
I would suggest to just use the text protocol part and rather relay it to a GtkIMContext instead of the input-method protocol part. The StIMText component in gnome-shell already uses GtkIMContext. I also work on an update for the text protocol to fix some problems there.
Thanks for the comment, Jan. I'm not sure if I get what you mean here exactly, but if we use GtkIMContext, it loads the im-ibus module, which in turn talks to IBus through a socket. The socket is not exposed to sandboxed apps, and the socket address can change when a user restarts IBus. So, I think we would eventually need the input-method part of the protocol; maybe it could be simplified to resemble the GtkIMContext interface, and combined with the text protocol?
My idea would be that the compositor uses GtkIMContext and so the socket would only be exposed to the compositor. The text protocol would be still used for applications to not have to talk to the IBus socket but just to the compositor.
Created attachment 319533 [details] [review] wayland: Implement text_input_unstable_v1 protocol With this patch, mutter internally loads an immodule specified by GTK_IM_MODULE and translates wayland protocol messages into corresponding GtkIMContext method calls. This is the counterpart of the GTK patch adding "platform" immodule, which does the inverse of this patch; turns GtkIMContext method calls into wayland protocol messages (bug 698307).
Review of attachment 319533 [details] [review]: Thanks for working on this. I have a few comments below but this seems mostly reasonable. ::: src/wayland/meta-wayland-keyboard.h @@ +59,3 @@ } MetaWaylandXkbInfo; +struct _MetaWaylandKeyboardGrabInterface You probably did this before https://git.gnome.org/browse/mutter/commit/?id=ec9abaf1ef42546ffcb69b6343f97438267bb20d landed so now this will need to be adapted to use that code. ::: src/wayland/meta-wayland-text-input.c @@ +108,3 @@ + + g_clear_object (&self->context); + g_clear_object (&self->simple_context); There's no reason to have a _dispose() function here. Please move this to finalize which is easier to reason about. @@ +261,3 @@ + /* Divert to the default grab, if the event is not handled. */ + MetaWaylandKeyboardGrab *default_grab = &grab->keyboard->default_grab; + default_grab->interface->key (default_grab, time, key, state); We should be saving the existing MetaWaylandKeyboard->grab when we get our grab and divert there instead of assuming the event should go to the default grab. @@ +410,3 @@ + gboolean repeat; + + repeat = g_settings_get_boolean (keyboard->settings, "repeat"); I'd rather we didnt' have yet another place handling key repeat. We get key repeat from clutter up to meta_wayland_keyboard_handle_event() so if we need key repeat to feed the GtkIMContext we should hook this from there somehow to avoid having all this repeat handling code here. @@ +511,3 @@ + MetaWaylandKeyboard *keyboard = &seat->keyboard; + + zwp_text_input_v1_send_enter (resource, surface_resource); We should only send the enter event at the end of this function to avoid any possibility of races here. And in fact we might even want to deny activation, see below. @@ +514,3 @@ + + if (keyboard->grab != &keyboard->default_grab) + meta_wayland_keyboard_end_grab (keyboard); This allows text protocol clients to essentially override grabs in the compositor unconditionally. I think in this case we should just deny the activate request.
Created attachment 319729 [details] [review] wayland: Implement text_input_unstable_v1 protocol With this patch, mutter internally loads an immodule specified by GTK_IM_MODULE and translates wayland protocol messages into corresponding GtkIMContext method calls. This is the counterpart of the GTK patch adding "platform" immodule, which does the inverse of this patch; turns GtkIMContext method calls into wayland protocol messages (bug 698307).
Created attachment 319730 [details] [review] wayland: Don't send unnecessary wl_keyboard.leave meta_wayland_keyboard_start_grab() called meta_wayland_keyboard_set_focus(), which results in sending wl_keyboard.leave. This would cause busy loop when start_grab() is called from the text_input.activate protocol handler. Since all other callers of start_grab() explicitly call set_focus() as needed, don't touch focused surface in start_grab().
(In reply to Rui Matos from comment #6) > You probably did this before > https://git.gnome.org/browse/mutter/commit/ > ?id=ec9abaf1ef42546ffcb69b6343f97438267bb20d landed so now this will need to > be adapted to use that code. I didn't notice that change; thanks for the pointer. The code should now be adjusted to use the new API. Regarding the API, one thing I noticed is the start_grab() function always reset focused surface to NULL, and that causes infloop when used in this patch. I've attached a quick fix for that as comment 8. > ::: src/wayland/meta-wayland-text-input.c > @@ +108,3 @@ > + > + g_clear_object (&self->context); > + g_clear_object (&self->simple_context); > > There's no reason to have a _dispose() function here. Please move this to > finalize which is easier to reason about. Sure, done. > @@ +261,3 @@ > + /* Divert to the default grab, if the event is not handled. */ > + MetaWaylandKeyboardGrab *default_grab = &grab->keyboard->default_grab; > + default_grab->interface->key (default_grab, time, key, state); > > We should be saving the existing MetaWaylandKeyboard->grab when we get our > grab and divert there instead of assuming the event should go to the default > grab. As we now simply deny text_input.activate when other part of the compositor has a grab, this code is no longer necessary. Removed it. > @@ +410,3 @@ > + gboolean repeat; > + > + repeat = g_settings_get_boolean (keyboard->settings, "repeat"); > > I'd rather we didnt' have yet another place handling key repeat. We get key > repeat from clutter up to meta_wayland_keyboard_handle_event() so if we need > key repeat to feed the GtkIMContext we should hook this from there somehow > to avoid having all this repeat handling code here. I agree, but I have no idea how to do that in the mutter side. I guess it could better be done in the gtk+ patch, where text_input.keysym is handled. > @@ +511,3 @@ > + MetaWaylandKeyboard *keyboard = &seat->keyboard; > + > + zwp_text_input_v1_send_enter (resource, surface_resource); > > We should only send the enter event at the end of this function to avoid any > possibility of races here. And in fact we might even want to deny > activation, see below. Sure, moved it to the end. > @@ +514,3 @@ > + > + if (keyboard->grab != &keyboard->default_grab) > + meta_wayland_keyboard_end_grab (keyboard); > > This allows text protocol clients to essentially override grabs in the > compositor unconditionally. I think in this case we should just deny the > activate request. Changed this to just return from the handler.
Review of attachment 319730 [details] [review]: Right, focus around grabs is better left to callers who know better what should happen
Review of attachment 319729 [details] [review]: I still need to think about the repeat key issue better but this side of the protocol is looking fine. I'll just leave it in reviewed for now until we come up with better answers for breaking grabs. ::: src/wayland/meta-wayland-text-input.c @@ +301,3 @@ + return; + + meta_wayland_keyboard_end_grab (keyboard); We'll need a way to break the grab other than the client telling us to. Otherwise, if you, say, focus a different window, and the previously focused client (maliciously) doesn't deactivate, we'd still be sending it text input. @@ +452,3 @@ + MetaWaylandTextInput *text_input = wl_resource_get_user_data (resource); + + text_input->serial = serial; This request will need to be better specified in the protocol. Why is the client even sending a value? Shouldn't it be the compositor generating serial values? i.e. this could just be text_input->serial++ @@ +496,3 @@ + text_input->resource = wl_resource_create (client, + &zwp_text_input_v1_interface, + 1, wl_resource_get_version (resource)
Created attachment 319892 [details] [review] wayland: Implement text_input_unstable_v1 protocol With this patch, mutter internally loads an immodule specified by GTK_IM_MODULE and translates wayland protocol messages into corresponding GtkIMContext method calls. This is the counterpart of the GTK patch adding "platform" immodule, which does the inverse of this patch; turns GtkIMContext method calls into wayland protocol messages (bug 698307).
(In reply to Rui Matos from comment #11) > Review of attachment 319729 [details] [review] [review]: > > I still need to think about the repeat key issue better but this side of the > protocol is looking fine. I'll just leave it in reviewed for now until we > come up with better answers for breaking grabs. Sure, let's think a bit more about it. > + MetaWaylandTextInput *text_input = wl_resource_get_user_data (resource); > + > + text_input->serial = serial; > > This request will need to be better specified in the protocol. Why is the > client even sending a value? Shouldn't it be the compositor generating > serial values? i.e. this could just be text_input->serial++ I suppose the serial is used for the input method protocol, where input method is implemented as a privileged client and can appear/disappear multiple times. In that case, clients could ignore events from the former input method, by checking the serials. Since we don't use the protocol anymore, I guess it is no longer necessary. > @@ +496,3 @@ > + text_input->resource = wl_resource_create (client, > + &zwp_text_input_v1_interface, > + 1, > > wl_resource_get_version (resource) Done. In addition to it, I changed the text_input.keysym emission with wl_keyboard.key, with that key repeat seems to work for non-letter keys.
Note that a new version of the protocol was just posted: http://lists.freedesktop.org/archives/wayland-devel/2016-January/026720.html
Created attachment 319896 [details] [review] wayland: Implement text_input_unstable_v1 protocol With this patch, mutter internally loads an immodule specified by GTK_IM_MODULE and translates wayland protocol messages into corresponding GtkIMContext method calls. This is the counterpart of the GTK patch adding "platform" immodule, which does the inverse of this patch; turns GtkIMContext method calls into wayland protocol messages (bug 698307). -- Sorry, attached a wrong patch.
(In reply to Jonas Ådahl from comment #14) > Note that a new version of the protocol was just posted: > http://lists.freedesktop.org/archives/wayland-devel/2016-January/026720.html Yes I would really suggest to use the new version: * Use only one wp_text_input per wl_seat (client side should be handled by client toolkit) * Allow focus tracking without wl_keyboard present * Improve update state handling * Allow state requests It should also make the focus situation more clear. With the new version to have a surface with active text input focus it needs to be activated (request to use text input/input method) and have text/keyboard focus.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/mutter/-/issues/ Thank you for your understanding and your help.