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 696996 - keyboard: Introduce a SetInputSource DBus method
keyboard: Introduce a SetInputSource DBus method
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: keyboard
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
3.10
: 687935 (view as bug list)
Depends on:
Blocks: 697007
 
 
Reported: 2013-03-31 20:35 UTC by Rui Matos
Modified: 2013-05-24 21:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
keyboard: Cancel and bail out of async DBus operations on plugin stop (3.02 KB, patch)
2013-03-31 20:35 UTC, Rui Matos
committed Details | Review
keyboard: Claim a DBus well known name (5.82 KB, patch)
2013-03-31 20:35 UTC, Rui Matos
reviewed Details | Review
keyboard: Introduce a SetInputSource DBus method (7.75 KB, patch)
2013-03-31 20:35 UTC, Rui Matos
needs-work Details | Review
keyboard: Make sure the XKB group in use is always what we want (1.08 KB, patch)
2013-03-31 20:35 UTC, Rui Matos
committed Details | Review
keyboard: Claim a DBus well known name (6.03 KB, patch)
2013-04-02 15:08 UTC, Rui Matos
committed Details | Review
keyboard: Introduce a SetInputSource DBus method (6.62 KB, patch)
2013-04-02 15:16 UTC, Rui Matos
none Details | Review
keyboard: Introduce a SetInputSource DBus method (6.74 KB, patch)
2013-04-03 16:12 UTC, Rui Matos
needs-work Details | Review
keyboard: Introduce a SetInputSource DBus method (6.75 KB, patch)
2013-04-03 17:29 UTC, Rui Matos
committed Details | Review

Description Rui Matos 2013-03-31 20:35:33 UTC
This patch set is the g-s-d part required to make input source
switching more reliable (not lose key events to the wrong keymap) and
also makes it possible to do the modifiers-only keybinding in mutter
and thus work in the overview etc.

I hope we can land this for 3.8.1 even though it adds API[1] since
these are pretty nasty bugs for a large number of users disrupting
their routine typing.

[1] I actually consider this "private" API in that only other GNOME
core components should be making use of it. I wonder if we should make
that explicit somehow?
Comment 1 Rui Matos 2013-03-31 20:35:36 UTC
Created attachment 240255 [details] [review]
keyboard: Cancel and bail out of async DBus operations on plugin stop

Prevents potential uses after free.
Comment 2 Rui Matos 2013-03-31 20:35:39 UTC
Created attachment 240256 [details] [review]
keyboard: Claim a DBus well known name

For now we'll just claim the name and export an empty interface. We'll
grow the interface as needed.
Comment 3 Rui Matos 2013-03-31 20:35:44 UTC
Created attachment 240257 [details] [review]
keyboard: Introduce a SetInputSource DBus method

The only way for external components to activate an input source is by
setting the gsettings key. That's not optimal since external
components then have no way of knowing when exactly the switch is
completed.

This commit introduces a DBus method to set an input source and we
make sure to only return the method after all the changes have been
made, i.e. the XKB keyboard description has been changed and the IBus
engine (if any) has been activated.

Since setting the IBus engine happens asynchronously we queue requests
from this DBus API if there's still an ongoing previous request.
Comment 4 Rui Matos 2013-03-31 20:35:48 UTC
Created attachment 240258 [details] [review]
keyboard: Make sure the XKB group in use is always what we want

The layout we want is always in the first XKB group index so we should
enforce it to make sure we never end up with the wrong one.
Comment 5 Rui Matos 2013-04-02 08:39:50 UTC
*** Bug 687935 has been marked as a duplicate of this bug. ***
Comment 6 Bastien Nocera 2013-04-02 09:58:55 UTC
(In reply to comment #3)
> Created an attachment (id=240257) [details] [review]
> keyboard: Introduce a SetInputSource DBus method
> 
> The only way for external components to activate an input source is by
> setting the gsettings key. That's not optimal since external
> components then have no way of knowing when exactly the switch is
> completed.

How is that a problem? Which component needs to know that the input source has been switched?
Why does blocking on the XKB or IBus configurations avoid problems?

> This commit introduces a DBus method to set an input source and we
> make sure to only return the method after all the changes have been
> made, i.e. the XKB keyboard description has been changed and the IBus
> engine (if any) has been activated.
> 
> Since setting the IBus engine happens asynchronously we queue requests
> from this DBus API if there's still an ongoing previous request.

This reads like: "we do everything async except when we don't".
Comment 7 Rui Matos 2013-04-02 10:07:25 UTC
(In reply to comment #6)
> > The only way for external components to activate an input source is by
> > setting the gsettings key. That's not optimal since external
> > components then have no way of knowing when exactly the switch is
> > completed.
> 
> How is that a problem? Which component needs to know that the input source has
> been switched?
> Why does blocking on the XKB or IBus configurations avoid problems?

See bug 697007.

> > This commit introduces a DBus method to set an input source and we
> > make sure to only return the method after all the changes have been
> > made, i.e. the XKB keyboard description has been changed and the IBus
> > engine (if any) has been activated.
> > 
> > Since setting the IBus engine happens asynchronously we queue requests
> > from this DBus API if there's still an ongoing previous request.
> 
> This reads like: "we do everything async except when we don't".

What does that mean? Yes, we set the ibus engine async because it's possible but the XKB layout upload thing is sync because there's no other way AFAICT.
Comment 8 Bastien Nocera 2013-04-02 10:23:21 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > > The only way for external components to activate an input source is by
> > > setting the gsettings key. That's not optimal since external
> > > components then have no way of knowing when exactly the switch is
> > > completed.
> > 
> > How is that a problem? Which component needs to know that the input source has
> > been switched?
> > Why does blocking on the XKB or IBus configurations avoid problems?
> 
> See bug 697007.

Can you make sure that information is available in the g-s-d commit?

> > > This commit introduces a DBus method to set an input source and we
> > > make sure to only return the method after all the changes have been
> > > made, i.e. the XKB keyboard description has been changed and the IBus
> > > engine (if any) has been activated.
> > > 
> > > Since setting the IBus engine happens asynchronously we queue requests
> > > from this DBus API if there's still an ongoing previous request.
> > 
> > This reads like: "we do everything async except when we don't".
> 
> What does that mean? Yes, we set the ibus engine async because it's possible
> but the XKB layout upload thing is sync because there's no other way AFAICT.

It means that switching between IBus methods could still still broken though, it would be nice to mention more clearly that this is only fixed for XKB input sources.
Comment 9 Bastien Nocera 2013-04-02 10:24:03 UTC
Review of attachment 240255 [details] [review]:

Looks good.
Comment 10 Rui Matos 2013-04-02 10:26:57 UTC
(In reply to comment #8)
> > See bug 697007.
> 
> Can you make sure that information is available in the g-s-d commit?

Ok, I'll expand the commit message.

> It means that switching between IBus methods could still still broken though,
> it would be nice to mention more clearly that this is only fixed for XKB input
> sources.

Oh, but ibus engines are synchronized here as well since I'm not returning the dbus call until I hear back (asynchronously) from ibus that it completed the engine switch.
Comment 11 Bastien Nocera 2013-04-02 10:27:58 UTC
Review of attachment 240256 [details] [review]:

Looks good pending the following patches.

::: plugins/keyboard/gsd-keyboard-manager.c
@@ +1564,3 @@
+        priv->dbus_connection = connection;
+
+        priv->dbus_register_object_id =

On the same line please, I have a wide screen.

@@ +1578,3 @@
+        }
+
+        priv->dbus_own_name_id =

Ditto.

@@ +1681,3 @@
 
+        if (p->dbus_own_name_id) {
+                g_bus_unown_name (p->dbus_own_name_id);

I believe you could use g_clear_pointer() here.

@@ +1686,3 @@
+
+        if (p->dbus_register_object_id) {
+                g_dbus_connection_unregister_object (p->dbus_connection,

And here.
Comment 12 Bastien Nocera 2013-04-02 10:35:49 UTC
Review of attachment 240257 [details] [review]:

::: plugins/keyboard/gsd-keyboard-manager.c
@@ +124,3 @@
         guint dbus_register_object_id;
+
+        GQueue *set_input_source_invocation_queue;

Is it useful having a queue? You seem to be queuing and processing the requests serially, when you could just throw away the old request when one is still being processed, and replace it with the new one.

Eg. when receiving new items:
if (!->current_invocation) {
  ->current_invocation = invocation;
  /* do things */
} else {
  if (->next_invocation)
    return_as_cancelled (->next_invocation);
  ->next_invocation = invocation;
}

Might be micro-optimising, but I really want to avoid hanging the keyboard for more time than necessary.
Comment 13 Bastien Nocera 2013-04-02 10:37:18 UTC
Review of attachment 240258 [details] [review]:

Please add a comment above that line. Looks good.
Comment 14 Rui Matos 2013-04-02 15:08:40 UTC
Created attachment 240397 [details] [review]
keyboard: Claim a DBus well known name

--

(In reply to comment #11)
> ::: plugins/keyboard/gsd-keyboard-manager.c
> +        if (p->dbus_own_name_id) {
> +                g_bus_unown_name (p->dbus_own_name_id);
>
> I believe you could use g_clear_pointer() here.
>
> @@ +1686,3 @@
> +
> +        if (p->dbus_register_object_id) {
> +                g_dbus_connection_unregister_object (p->dbus_connection,
>
> And here.

Nope, these are not pointers and the second one actually needs the
GDBusConnection object to be passed in to unregister.

Re-indented the other lines.
Comment 15 Rui Matos 2013-04-02 15:16:41 UTC
Created attachment 240398 [details] [review]
keyboard: Introduce a SetInputSource DBus method

--
E.g. gnome-shell can use this method and freeze keyboard events in the
X server until it hears back from g-s-d to ensure that events won't be
misinterpreted after an input source switch.
--

Added the comment above to the commit message.

(In reply to comment #12)
> Is it useful having a queue? You seem to be queuing and processing the requests
> serially, when you could just throw away the old request when one is still
> being processed, and replace it with the new one.

Yeah, the queue is not needed. We can just use the cancellable used in
the ibus async call and cancel that if another invocation arrives. How
does it look now?

> Might be micro-optimising, but I really want to avoid hanging the keyboard for
> more time than necessary.

Well, hanging the keyboard is up to gnome-shell. I'm currently using
the the default dbus timeout there to call this method by I could put
an hard limit there and unthaw the events after, say 2 seconds, if
g-s-d doesn't reply under that.

Anyway, in my testing here, mashing the switch input source keyboard
shortcut, it seems to handle things just fine even having to activate
the anthy engine which is a python process.
Comment 16 Rui Matos 2013-04-03 16:12:17 UTC
Created attachment 240498 [details] [review]
keyboard: Introduce a SetInputSource DBus method

--

(In reply to comment #15)
> Anyway, in my testing here, mashing the switch input source keyboard
> shortcut, it seems to handle things just fine even having to activate
> the anthy engine which is a python process.

After increasing my key repeat rate I could actually trigger that
g_warn_if_fail() so I just changed it to not rely on
g_cancellable_cancel() running set_ibus_engine_finish() to clear the
previous invocation and do it explicitly instead.
Comment 17 Bastien Nocera 2013-04-03 16:57:57 UTC
Review of attachment 240498 [details] [review]:

::: plugins/keyboard/gsd-keyboard-manager.c
@@ +1559,3 @@
+                return;
+
+        if (priv->pending_ops > 0) {

g_atomic_int_dec_and_test()?

@@ +1575,3 @@
+                return;
+
+        priv->pending_ops += 1;

g_atomic_int_inc()

@@ +1584,3 @@
+        guint idx;
+
+        priv->pending_ops = 0;

g_atomic_int_set();

@@ +1613,3 @@
+                         * ibus_bus_set_global_engine_async() call
+                         * going on. */
+                        g_cancellable_cancel (priv->ibus_cancellable);

Don't you need to replace it after cancelling it?

@@ +1614,3 @@
+                         * going on. */
+                        g_cancellable_cancel (priv->ibus_cancellable);
+                        g_clear_pointer (&priv->invocation, set_input_source_return);

I would rather you set the pending_ops here, as that's where you reduce the number of pending ops to 0.
Comment 18 Rui Matos 2013-04-03 17:29:54 UTC
Created attachment 240517 [details] [review]
keyboard: Introduce a SetInputSource DBus method

--

(In reply to comment #17)
> ::: plugins/keyboard/gsd-keyboard-manager.c
> @@ +1559,3 @@
> +                return;
> +
> +        if (priv->pending_ops > 0) {
>
> g_atomic_int_dec_and_test()?

I don't see the usefulness of that since this code is never called
from multiple threads.

> @@ +1613,3 @@
> +                         * ibus_bus_set_global_engine_async() call
> +                         * going on. */
> +                        g_cancellable_cancel (priv->ibus_cancellable);
>
> Don't you need to replace it after cancelling it?

That happens in set_ibus_engine() right before it calls out to ibus
asynchronously.

> @@ +1614,3 @@
> +                         * going on. */
> +                        g_cancellable_cancel (priv->ibus_cancellable);
> +                        g_clear_pointer (&priv->invocation,
> set_input_source_return);
>
> I would rather you set the pending_ops here, as that's where you reduce the
> number of pending ops to 0.

OK, makes sense.
Comment 19 Rui Matos 2013-04-11 08:45:10 UTC
Pushing a-c-n patches.

Attachment 240255 [details] pushed as 189160b - keyboard: Cancel and bail out of async DBus operations on plugin stop
Attachment 240258 [details] pushed as 718ab59 - keyboard: Make sure the XKB group in use is always what we want
Comment 20 Bastien Nocera 2013-05-17 08:38:18 UTC
Review of attachment 240397 [details] [review]:

Looks good.
Comment 21 Bastien Nocera 2013-05-17 08:40:00 UTC
Review of attachment 240517 [details] [review]:

Looks good.

::: plugins/keyboard/gsd-keyboard-manager.c
@@ +146,3 @@
         "  <interface name='org.gnome.SettingsDaemon.Keyboard'>"
+        "    <method name='SetInputSource'>"
+        "      <arg type='u' name='idx' direction='in'/>"

index? Or even "input_source_index". No reason to keep the name this short.
Comment 22 Rui Matos 2013-05-24 21:28:15 UTC
Attachment 240397 [details] pushed as 7ef54e0 - keyboard: Claim a DBus well known name
Attachment 240517 [details] pushed as 6b01ab8 - keyboard: Introduce a SetInputSource DBus method