GNOME Bugzilla – Bug 676102
XKB keyboard layouts and IBus integration
Last modified: 2012-11-08 19:27:17 UTC
These patches go on top of the clean-ups in bug 674948.
Created attachment 214105 [details] [review] keyboard: Apply XKB layouts ourselves and stop relying on libgnomekbd libgnomekbd/xklavier aren't a good fit to have the keyboard input story that we want since they rely on implementation details of the XKB protocol to provide users with a means to switch keyboard layouts. Of note here is a) their reliance on XKB groups, of which there can be only up to 4, to specify the layouts the user is able to switch between and b) their reliance on XKB options to specify the keybinding to switch layouts which is a restricted set and falls outside the regular GNOME desktop wide keybindings management as it's implemented entirely on the X server side. This commit introduces the use of a shared GSettings schema from gsettings-desktop-schemas which will be the storage for our new concept of "input sources". Input sources are basically a tuple of keyboard layout and, if needed, an IBus input engine that work together to provide the user with working keyboard input. As a start we only handle the keyboard layout for now. We do it using roughly the same method that setxkbmap(1) uses which should allow the users that want to specify their own XKB features (except layout) to still do so outside of GNOME (e.g. in a session startup script).
Created attachment 214106 [details] [review] keyboard: Add the IBus engine setting code We simply connect to IBus and tell it to switch engine when our current input source setting is changed and an IBus engine is specified there. The responsibility to make sure that this engine actually exists on the IBus side is left to whoever writes the setting. At the same time, if an IBus engine is specified, we flip the setting that backs the Gtk/IMModule XSETTING so that GTK+ applications get notified to load the IBus input method when needed and go back to the simple input method when IBus isn't required.
Created attachment 214107 [details] [review] keyboard: Always add a latin and a UI language XKB layouts Toolkits need to know about both a latin layout to handle accelerators which are usually defined like Ctrl+C and a layout with the symbols for the language used in UI strings to handle mnemonics like Alt+Ф, so we try to find and add them in XKB group slots after the layout which the user actually intends to type with.
Review of attachment 214105 [details] [review]: ::: plugins/keyboard/Makefile.am @@ +23,3 @@ $(NULL) +XKBCONFIGROOT=@XKBCONFIGROOT@ Why do we need that? ::: plugins/keyboard/gsd-keyboard-manager.c @@ +66,3 @@ +#define GNOME_DESKTOP_INPUT_SOURCES_DIR "org.gnome.desktop.input-sources" + +#define KEY_CURRENT_IS "current" KEY_CURRENT_INPUT_SOURCE isn't that long. @@ +69,3 @@ +#define KEY_INPUT_SOURCES "sources" + +#ifndef DFLT_XKB_CONFIG_ROOT Why do we need all that? Can't we require a new enough version of xkb to have those defined? @@ +87,3 @@ GSettings *settings; + GSettings *is_settings; + gulong ignore_serial; The "ignore_serial" stuff all across this file look like magic to me, and not the good kind of magic. Please document it. @@ +217,3 @@ + if (!XkbRF_GetNamesProp (display, rules, var_defs) || !*rules) { + *rules = strdup (DFLT_XKB_RULES_FILE); + var_defs->model = strdup (DFLT_XKB_MODEL); can't you use g_strdup() and thus get rid of all the "if (foo) free (foo)" code? @@ +279,3 @@ + + if (!XkbRF_SetNamesProp (display, rules_file, var_defs)) + g_warning ("Couldn't update the XKB root window property"); Are you sure this doesn't generate X errors? Do we need push_error() pop_error() around here? @@ +299,3 @@ + rules_path = g_strdup (rules_file); + else + rules_path = g_strdup_printf ("%s/rules/%s", That's not how you create file paths, see g_build_filename() for example.
Review of attachment 214106 [details] [review]: ::: plugins/keyboard/gsd-keyboard-manager.c @@ +352,3 @@ +{ + IBusEngineDesc *desc = NULL; + IBusBus *ibus = ibus_bus_new (); Don't assign in the declaration, especially instantiating new objects. @@ +355,3 @@ + + if (!ibus_bus_set_global_engine (ibus, engine) || + !(desc = ibus_bus_get_global_engine (ibus))) Split this up so the error paths aren't as messy as they are now. @@ +356,3 @@ + if (!ibus_bus_set_global_engine (ibus, engine) || + !(desc = ibus_bus_get_global_engine (ibus))) + g_warning ("Couldn't set IBus engine"); Better error reporting? @@ +362,3 @@ + GTK_IM_MODULE_IBUS); + + g_object_unref (ibus); Do we need to keep the ibus object around? @@ +519,3 @@ +#ifdef HAVE_IBUS + ibus_init (); Error reporting?
Review of attachment 214107 [details] [review]: The code addedin gsd-keyboard-manager.c should be split out, a test application written, and automatically launched. See this for example: http://git.gnome.org/browse/gnome-control-center/tree/panels/info/test-hostname.c Need a test application for the xkb-rules-db code as well, and it needs to be valgrinded clean. Where's the code to clean up the database's allocated memory? Could you use a database object instead, which would make checking the lifetime of the various allocated bits of memory much simpler? ::: plugins/keyboard/xkb-rules-db.c @@ +34,3 @@ + +#ifndef DFLT_XKB_CONFIG_ROOT +#define DFLT_XKB_CONFIG_ROOT "/usr/share/X11/xkb" Isn't this the same defines I was saying we shouldn't need? We probably really don't want them in 2 files...
Created attachment 214685 [details] [review] keyboard: Apply XKB layouts ourselves and stop relying on libgnomekbd -- (In reply to comment #4) > +XKBCONFIGROOT=@XKBCONFIGROOT@ > > Why do we need that? [ this moved to a gnome-desktop patch ] Removed. > KEY_CURRENT_INPUT_SOURCE isn't that long. Agree. > +#ifndef DFLT_XKB_CONFIG_ROOT > > Why do we need all that? Can't we require a new enough version of xkb to have > those defined? [ this moved to a gnome-desktop patch ] Yup, it seems that xkeyboard-config.pc defines this one. > The "ignore_serial" stuff all across this file look like magic to me, and not > the good kind of magic. Please document it. This was a just a leftover from previous experiments. Removed. > + if (!XkbRF_GetNamesProp (display, rules, var_defs) || !*rules) { > + *rules = strdup (DFLT_XKB_RULES_FILE); > + var_defs->model = strdup (DFLT_XKB_MODEL); > > can't you use g_strdup() and thus get rid of all the "if (foo) free (foo)" > code? [ this moved to a gnome-desktop patch ] Seem like we can yes. Even though the XkbRF* functions allocate it internally with malloc and friends. > + if (!XkbRF_SetNamesProp (display, rules_file, var_defs)) > + g_warning ("Couldn't update the XKB root window property"); > > Are you sure this doesn't generate X errors? Do we need push_error() > pop_error() around here? [ this moved to a gnome-desktop patch ] Added. > + rules_path = g_strdup_printf ("%s/rules/%s", > > That's not how you create file paths, see g_build_filename() for example. [ this moved to a gnome-desktop patch ] Done. Thanks for the review.
Created attachment 214686 [details] [review] keyboard: Add the capability to set IBus engines from input sources -- I re-worked things a bit. Now I'm keeping the IBus connection always opened. I'm still missing code to re-connect if IBus crashes or otherwise goes away but I think that can go in another patch and this can be review as is.
(In reply to comment #7) > [ this moved to a gnome-desktop patch ] Where's the bug? Can you please add the blockers as appropriate. > > + if (!XkbRF_GetNamesProp (display, rules, var_defs) || !*rules) { > > + *rules = strdup (DFLT_XKB_RULES_FILE); > > + var_defs->model = strdup (DFLT_XKB_MODEL); > > > > can't you use g_strdup() and thus get rid of all the "if (foo) free (foo)" > > code? > > [ this moved to a gnome-desktop patch ] > > Seem like we can yes. Even though the XkbRF* functions allocate it > internally with malloc and friends. If the XkbRF* functions do allocation, then you need to use the corresponding free function. So, in fact, you can't stop using all that "if (foo) free (foo)".
free(NULL); is perfectly valid C (it's a no-op).
Created attachment 214897 [details] [review] keyboard: Apply XKB layouts ourselves and stop relying on libgnomekbd
Created attachment 214898 [details] [review] keyboard: Add the capability to set IBus engines from input sources
Created attachment 215174 [details] [review] media-keys: Add key bindings to switch input sources
Review of attachment 214898 [details] [review]: ::: plugins/keyboard/gsd-keyboard-manager.c @@ +671,3 @@ + g_object_unref (p->interface_settings); + p->interface_settings = NULL; + } Could just g_clear_object() here @@ +676,3 @@ + g_object_unref (p->ibus); + p->ibus = NULL; + } And here
Review of attachment 214897 [details] [review]: ::: plugins/keyboard/gsd-keyboard-manager.c @@ +201,3 @@ + g_return_if_fail (p != NULL); + + if (p->keymap) What halfline said actually, the ifs aren't needed. @@ +227,3 @@ + + /* Upload it to the X server using the same method as setxkbmap */ + xkb_desc = XkbGetKeyboardByName (display, I think this need gdk_error_trap_push()/gdk_error_trap_pop() to catch BadMatch. @@ +293,3 @@ + if ((g_strcmp0 (latin_layout, locale_layout) == 0 && + g_strcmp0 (latin_variant, locale_variant) == 0) + || This section is unreadable. @@ +309,3 @@ + free (xkb_var_defs->layout); + + xkb_var_defs->layout = This is unreadable. @@ +321,3 @@ + free (xkb_var_defs->variant); + + xkb_var_defs->variant = This is unreadable. @@ +360,3 @@ + } + + gdk_error_trap_pop_ignored (); Why do you ignore the error here, but print out a warning if you get an error from the function? Both are returns from the function, just one is async. @@ +387,3 @@ + } + if (current >= g_variant_n_children (sources)) { + g_warning ("Current input source index out of bounds, resetting"); There's no need for a warning. I'm also not sure that you want to reset the index when removing the last item from the list for example. @@ +583,3 @@ + + if (p->input_sources_settings != NULL) { + g_object_unref (p->input_sources_settings); g_clear_object() @@ +588,3 @@ + + if (p->xkb_info != NULL) { + g_object_unref (p->xkb_info); g_clear_object()
Review of attachment 215174 [details] [review]: ::: data/org.gnome.settings-daemon.plugins.media-keys.gschema.xml.in.in @@ +177,3 @@ </key> + <key name="switch-input-source" type="s"> + <default>''</default> What's the default keybinding to do that currently? ::: plugins/media-keys/gsd-media-keys-manager.c @@ +1790,3 @@ + settings = g_settings_new (GNOME_DESKTOP_INPUT_SOURCES_DIR); + sources = g_settings_get_value (settings, KEY_INPUT_SOURCES); + n = g_variant_n_children (sources); int new_index; new_index = n + (type == SWITCH_INPUT_SOURCE_KEY ? +1 : -1); if (new_index < 0) new_index = n; else if (new_index > n) new_index = 0; @@ +1791,3 @@ + sources = g_settings_get_value (settings, KEY_INPUT_SOURCES); + n = g_variant_n_children (sources); + if (n > 0) { if (n == 0) goto out; (and do this before any of the calculations)
Created attachment 215330 [details] [review] keyboard: Apply XKB layouts ourselves and stop relying on libgnomekbd -- (In reply to comment #15) > + if (p->keymap) > > What halfline said actually, the ifs aren't needed. Sure. > @@ +227,3 @@ > + > + /* Upload it to the X server using the same method as setxkbmap */ > + xkb_desc = XkbGetKeyboardByName (display, > > I think this need gdk_error_trap_push()/gdk_error_trap_pop() to catch BadMatch. There's already a trap_push/pop on the outer function. > @@ +293,3 @@ > + if ((g_strcmp0 (latin_layout, locale_layout) == 0 && > + g_strcmp0 (latin_variant, locale_variant) == 0) > + || > > This section is unreadable. Yeah, I hope it's clearer now. > @@ +360,3 @@ > + } > + > + gdk_error_trap_pop_ignored (); > > Why do you ignore the error here, but print out a warning if you get an error > from the function? Both are returns from the function, just one is async. Fixed. > @@ +387,3 @@ > + } > + if (current >= g_variant_n_children (sources)) { > + g_warning ("Current input source index out of bounds, > resetting"); > > There's no need for a warning. Agree. > I'm also not sure that you want to reset the index when removing the last item > from the list for example. Changed it to cap the value to N - 1. > g_clear_object() Ok.
Created attachment 215331 [details] [review] keyboard: Add the capability to set IBus engines from input sources
Created attachment 215332 [details] [review] media-keys: Add key bindings to switch input sources -- (In reply to comment #16) > What's the default keybinding to do that currently? I don't think there's a default currently. I guess people add one in "Region and Language" > Layouts > Options... > "Key(s) to change layout". Should I add one? BTW, with the current key binding infrastructure we have in g-s-d we can't use the kinds of key bindings that were available in those XKB options like Shift+CapsLock or Alt+Shift. I looked a bit into how we could do that and it seems that it's possible to do it with something like the gsd-locate-pointer code that checks when Ctrl is released and there were no other key presses in between the press and release. > ::: plugins/media-keys/gsd-media-keys-manager.c > @@ +1790,3 @@ > + settings = g_settings_new (GNOME_DESKTOP_INPUT_SOURCES_DIR); > + sources = g_settings_get_value (settings, KEY_INPUT_SOURCES); > + n = g_variant_n_children (sources); > > int new_index; > > new_index = n + (type == SWITCH_INPUT_SOURCE_KEY ? +1 : -1); > if (new_index < 0) > new_index = n; > else if (new_index > n) > new_index = 0; > > @@ +1791,3 @@ > + sources = g_settings_get_value (settings, KEY_INPUT_SOURCES); > + n = g_variant_n_children (sources); > + if (n > 0) { > > if (n == 0) > goto out; > (and do this before any of the calculations) OK, I think it's clearer now.
Created attachment 215369 [details] [review] keyboard: Apply XKB layouts ourselves and stop relying on libgnomekbd -- Updated for schema changes.
Created attachment 215370 [details] [review] media-keys: Add key bindings to switch input sources -- Updated for schema changes.
Created attachment 215371 [details] [review] keyboard: Add the capability to set IBus engines from input sources -- Updated for schema changes.
Can you please make sure that you get a replacement status icon made? Also, did you run test-keyboard stand-alone under valgrind to make sure that there aren't any leaks?
Currently switch-input-source and switch-input-source-backward in org.gnome.settings-daemon.plugins.media-keys.gschema.xml has the string type but not array type. I think multiple shortcut keys are needed to switch IMEs. E.g. Korean users use Alt_R and Hangul keys to switch Korean keyboard layout and ibus-hangul. Probably I think the default shortcut key is Control+space and it could be customized by users to add other shortcut keys. apply_input_sources_settings() always calls apply_xkb_layout() but I think some input method engines don't change the XKB but use the current XKB. So if ibus_engine_desc_get_layout() returns 'default', I'd like to cancel apply_xkb_layout().
(In reply to comment #24) > Currently switch-input-source and switch-input-source-backward in > org.gnome.settings-daemon.plugins.media-keys.gschema.xml has the string type > but not array type. > I think multiple shortcut keys are needed to switch IMEs. > E.g. Korean users use Alt_R and Hangul keys to switch Korean keyboard layout > and ibus-hangul. That's bug 677427. > Probably I think the default shortcut key is Control+space and it could be > customized by users to add other shortcut keys. I'm a bit wary of assigning that specific shortcut by default for all the clashes that it will probably cause with applications. We could assign it only when more than 1 input source is configured and there's nothing configured yet? Some design input here would be nice. FWIW, Windows 8 uses Windows+Space to switch input sources. > apply_input_sources_settings() always calls apply_xkb_layout() but I think some > input method engines don't change the XKB but use the current XKB. So if > ibus_engine_desc_get_layout() returns 'default', I'd like to cancel > apply_xkb_layout(). Like I told you on IRC, I don't think this is a good idea. Even if technically IBUS engines can cope with an arbitrary XKB layout, I think it's very confusing for users that some keys would input different symbols depending on the previously selected input source. It's just too obscure. I think IBUS engines must specify a XKB layout or otherwise we should default to the US layout.
(In reply to comment #25) > > Probably I think the default shortcut key is Control+space and it could be > > customized by users to add other shortcut keys. > > I'm a bit wary of assigning that specific shortcut by default for all the > clashes that it will probably cause with applications. We could assign it only > when more than 1 input source is configured and there's nothing configured yet? If one XKB layout only is configured, I think the shortcut key is not needed. If one XKB layout and one input method are configured, I think the default shortcut key is needed and Asian IM users use Control+space for ten years. If there is no input methods but some XKB layouts, different shortcut keys might be better. > Some design input here would be nice. FWIW, Windows 8 uses Windows+Space to > switch input sources. I know it however my concern is Windows key does not exist in non-Win keyboards. > > apply_input_sources_settings() always calls apply_xkb_layout() but I think some > > input method engines don't change the XKB but use the current XKB. So if > > ibus_engine_desc_get_layout() returns 'default', I'd like to cancel > > apply_xkb_layout(). > > Like I told you on IRC, I don't think this is a good idea. Even if technically > IBUS engines can cope with an arbitrary XKB layout, I think it's very confusing > for users that some keys would input different symbols depending on the > previously selected input source. It's just too obscure. > > I think IBUS engines must specify a XKB layout or otherwise we should default > to the US layout. If the 'default' is not implemented, I think it's very confusing users and probably I need to enable gtk ibus panel and ibus-gjs against gnome-settings-daemon. I think input method should not have the keyboard layouts. As I noted, ibus-m17n engines need to have 'us' layout because it works likes keyboard layouts rather than input methods. But pure input method could work for any keyboard layouts and input methods convert the single byte chars (e.g. 'a') to the multi byte chars (e.g. 'ａ') according to the current keyboard layout.
I have another problem in the current patches. Fedora 17 ibus uses ibus 1.5 and now it provides the new Control+space key which can switch ibus engines by pressing space key till Control key is released. The new Control+space key works likes metacity Alt+tab and I think this would need a GUI of the switcher dialog to select the engine. The previous ibus provides toggle shortcut keys and prev/next shortcut keys. ibus 1.5 migrated the toggle shortcut keys and prev/next shortcut keys to the new Control+space. Currently switch-input-source and switch-input-source-backward are implemented in org.gnome.settings-daemon.plugins.media-keys.gschema.xml. The switch-input-source/switch-input-source-backward are almost same with ibus prev/next shorcut keys. If the switcher dialog is not implemented, I think the toggle shortcut keys are required to switch a XKB layout and an input method. E.g. in case of the ibus engine list is, us, anthy, jp: toggle shortcut key swaps the first engine and the second engine, us <-> anthy. next shortcut key switches us -> anthy -> jp -> us... prev shortcut key switches us -> jp -> anthy -> us... The new Control+space in ibus 1.5 swaps the engine when Control key is released. If Control key is released when anthy is selected, the order is anthy, us, jp. If Control key is released when jp is selected, the order is jp, us, anthy. This satisfies both toggle shortcut keys and prev/next shortcut keys in the previous ibus.
Review of attachment 215371 [details] [review]: ::: plugins/keyboard/gsd-keyboard-manager.c @@ +454,3 @@ + g_settings_set_string (priv->interface_settings, + KEY_GTK_IM_MODULE, + GTK_IM_MODULE_SIMPLE); I wonder if this is being shortcut anywhere for the common case where all input sources are xkb - we don't want to constantly reload immodules in all running applications... @@ +634,3 @@ +#ifdef HAVE_IBUS + manager->priv->interface_settings = g_settings_new (GNOME_DESKTOP_INTERFACE_DIR); + ibus_init (); Can ibus_init() be called more than once ? @@ +639,3 @@ + g_warning ("Couldn't connect to ibus-daemon"); + g_object_unref (manager->priv->ibus); + manager->priv->ibus = NULL; Could use g_clear_object here @@ +641,3 @@ + manager->priv->ibus = NULL; + } + if (manager->priv->ibus) Could just be an 'else' here ?
Created attachment 216931 [details] [review] keyboard: Add the capability to set IBus engines from input sources -- Here's a new version which tries to handle the ibus-daemon lifecycle. If you agree with this approach I'll do a request on ibus upstream to never start ibus on a gnome session from their initialization scripts. (In reply to comment #28) > ::: plugins/keyboard/gsd-keyboard-manager.c > @@ +454,3 @@ > + g_settings_set_string (priv->interface_settings, > + KEY_GTK_IM_MODULE, > + GTK_IM_MODULE_SIMPLE); > > I wonder if this is being shortcut anywhere for the common case where all input > sources are xkb - we don't want to constantly reload immodules in all running > applications... Oh yes, good catch. I'm handling this now. > @@ +634,3 @@ > +#ifdef HAVE_IBUS > + manager->priv->interface_settings = g_settings_new > (GNOME_DESKTOP_INTERFACE_DIR); > + ibus_init (); > > Can ibus_init() be called more than once ? I tested and it works. It just calls g_type_init() and a handful of *_get_type() for its types. Anyway g-s-d plugins are only initialized once, right?
I think it's good to run a .desktop file or another configuration file instead of running ibus-daemon direclty. The arguments of ibus-daemon will be provided by ibus package. Currently imsettings loads /etc/X11/xinit/xinputrc . Running ibus-daemon is not handled by ibus but imsettings so I think you need to ask imsettings instead.
(In reply to comment #29) > > @@ +634,3 @@ > > +#ifdef HAVE_IBUS > > + manager->priv->interface_settings = g_settings_new > > (GNOME_DESKTOP_INTERFACE_DIR); > > + ibus_init (); > > > > Can ibus_init() be called more than once ? > > I tested and it works. It just calls g_type_init() and a handful of > *_get_type() for its types. Anyway g-s-d plugins are only initialized > once, right? I think you can toggle the 'active' setting to have them stopped or started at will. I was just commenting on the general principle that stop should undo what start has done. If the init function is idempotent, there's no problem.
Review of attachment 216931 [details] [review]: ::: plugins/keyboard/gsd-keyboard-manager.c @@ +155,3 @@ + + g_clear_object (&priv->ibus); + g_clear_object (&priv->ibus_cancellable); Do you want to cancel the cancellable here, if it is non-NULL ? Not sure it makes a big difference, just wondering. @@ +256,3 @@ + g_warning ("Couldn't activate and connect to the IBus daemon"); + + set_ibus_state_off (manager); /* give up */ Do you forget to clear priv->ibus_connection_timeout_id here ? @@ +258,3 @@ + set_ibus_state_off (manager); /* give up */ + + return FALSE; I'd prefer G_SOURCE_REMOVE here. @@ +290,3 @@ + char *args[] = { + "ibus-daemon", + "--daemonize", Why daemonize ? I think daemonizing in the user session is basically always a mistake. @@ +324,3 @@ + g_clear_object (&priv->ibus); + + spawn_ibus (manager); This looks to me like we have a forced 2-second timeout before we even try spawning ibus ? Thats not great... Shouldn't we optimize for the case where ibus is not running yet, and just spawn it ? If it was already starting up anyway, at worst, we'll have an extra process that dies when it can't get its bus name. @@ +326,3 @@ + spawn_ibus (manager); + + return FALSE; I really prefer to return G_SOURCE_REMOVE nowadays, it is just much more obvious. Not sure what the policy in gnome-control-center is, though.
Review of attachment 216931 [details] [review]: Thanks for the comments. ::: plugins/keyboard/gsd-keyboard-manager.c @@ +155,3 @@ + + g_clear_object (&priv->ibus); + g_clear_object (&priv->ibus_cancellable); Yes, I always want to cancel any in-flight async op on this cancellable. Actually, right now, it can only be the initial engine description fetch. Also, when the cancellable is NULL, g_cancellable_cancel() is a nop so I can always call it unconditionally to keep the logic simpler. @@ +256,3 @@ + g_warning ("Couldn't activate and connect to the IBus daemon"); + + set_ibus_state_off (manager); /* give up */ set_ibus_state_off() always clears it. @@ +258,3 @@ + set_ibus_state_off (manager); /* give up */ + + return FALSE; Sure, I had forgotten that we now have this. Will fix. @@ +290,3 @@ + char *args[] = { + "ibus-daemon", + "--daemonize", OK. @@ +324,3 @@ + g_clear_object (&priv->ibus); + + spawn_ibus (manager); Yes you're right. I'll just remove the first connection attempt. I had simpler version before which was hitting some race conditions but now I think I over-did it.
Created attachment 217025 [details] [review] keyboard: Add the capability to set IBus engines from input sources -- Now I'm just spawning ibus imediately. Unfortunately I can't get rid of the timeout between spawning and connecting because of the race there. And since ibus doesn't show up on the session bus I can't watch that to properly handle this.
Review of attachment 217025 [details] [review]: The code looks like it will handle ibus-daemon crashing or going away for some other reason. Have you tested such scenarios ? ::: plugins/keyboard/gsd-keyboard-manager.c @@ +170,3 @@ + GError *error; + + g_return_if_fail (priv->ibus_state == IBUS_STATE_INITIALIZING); I wonder if you can actually assert this - couldn't you get a ::disconnected signal before the list_engines_async call returns (probably with an error) ? In that case, the state would already be OFF again. @@ +297,3 @@ + g_spawn_async (NULL, args, NULL, G_SPAWN_SEARCH_PATH, NULL, NULL, NULL, &error); + + if (error) { I wonder if this works in the case where ibus-daemon is already running, though ? When trying to launch ibus-daemon a second time here: $ ibus-daemon current session already has an ibus-daemon. [mclasen@localhost ibus-1.4.0]$ echo $? 255 Unfortunately, it return 255 for every possible error condition, so you'd have to parse the error message to find out whats going on. Another alternative might be to just pass --replace
Created attachment 217134 [details] [review] keyboard: Add the capability to set IBus engines from input sources -- (In reply to comment #35) > The code looks like it will handle ibus-daemon crashing or going away for some > other reason. Have you tested such scenarios ? Yes, I've been running with it and killing ibus-daemon now and then to make sure it's working. > ::: plugins/keyboard/gsd-keyboard-manager.c > @@ +170,3 @@ > + GError *error; > + > + g_return_if_fail (priv->ibus_state == IBUS_STATE_INITIALIZING); > > I wonder if you can actually assert this - couldn't you get a ::disconnected > signal before the list_engines_async call returns (probably with an error) ? In > that case, > the state would already be OFF again. Ups, you're right, on disconnection we cancel the cancellable and the result function will still be called for the cancellation. It's expected so I'm just adding the OFF state to the assertion. > @@ +297,3 @@ > + g_spawn_async (NULL, args, NULL, G_SPAWN_SEARCH_PATH, NULL, NULL, > NULL, &error); > + > + if (error) { > > I wonder if this works in the case where ibus-daemon is already running, though > ? The spawn call will succeed and the newly spawned daemon will just exit. > When trying to launch ibus-daemon a second time here: > > $ ibus-daemon > current session already has an ibus-daemon. > [mclasen@localhost ibus-1.4.0]$ echo $? > 255 > > Unfortunately, it return 255 for every possible error condition, so you'd have > to parse the error message to find out whats going on. > > Another alternative might be to just pass --replace I actually had this before and it's what was causing a race because with --replace you end up often connecting to the older instance which will exit just after we connect. Anyway, it seems to be working here both with ibus-daemon already running before g-s-d starts or not. More problematic is the race at session init time because gnome-shell will try to connect to ibus but it might not be ready yet since all this is async. At this point I think the only option is making ibus-daemon have a presence in the session bus.
It seems "default" layout is not handled yet. If the 'default' layout is applied in ibus engine, apply_xkb_layout() could be ignored or g-s-d could take the first layout of the XKB layout engines. If it's not implemented, I'm thinking a possibility to disconnect g-s-d or ibus engine could provide many instances per layout in case that the parent is g-s-d but probably the appearance would not be good. Also I think IME switcher dialog or IME toggle trigger key is needed. > + if (engine_desc) { > + layout = ibus_engine_desc_get_layout (engine_desc); > + variant = ""; I think this needs to handle variant. E.g. layout == 'en(dvorak)', the layout is 'en' and variant is 'dvorak'. (In reply to comment #36) > More problematic is the race at session init time because gnome-shell > will try to connect to ibus but it might not be ready yet since all > this is async. At this point I think the only option is making > ibus-daemon have a presence in the session bus. Probably I think you could check 'connect' signal in js/ui/status/ibus/indicator.js
(In reply to comment #36) > > Have you tested such scenarios ? > > Yes, I've been running with it and killing ibus-daemon now and then to > make sure it's working. Ah, good. > > Another alternative might be to just pass --replace > > I actually had this before and it's what was causing a race because > with --replace you end up often connecting to the older instance which > will exit just after we connect. Anyway, it seems to be working here > both with ibus-daemon already running before g-s-d starts or not. I see. Ok then. > More problematic is the race at session init time because gnome-shell > will try to connect to ibus but it might not be ready yet since all > this is async. At this point I think the only option is making > ibus-daemon have a presence in the session bus. Doesn't the shell have to somehow retry anyway, in order to be robust against ibus-daemon crashing ? I guess a bus name would make that easier too
Created attachment 217407 [details] [review] keyboard: Add the capability to set IBus engines from input sources -- After talking with Takao on IRC we reached the conclusion that it's better to just add the ibus-daemon to the session bus. See http://code.google.com/p/ibus/issues/detail?id=1476 I changed this patch accordingly: it now relies on dbus activation to spawn ibus-daemon and watches a well known name there to track its life cylce.
Review of attachment 217407 [details] [review]: Looks good to me now - will we get the ibus bus name into rawhide soon ?
Review of attachment 217407 [details] [review]: ::: plugins/keyboard/gsd-keyboard-manager.c @@ +236,3 @@ + stop_ibus_name_watch (manager); + start_ibus_name_watch (manager); + priv->ibus_reactivated = TRUE; I don't like this "reactivated" business. Can't we just bail out if one of the calls to the ibus-daemon fails? Why would we enter a loop? @@ +619,3 @@ + goto exit; + } +#ifdef HAVE_IBUS I really don't like having an ifdef opening and closing in different blocks. @@ +627,3 @@ + GTK_IM_MODULE_SIMPLE); + g_free (module); + } else if (g_str_equal (type, INPUT_SOURCE_TYPE_IBUS)) { And you should warn here if you have an ibus engine configured and g-s-d is built without ibus support. @@ +646,3 @@ + module = g_settings_get_string (priv->interface_settings, + KEY_GTK_IM_MODULE); + if (!g_str_equal (module, GTK_IM_MODULE_IBUS)) You have that construct twice in this code. Can't you monitor which im module is in use and do this checking in a separate function?
Created attachment 217639 [details] [review] keyboard: Add the capability to set IBus engines from input sources -- (In reply to comment #41) > ::: plugins/keyboard/gsd-keyboard-manager.c > @@ +236,3 @@ > + stop_ibus_name_watch (manager); > + start_ibus_name_watch (manager); > + priv->ibus_reactivated = TRUE; > > I don't like this "reactivated" business. Can't we just bail out if one of the > calls to the ibus-daemon fails? Why would we enter a loop? The loop happens if we stop the name watch and start it again with the AUTO_START flag and dbus-daemon fails to bring ibus up for some reason. Then the vanished callback is called and... we are in a loop. I actually tried "chmod a-x `which ibus-daemon`" to see what would happen and that's why I added that flag. Now, we can just not care when ibus disappears and not try to bring it up again. It's not supposed to crash anyway and it's actually architected with engines running in separate processes to lower that likelihood. > @@ +619,3 @@ > + goto exit; > + } > +#ifdef HAVE_IBUS > > I really don't like having an ifdef opening and closing in different blocks. Ok I moved things around now. > @@ +627,3 @@ > + GTK_IM_MODULE_SIMPLE); > + g_free (module); > + } else if (g_str_equal (type, INPUT_SOURCE_TYPE_IBUS)) { > > And you should warn here if you have an ibus engine configured and g-s-d is > built without ibus support. Done. > @@ +646,3 @@ > + module = g_settings_get_string > (priv->interface_settings, > + KEY_GTK_IM_MODULE); > + if (!g_str_equal (module, GTK_IM_MODULE_IBUS)) > > You have that construct twice in this code. Can't you monitor which im module > is in use and do this checking in a separate function? Also done.
(In reply to comment #42) > Created an attachment (id=217639) [details] [review] > keyboard: Add the capability to set IBus engines from input sources > > -- > (In reply to comment #41) > > ::: plugins/keyboard/gsd-keyboard-manager.c > > @@ +236,3 @@ > > + stop_ibus_name_watch (manager); > > + start_ibus_name_watch (manager); > > + priv->ibus_reactivated = TRUE; > > > > I don't like this "reactivated" business. Can't we just bail out if one of the > > calls to the ibus-daemon fails? Why would we enter a loop? > > The loop happens if we stop the name watch and start it again with the > AUTO_START flag and dbus-daemon fails to bring ibus up for some > reason. Then the vanished callback is called and... we are in a > loop. I actually tried "chmod a-x `which ibus-daemon`" to see what > would happen and that's why I added that flag. > > Now, we can just not care when ibus disappears and not try to bring it > up again. It's not supposed to crash anyway and it's actually > architected with engines running in separate processes to lower that > likelihood. How about removing the auto_start flag, and making sure we use async function to poke it. That way, the daemon would autostart when we need poke it, and that's all. Would probably stop those loops, and get us errors in code paths where we can get to the errors.
(In reply to comment #43) > How about removing the auto_start flag, and making sure we use async function > to poke it. That way, the daemon would autostart when we need poke it, and > that's all. Would probably stop those loops, and get us errors in code paths > where we can get to the errors. Hmm, I'm afraid you might be overlooking something here. We don't do any calls on ibus-daemon through the session bus. The DBus calls that libibus does in the background for us use a connection directly to ibus-daemon. ibus-daemon didn't have any presence at all in the session DBus daemon and the only thing it's going to have there going forward is owning a well known name which just allows us to watch it and thus have a race-free way of keeping track of its life-cycle.
The problem is that you're not only keeping track of its lifecycle, but also taking care of launching it in a way that's not race-free. Let the session D-Bus launch it automatically when libibus pokes at it.
(In reply to comment #45) > The problem is that you're not only keeping track of its lifecycle, but also > taking care of launching it in a way that's not race-free. I fail to see where's the race with the current patch. It *was* racy with the previous patch that was g_spawning ibus-daemon, but now there's no race as far as I can tell. > Let the session > D-Bus launch it automatically when libibus pokes at it. Yeah, that would probably be better but it implies more ibus patching, which I can certainly do, but will probably delay us even more in having an upstream release that we can use?
Bastien, ok to land this as-is now, and work on moving the activation into libibus afterwards ? We really should land this before guadec, so we can gather some feedback.
Created attachment 218443 [details] [review] keyboard: Add the capability to set IBus engines from input sources -- This implements Bastien's suggestion. I added a new patch in the IBus bug report which tries to dbus activate the ibus daemon when connecting asynchronously. I've also modified things slightly here and am only connecting to ibus (and thus activating it) when there's actually a configured input source which needs it. This requires some changes to the control center patch which I'll update shortly.
Review of attachment 218443 [details] [review]: ::: plugins/keyboard/gsd-keyboard-manager.c @@ +76,3 @@ +#define GTK_IM_MODULE_SIMPLE "gtk-im-context-simple" +#ifdef HAVE_IBUS +#define GTK_IM_MODULE_IBUS "ibus" No need to optionally define that, it's just a string. @@ +139,3 @@ +{ + GsdKeyboardManagerPrivate *priv = manager->priv; + GList *list, *iter; Call iter, "l" instead. @@ +158,3 @@ + priv->ibus_engines = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref); + + for (iter = list; iter; iter = iter->next) add braces here, for the multi-line block. @@ +175,3 @@ + + ibus_bus_list_engines_async (priv->ibus, + -1, /* default gdbus timeout */ No need for that comment. @@ +193,3 @@ + + g_signal_connect_swapped (priv->ibus, "disconnected", + G_CALLBACK (on_ibus_disconnected), manager); As mentioned in IRC: <hadess> rtcm, i don't think we care it's been disconnected <hadess> rtcm, when it's disconnected, it's either because we don't need it (and we've destroyed the ibus object), or because it crashed, in which case it should restart the first time we poke at it @@ +227,3 @@ + G_CALLBACK (on_ibus_connected), manager); +} +#endif #endif /* HAVE_IBUS */ The start of the condition is pretty far away.
Created attachment 218480 [details] [review] keyboard: Add functionality to set IBus engines from input sources -- Addressed all the points. Thanks
Created attachment 218481 [details] [review] keyboard: Use GSettings::change-event to cut on needless work Since we end up always doing the same amount of work for in input sources settings we might as well save some when several keys change in bulk. -- Noticed that we could do an optimization here while playing with changing the input sources order in control center. I can squash this with the previous patch if you prefer.
Review of attachment 218480 [details] [review]: Looks good to commit. ::: plugins/keyboard/gsd-keyboard-manager.c @@ +145,3 @@ + g_clear_object (&priv->ibus_cancellable); + + if (error) { if (list == NULL && error != NULL) @@ +155,3 @@ + priv->ibus_engines = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_object_unref); + + for (l = list; l; l = l->next) { IBusEngineDesc *engine = l->data; and use engine in the function. @@ +200,3 @@ + } + + if (!need_ibus) Why don't you call clear_ibus() before? Surely, it should be stopped if it's not needed anymore.
Review of attachment 218481 [details] [review]: Looks good. As it's an optimisation, it can live outside the main patch.
Created attachment 218602 [details] [review] keyboard: Add functionality to set IBus engines from input sources -- Since IBus upstream doesn't want to ship a dbus service file to make it activatable but agreed to add a patch to own a well known name we concluded, on IRC, that we will ship the dbus service file for IBus. I've modified the patch to do that and activate the service by watching the well known name with the AUTO_START flag. I immediately unwatch it since we don't need to track it. I've also addressed Bastien's comments and re-worked the autoconf foo.
(In reply to comment #54) > Created an attachment (id=218602) [details] [review] > keyboard: Add functionality to set IBus engines from input sources I wish ibus package provides org.freedesktop.IBus.service to determine the arguments in ibus side while I don't mind if it's upstreamed or not.
Review of attachment 218602 [details] [review]: ::: data/Makefile.am @@ +90,3 @@ + echo 'Name=org.freedesktop.IBus'; \ + echo 'Exec=${bindir}/ibus-daemon --replace --xim --panel disable') > $@.tmp && \ + mv $@.tmp $@ Use a .in file instead please.
Created attachment 218619 [details] [review] keyboard: Add functionality to set IBus engines from input sources -- Using a .in file now. (In reply to comment #55) > I wish ibus package provides org.freedesktop.IBus.service to determine the > arguments in ibus side while I don't mind if it's upstreamed or not. I think that's not a good idea. I've added the rationale for this to the commit message: The ibus-daemon claims a well known name in the session bus but doesn't ship a .service file to make it activatable so we ship one ourselves which will spawn it in a convenient way for us. In particular, we disable its 'panel' component since that functionality is provided by gnome-shell and would conflict at run time. We also tell it to replace an existing ibus-daemon since traditionally it would be spawned at session init time before even DBus was up and out of control of gnome-session so, in case that happens, our own spawned version will take place which is what we intend since the traditionally launched ibus-daemon is highly likely to be running with its 'panel' component enabled.
Review of attachment 218619 [details] [review]: ::: configure.ac @@ +182,3 @@ + enable_ibus=yes) + +if test "x$enable_ibus" = "xyes" ; then Doesn't that fail with "auto" as the enable_ibus value? You'll still need to check for presence of ibus-1.0 in that case. Or make it a hard requirement that can be disabled with --disable-ibus. ::: data/Makefile.am @@ +90,3 @@ + $(AM_V_GEN) sed -e "s|\@bindir\@|$(bindir)|" $< > $@.tmp && mv $@.tmp $@ + +EXTRA_DIST += $(dbusservice_in_files) If you add the EXTRA_DIST within the if, you'll break dist'ing without ibus support. ::: data/org.freedesktop.IBus.service.in @@ +1,3 @@ +[D-BUS Service] +Name=org.freedesktop.IBus +Exec=@bindir@/ibus-daemon --replace --xim --panel disable Will the fallback session still get its ibus notification area icon?
Created attachment 218667 [details] [review] keyboard: Add functionality to set IBus engines from input sources -- (In reply to comment #58) > ::: configure.ac > @@ +182,3 @@ > + enable_ibus=yes) > + > +if test "x$enable_ibus" = "xyes" ; then > > Doesn't that fail with "auto" as the enable_ibus value? > You'll still need to check for presence of ibus-1.0 in that case. There's no auto. > Or make it a hard requirement that can be disabled with --disable-ibus. That's how it behaves as is. > ::: data/Makefile.am > @@ +90,3 @@ > + $(AM_V_GEN) sed -e "s|\@bindir\@|$(bindir)|" $< > $@.tmp && mv $@.tmp $@ > + > +EXTRA_DIST += $(dbusservice_in_files) > > If you add the EXTRA_DIST within the if, you'll break dist'ing without ibus > support. Fixed. > ::: data/org.freedesktop.IBus.service.in > @@ +1,3 @@ > +[D-BUS Service] > +Name=org.freedesktop.IBus > +Exec=@bindir@/ibus-daemon --replace --xim --panel disable > > Will the fallback session still get its ibus notification area icon? Hmmm, I guess it won't, no. TBH, I'm inclined to just do XKB for fallback as I'm not inclined to re-implement all the gnome-shell part in gtk+ (on which module would that live, g-s-d?) and sort of keep the status quo there for IM users.
(In reply to comment #59) > Hmmm, I guess it won't, no. TBH, I'm inclined to just do XKB for > fallback as I'm not inclined to re-implement all the gnome-shell part > in gtk+ (on which module would that live, g-s-d?) and sort of keep the > status quo there for IM users. I agree with that.
Review of attachment 218667 [details] [review]: ::: .gitignore @@ -40,3 @@ data/gnome-settings-daemon.desktop.in data/50-accessibility.xml -data/org.gnome.SettingsDaemon.service Remove that line in a separate patch please. ::: configure.ac @@ +177,3 @@ +AC_ARG_ENABLE(ibus, + AS_HELP_STRING([--enable-ibus], > That's how it behaves as is. Why is it called --enable-ibus then? @@ +188,3 @@ +fi + +PKG_CHECK_MODULES(KEYBOARD, xkbfile $IBUS_MODULE gnome-desktop-3.0 >= $GNOME_DESKTOP_REQUIRED_VERSION) Move that below the ibus checks. ::: data/Makefile.am @@ +92,3 @@ + $(AM_V_GEN) sed -e "s|\@bindir\@|$(bindir)|" $< > $@.tmp && mv $@.tmp $@ + +DISTCLEANFILES += $(dbusservice_DATA) CLEANFILES instead. ::: plugins/keyboard/gsd-keyboard-manager.c @@ +197,3 @@ + + /* DBus activate it if the well known name isn't there. */ + g_bus_unwatch_name (g_bus_watch_name (G_BUS_TYPE_SESSION, This is awful. The whole point of using a well-known name with a .service file is to make the daemon autostart when the first function on that object is poked at. You shouldn't have to do ugliness like that.
(In reply to comment #61) > +AC_ARG_ENABLE(ibus, > + AS_HELP_STRING([--enable-ibus], > > > That's how it behaves as is. > Why is it called --enable-ibus then? Changed to --disable-ibus and fixed the help string. > @@ +188,3 @@ > +PKG_CHECK_MODULES(KEYBOARD, xkbfile $IBUS_MODULE gnome-desktop-3.0 >= > $GNOME_DESKTOP_REQUIRED_VERSION) > > Move that below the ibus checks. Done. > ::: data/Makefile.am > @@ +92,3 @@ > +DISTCLEANFILES += $(dbusservice_DATA) > > CLEANFILES instead. Done. > ::: plugins/keyboard/gsd-keyboard-manager.c > @@ +197,3 @@ > + > + /* DBus activate it if the well known name isn't there. */ > + g_bus_unwatch_name (g_bus_watch_name (G_BUS_TYPE_SESSION, > > This is awful. The whole point of using a well-known name with a .service file > is to make the daemon autostart when the first function on that object is poked > at. You shouldn't have to do ugliness like that. Right. IBus is being sui generis here. I expanded the comment explaining this. Thanks for the review. Attachment 218481 [details] pushed as 207d0ef - keyboard: Use GSettings::change-event to cut on needless work Attachment 218667 [details] pushed as 557bfce - keyboard: Add functionality to set IBus engines from input sources
Other applications used to be able to use XKB to 1. get a list of the currently available layouts (or at least 4 of them) 2. select one of them That was used by for instance other keyboard layout switchers, for instance in cinnamon. Such applications that rely on the XKB API do now behave very strangely. What should applications now do to cooperate with gnome-settings-daemon?
(In reply to comment #63) > Other applications used to be able to use XKB to > 1. get a list of the currently available layouts (or at least 4 of them) > 2. select one of them > > That was used by for instance other keyboard layout switchers, for instance in > cinnamon. Such applications that rely on the XKB API do now behave very > strangely. > > What should applications now do to cooperate with gnome-settings-daemon? Undefined at this point. If you have specific requests or use cases that you'd like addressed please file a new bug and explain your rationale there.
Filed Bug 687935 - Need for some "official" API for interacting with new 3.6 keyboard layout mechanism