GNOME Bugzilla – Bug 696996
keyboard: Introduce a SetInputSource DBus method
Last modified: 2013-05-24 21:28:25 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?
Created attachment 240255 [details] [review] keyboard: Cancel and bail out of async DBus operations on plugin stop Prevents potential uses after free.
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.
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.
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.
*** Bug 687935 has been marked as a duplicate of this bug. ***
(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".
(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.
(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.
Review of attachment 240255 [details] [review]: Looks good.
(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.
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.
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.
Review of attachment 240258 [details] [review]: Please add a comment above that line. Looks good.
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.
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.
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.
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.
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.
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
Review of attachment 240397 [details] [review]: Looks good.
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.
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