GNOME Bugzilla – Bug 698307
There should be support for the Wayland input method protocol
Last modified: 2018-04-15 00:36:44 UTC
There is an input method protocol in Wayland which should be supported by GTK+
Created attachment 241831 [details] [review] wayland: add input method module
Weston requires currently some patches: http://lists.freedesktop.org/archives/wayland-devel/2013-April/008670.html or use Weston from https://github.com/openismus/weston Beside the weston-keyboard there is also Wayland support for IBus in https://github.com/openismus/ibus
Review of attachment 241831 [details] [review]: text-client-protocol.h and text-protocol.c look generated ? Its fine to include them, but it would be nice to have a) a small top comment that says 'generated - don't edit' or so, and also have a README that explains how they were generated. There's a few coding style issues in the non-generated c files that should be addressed: - space between function_name and ( - line up parameters Other than that, it looks fine to me.
Created attachment 251303 [details] [review] deal with platform-specific im modules
Created attachment 251304 [details] [review] wayland: add input method module
I've fixed up the stylistic issues I had with the patch. Still outstanding: adding a note about how to regenerate the text protocol files. When I got into trying this, I found a couple of issues. First, when using gtk with wayland backend under X, we end up loading the wayland im module, and crash. The first patch I attached fixes this, by doing backend filtering on the available im modules. Second, when using gtk under wayland 1.2, I find that I get into an infinite recursion as soon as I focus an entry.
with a few more fixes that I've pushed, I can now avoid the infinite recursion (but that still needs looking at). So, I can interact with entries under wayland. But no text ever appears, because the weston I'm running does not implement the text protocol. We should probably do something with the key events we are getting in this case.
Hi Matthias, 1. Would be good to copy the .xml in and use @wayland_scanner_rules@ to generated the source files, rather than copy the autogenerated files. 2. I'm confused by your statement about the weston not supporting the text protocol. With Weston 1.2.0 (and earlier I think) it was implemented. You might need to start the keyboard yourself (its under clients)
(In reply to comment #8) > Hi Matthias, > > 1. Would be good to copy the .xml in and use @wayland_scanner_rules@ to > generated the source files, rather than copy the autogenerated files. > > 2. I'm confused by your statement about the weston not supporting the text > protocol. With Weston 1.2.0 (and earlier I think) it was implemented. You might > need to start the keyboard yourself (its under clients) Oh, but I am not running a keyboard, or a server-side input method. Who is responsible for turning key events into text events in that case ?
(In reply to comment #9) > Oh, but I am not running a keyboard, or a server-side input method. Who is > responsible for turning key events into text events in that case ? I guess it would make sense to include something like GtkIMContextSimple for that cases into the Wayland compositor. The compositor should always be able to provide text events.
It really depends on the contract that we want to make between the compositor and the app. If the app is supposed to ignore key events for text input (but obviously not for keynav and activation), then the compositor always needs to generate text events. If text events are optional, the app needs to be prepared to do the imcontextsimple thing client-side to turn key events into text. In the latter case, we really should have a way for the app to know if text events are going to be generated by the compositor or not.
When there is a wl_text_input_manager global available, the compositor must generate text events. I should clarify that in the wl_text_input_manager documentation.
I don't think this is going to make 3.10, at this stage
Did we ever get feedback from Rui on the Wayland IM protocol? I don't really care about it, I think having only IBus is fine.
I'd like to resume the discussion on this, if it is still relevant. It's true that IBus already basically works with Wayland clients. However, that means that if a client is given access to the bus on which ibus-* are running, it can eavesdrop all messages to other clients, including key events and preedit changes (as you can see with: dbus-monitor --address `ibus address` "type='signal'"). I think that negates the security brought by Wayland, and is especially not good for untrusted sandboxed applications. I can think of two possibilities: - Use the Wayland-based protocol (or something which works in a peer-to-peer fashion) to isolate the communication between IBus components from wayland clients - Make the IBus protocol secure The former requires the implementation in gtk+ and gnome-shell, and perhaps the protocol might need further refinement (since IBus <-> wayland bridge is already there, I can adjust it to the new protocol if needed). The latter adds another hack in ibus-daemon, and might make diagnostic of IBus components difficult.
I would much rather do the latter. The Wayland input method protocol was built by Openismus, who no longer exist, isn't maintained, and hasn't been used in real-world applications yet.
In mutter bug 749872, Jan suggested to only use the text protocol and just translate the protocol messages into GtkIMContext method calls. I haven't had much time to look into it, but it sounds reasonable and would be straightforward.
(In reply to Jasper St. Pierre from comment #16) > I would much rather do the latter. The Wayland input method protocol was > built by Openismus, who no longer exist, isn't maintained, and hasn't been > used in real-world applications yet. What does it mean for a protocol to be maintained ? Obviously, if we use the protocol, we will do the necessary changes to it.
(In reply to Jasper St. Pierre from comment #16) > The Wayland input method protocol was built by Openismus, who no longer exist, > isn't maintained, and hasn't been used in real-world applications yet. I already replied to these statements multiple times on wayland IRC and/or mailing list. But once again: * Yes Openismus does not exist anymore, but the great thing about open source is that it does not matter at all. * I am maintaining the protocol (see http://cgit.freedesktop.org/wayland/wayland-protocols/tree/unstable/text-input/README). * There are at least 3 embedded products using that protocol (Which resulted in improvements to the protocol. See WIP: https://github.com/jpetersen/wayland-protocols/blob/master/unstable/text-input/text-input-unstable-v2.xml).
My initial reading of the protocol when I looked at it way back when was fairly negative -- it seemed to be designed for third-party keyboards and similar. GNOME doesn't want to take that approach. I don't know what this protocol offers over the existing IBus one, considering that's the only protocol we expect to be integrated with. If it's "the IBus protocol is insecure", I'd rather we make it secure, because we sort of need to figure out how to make secure DBus protocols, rather than shove another thing in the compositor. Porting to a new protocol which is different from the X11 equivalent for no practical benefit to the user seems like an easy way to introduce new bugs and make the system inconsistent.
The text-input protocol has nothing to do with third-party keyboards. As I proposed in mutter bug 749872 best way is to have the text protocol to map between client applications and a GtkIMContext object on mutter/gnome-shell side. Which makes the whole input method handling consistent inside a scene graph based compositor like mutter/gnome-shell between applications and input fields in gnome-shell itself. In addition to IBus the text-input protocol can also be used for caribou/gnome-shell virtual keyboard without hacks like https://git.gnome.org/browse/caribou/tree/modules/gtk3/caribou-gtk-module.vala and it will also work for all applications supporting the text-input protocol (and not just Gtk+).
Created attachment 319534 [details] [review] wayland: add input method module Add support for an input method module using the Wayland text protocol. -- Rebased against the current git master, and fixed missing cleanups of pending_preedit_* in reset_preedit().
Rui, can you review this, please ?
Review of attachment 319534 [details] [review]: Thanks for the patch! Still needs some work though, let me know if you intend to continue working on it. ::: modules/input/gtkimcontextwayland.c @@ +95,3 @@ +text_input_manager_get (void) +{ + if (!text_input_manager) Since this is going to get hit for each IM context in the process we should ensure we really only try to bind the interface once in case the compositor doesn't implement it. Perhaps using GOnce or just a different static flag variable. @@ +195,3 @@ + { + gtk_im_context_get_surrounding (GTK_IM_CONTEXT (self), + &surrounding, surrounding is being leaked @@ +244,3 @@ + priv->preedit_cursor = priv->pending_preedit_cursor; + priv->preedit_text = g_strdup (text); + priv->preedit_commit = g_strdup (commit); All of these might be leaked if the compositor sends us several preedit_string events in a row. I'd prefer these pango attr variables to use the ref counting API. @@ +331,3 @@ + +static xkb_mod_index_t +modifiers_get_index (struct wl_array *modifiers_map, All of this modifier handling can probably go away. See below @@ +402,3 @@ + +static void +text_input_keysym (void *data, I don't think we should support this. Even its presence in the protocol needs discussion. Do you see any use cases for it? It seems to me that the compositor can always send real key events through the wl_keyboard interface. E.g. if the IM engine that the compositor is talking to doesn't eat the key event, then the compositor should forward it to the client through wl_keyboard events, right? @@ +534,3 @@ + if (attrs != NULL) + { + *attrs = pango_attr_list_new (); This should certainly be using priv->preedit_attrs if they exist otherwise where are the IM provided attrs used? @@ +566,3 @@ + device = gdk_seat_get_pointer (seat); + + g_return_if_fail (GDK_IS_WAYLAND_DEVICE (device)); Same comments as below for _focus_out @@ +597,3 @@ + GdkDevice *device; + + g_return_if_fail (GDK_IS_WAYLAND_WINDOW (priv->window)); We don't neet these assertions since gtk+ won't load this IM module if the wayland gdk backend isn't being used. @@ +607,3 @@ + commit_and_reset_preedit (self); + + zwp_text_input_v1_deactivate (priv->text_input, Either we need a _hide_input_panel() here or we agree that the _activate() and _deactivate() requests are enough for the compositor to know when the input panel is required. I'd go with the latter, i.e. keep it simpler, for now until we clearly need the *_input_panel() requests @@ +608,3 @@ + + zwp_text_input_v1_deactivate (priv->text_input, + gdk_wayland_device_get_wl_seat (device)); Why do we need the pointer device? Isn't gdk_display_get_default_seat() enough here? @@ +654,3 @@ + GtkIMContextWaylandPrivate); + + self->priv->text_input = zwp_text_input_manager_v1_create_text_input(text_input_manager_get ()); For compositors that don't support the interface, we need to handle text_input_manager_get() returning NULL. And, in that case, we can make all the GtkIMContext vfuncs we implement return early.
Created attachment 319877 [details] [review] immodule: Filter backend specific modules in XSETTINGS and the env var This allows configurations, where different gtk+ applications might be running with different GDK backends, to specify a single global IM list without breaking due to the wrong module for the backend in use getting loaded.
Created attachment 319894 [details] [review] wayland: add input method module Add support for an input method module using the Wayland text protocol.
Created attachment 319897 [details] [review] wayland: add input method module Add support for an input method module using the Wayland text protocol. -- Fixed some mistakes in the previous attachement
(In reply to Rui Matos from comment #24) > Review of attachment 319534 [details] [review] [review]: > > Thanks for the patch! Still needs some work though, let me know if you > intend to continue working on it. Sure, I'm willing to. > ::: modules/input/gtkimcontextwayland.c > @@ +95,3 @@ > +text_input_manager_get (void) > +{ > + if (!text_input_manager) > > Since this is going to get hit for each IM context in the process we should > ensure we really only try to bind the interface once in case the compositor > doesn't implement it. Perhaps using GOnce or just a different static flag > variable. Fixed it using GOnce. > @@ +195,3 @@ > + { > + gtk_im_context_get_surrounding (GTK_IM_CONTEXT (self), > + &surrounding, > > surrounding is being leaked Fixed. > @@ +244,3 @@ > + priv->preedit_cursor = priv->pending_preedit_cursor; > + priv->preedit_text = g_strdup (text); > + priv->preedit_commit = g_strdup (commit); > > All of these might be leaked if the compositor sends us several > preedit_string events in a row. > > I'd prefer these pango attr variables to use the ref counting API. Added cleanup around them, using pango_attr_list_{ref,unref}. > @@ +331,3 @@ > + > +static xkb_mod_index_t > +modifiers_get_index (struct wl_array *modifiers_map, > > All of this modifier handling can probably go away. See below Removed all the modifier stuff. > @@ +402,3 @@ > + > +static void > +text_input_keysym (void *data, > > I don't think we should support this. Even its presence in the protocol > needs discussion. > > Do you see any use cases for it? It seems to me that the compositor can > always send real key events through the wl_keyboard interface. E.g. if the > IM engine that the compositor is talking to doesn't eat the key event, then > the compositor should forward it to the client through wl_keyboard events, > right? I have no idea, and if I change the mutter patch to use wl_keyboard.key instead, it seems to work. So I changed this function as no-op. > @@ +534,3 @@ > + if (attrs != NULL) > + { > + *attrs = pango_attr_list_new (); > > This should certainly be using priv->preedit_attrs if they exist otherwise > where are the IM provided attrs used? Sure, fixed to just return priv->preedit_attrs with the refcount incremented. > @@ +566,3 @@ > + device = gdk_seat_get_pointer (seat); > + > + g_return_if_fail (GDK_IS_WAYLAND_DEVICE (device)); > > Same comments as below for _focus_out > > @@ +597,3 @@ > + GdkDevice *device; > + > + g_return_if_fail (GDK_IS_WAYLAND_WINDOW (priv->window)); > > We don't neet these assertions since gtk+ won't load this IM module if the > wayland gdk backend isn't being used. Removed the assertions. > @@ +607,3 @@ > + commit_and_reset_preedit (self); > + > + zwp_text_input_v1_deactivate (priv->text_input, > > Either we need a _hide_input_panel() here or we agree that the _activate() > and _deactivate() requests are enough for the compositor to know when the > input panel is required. I'd go with the latter, i.e. keep it simpler, for > now until we clearly need the *_input_panel() requests I've leave as it is. > @@ +608,3 @@ > + > + zwp_text_input_v1_deactivate (priv->text_input, > + gdk_wayland_device_get_wl_seat (device)); > > Why do we need the pointer device? Isn't gdk_display_get_default_seat() > enough here? Sure, simplified also using gdk_seat_get_wl_seat(). > @@ +654,3 @@ > + GtkIMContextWaylandPrivate); > + > + self->priv->text_input = > zwp_text_input_manager_v1_create_text_input(text_input_manager_get ()); > > For compositors that don't support the interface, we need to handle > text_input_manager_get() returning NULL. And, in that case, we can make all > the GtkIMContext vfuncs we implement return early. Added g_return_if_fail() to the vfuncs which shall interact with compositor. Other than those changes, I've simplified module/input/Makefile.am and gdk/wayland/Makefile.am a bit so it create a new static library libgdk-wayland-protocol.la, which can be shared between libgdk-wayland.la and im-wayland.la.
Kind of unfortunate timing for this patch... but there seems to be a newer version of the text input protocol being discussed: http://lists.freedesktop.org/archives/wayland-devel/2016-January/026720.html
Yes sorry for pushing that now, but it think it will be much better to base Gtk+/Mutter support on the new version of the protocol and do not run into the same problems we have already seen in real world use (on embedded devices) of the first protocol version.
We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.
As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla. If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab: https://gitlab.gnome.org/GNOME/gtk/issues/new