After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 698307 - There should be support for the Wayland input method protocol
There should be support for the Wayland input method protocol
Status: RESOLVED OBSOLETE
Product: gtk+
Classification: Platform
Component: Input Methods
3.7.x
Other Linux
: Normal normal
: ---
Assigned To: Jan Arne Petersen
gtk-bugs
wayland
Depends on:
Blocks: wayland
 
 
Reported: 2013-04-18 14:52 UTC by Jan Arne Petersen
Modified: 2018-04-15 00:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
wayland: add input method module (49.72 KB, patch)
2013-04-18 14:53 UTC, Jan Arne Petersen
reviewed Details | Review
deal with platform-specific im modules (2.16 KB, patch)
2013-08-11 18:14 UTC, Matthias Clasen
committed Details | Review
wayland: add input method module (50.99 KB, patch)
2013-08-11 18:15 UTC, Matthias Clasen
none Details | Review
wayland: add input method module (29.93 KB, patch)
2016-01-22 03:58 UTC, Daiki Ueno
none Details | Review
immodule: Filter backend specific modules in XSETTINGS and the env var (2.23 KB, patch)
2016-01-27 19:52 UTC, Rui Matos
none Details | Review
wayland: add input method module (27.10 KB, patch)
2016-01-28 07:49 UTC, Daiki Ueno
none Details | Review
wayland: add input method module (27.07 KB, patch)
2016-01-28 08:09 UTC, Daiki Ueno
none Details | Review

Description Jan Arne Petersen 2013-04-18 14:52:41 UTC
There is an input method protocol in Wayland which should be supported by GTK+
Comment 1 Jan Arne Petersen 2013-04-18 14:53:33 UTC
Created attachment 241831 [details] [review]
wayland: add input method module
Comment 2 Jan Arne Petersen 2013-04-18 15:00:48 UTC
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
Comment 3 Matthias Clasen 2013-08-07 11:40:38 UTC
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.
Comment 4 Matthias Clasen 2013-08-11 18:14:23 UTC
Created attachment 251303 [details] [review]
deal with platform-specific im modules
Comment 5 Matthias Clasen 2013-08-11 18:15:52 UTC
Created attachment 251304 [details] [review]
wayland: add input method module
Comment 6 Matthias Clasen 2013-08-11 18:20:52 UTC
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.
Comment 7 Matthias Clasen 2013-08-11 19:46:26 UTC
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.
Comment 8 Rob Bradford 2013-08-14 14:32:07 UTC
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)
Comment 9 Matthias Clasen 2013-08-14 22:21:56 UTC
(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 ?
Comment 10 Jan Arne Petersen 2013-08-17 15:26:14 UTC
(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.
Comment 11 Matthias Clasen 2013-08-18 14:53:53 UTC
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.
Comment 12 Jan Arne Petersen 2013-08-20 12:33:09 UTC
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.
Comment 13 Matthias Clasen 2013-09-03 13:02:17 UTC
I don't think this is going to make 3.10, at this stage
Comment 14 Jasper St. Pierre (not reading bugmail) 2014-04-28 17:35:32 UTC
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.
Comment 15 Daiki Ueno 2015-05-21 01:29:35 UTC
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.
Comment 16 Jasper St. Pierre (not reading bugmail) 2016-01-07 03:41:40 UTC
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.
Comment 17 Daiki Ueno 2016-01-07 04:08:40 UTC
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.
Comment 18 Matthias Clasen 2016-01-07 14:10:59 UTC
(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.
Comment 19 Jan Arne Petersen 2016-01-07 15:57:52 UTC
(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).
Comment 20 Jasper St. Pierre (not reading bugmail) 2016-01-07 16:06:50 UTC
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.
Comment 21 Jan Arne Petersen 2016-01-07 17:12:48 UTC
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+).
Comment 22 Daiki Ueno 2016-01-22 03:58:14 UTC
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().
Comment 23 Matthias Clasen 2016-01-24 18:10:48 UTC
Rui, can you review this, please ?
Comment 24 Rui Matos 2016-01-27 19:50:57 UTC
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.
Comment 25 Rui Matos 2016-01-27 19:52:03 UTC
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.
Comment 26 Daiki Ueno 2016-01-28 07:49:41 UTC
Created attachment 319894 [details] [review]
wayland: add input method module

Add support for an input method module using the Wayland
text protocol.
Comment 27 Daiki Ueno 2016-01-28 08:09:04 UTC
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
Comment 28 Daiki Ueno 2016-01-28 08:20:24 UTC
(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.
Comment 29 Carlos Garnacho 2016-01-28 11:34:53 UTC
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
Comment 30 Jan Arne Petersen 2016-01-28 15:49:52 UTC
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.
Comment 31 Matthias Clasen 2018-02-10 05:26:14 UTC
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.
Comment 32 Matthias Clasen 2018-04-15 00:36:44 UTC
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