GNOME Bugzilla – Bug 680313
keyboard: deal with input methods for non-gtk apps
Last modified: 2012-09-04 23:23:00 UTC
I noticed that gitk (which is a tk app) seems stuck in anthy after I switched the shell keyboard menu back to English. I think whats happening here is that gitk probably uses xim, and because of XMODIFIERS=@im=ibus in the environment, it talks to ibus, but we don't tell ibus that we're using 'en' now, because we rely on gtk apps switching from the ibus module to simple...
Created attachment 222265 [details] [review] main: Set session env vars to make non gtk+ apps work with IBus If we are compiled with IBus support and the current session isn't fallback then we always set the QT_IM_MODULE and XMODIFIERS environment variables so that Qt and XIM supporting applications work as best as possible.
Created attachment 222266 [details] [review] keyboard: Instruct IBus to use its XKB engine for XKB input sources Otherwise Qt and XIM supporting applications would continue using the last IBus engine that was set before the user switched to a XKB input source.
Review of attachment 222265 [details] [review]: This is a gross place to be doing it, could we at least split it up in another source file?
Review of attachment 222266 [details] [review]: ::: plugins/keyboard/gsd-keyboard-manager.c @@ +133,3 @@ + +static gchar * +make_xkb_engine_id (const gchar *engine_id) are you making an engine id? why do you take one as input? @@ +140,3 @@ + + /* engine_id is like "xkb:layout:variant:lang" where + * 'variant' and 'lang' might be empty */ Could you add a test case for this function, with expected input and output? @@ +207,3 @@ + if (strncmp ("xkb:", engine_id, 4) == 0) + g_hash_table_replace (priv->ibus_xkb_engines, + make_xkb_engine_id (engine_id), You don't check the output of make_xkb_engine_id() then? @@ +208,3 @@ + g_hash_table_replace (priv->ibus_xkb_engines, + make_xkb_engine_id (engine_id), + g_object_ref (engine)); Why do you ref it here, and not a couple of lines before?
Created attachment 222954 [details] [review] main: Set session env vars to make non gtk+ apps work with IBus -- * Added a call to register_with_gnome_session() in the error branch after g_dbus_proxy_call_finish() so that the session init process can continue in this error case. (In reply to comment #3) > This is a gross place to be doing it, could we at least split it up in another > source file? Another file for a dbus call and a couple of set_session_env() calls? Consider that this really has to be done very early on, before we register with gnome-session since that's when gnome-session accepts calls to the Setenv method. Also, when we get rid of fallback mode this can essentially be reverted and the set_session_env() calls can be made unconditionally in got_session_proxy() before registering, it's 4 lines patch including the #ifdef/#endif...
I just really don't like crap like that taking space in what should be the simplest code. Commit if you want, just make sure that a bug gets opened about it.
Created attachment 222976 [details] [review] keyboard: Instruct IBus to use its XKB engine for XKB input sources -- (In reply to comment #4) > +make_xkb_engine_id (const gchar *engine_id) > > are you making an engine id? why do you take one as input? Right, this should really be called make_xkb_source_id(). Changed here. > @@ +207,3 @@ > + if (strncmp ("xkb:", engine_id, 4) == 0) > + g_hash_table_replace (priv->ibus_xkb_engines, > + make_xkb_engine_id (engine_id), > > You don't check the output of make_xkb_engine_id() then? Checking now. > @@ +208,3 @@ > + g_hash_table_replace (priv->ibus_xkb_engines, > + make_xkb_engine_id (engine_id), > + g_object_ref (engine)); > > Why do you ref it here, and not a couple of lines before? That was because the ibus call returns us referenced objects already and I was taking a second one for this hash table. But it's not needed really since both hash tables have the same life span. So I just removed the ref here and the unref from this hash table's value destroy notify.
Created attachment 222977 [details] [review] keyboard: Add test cases for make_xkb_source_id() -- > @@ +140,3 @@ > + > + /* engine_id is like "xkb:layout:variant:lang" where > + * 'variant' and 'lang' might be empty */ > > Could you add a test case for this function, with expected input and output? Added. Not sure if this is the best way to add tests to the existing framework but it seems to work.
Review of attachment 222954 [details] [review]: ::: gnome-settings-daemon/main.c @@ +307,3 @@ set_locale (proxy); +#ifdef HAVE_IBUS + set_legacy_ibus_env_vars (proxy); Please make a note of set_legacy_ibus_env_vars() calling register_with_gnome_session() when done. I had to look at where you were calling it. Maybe a note in set_legacy_ibus_env_vars() would be nice too.
Review of attachment 222976 [details] [review]: Looks fine.
Review of attachment 222977 [details] [review]: For how to integrate the test, see test-hostname used in gnome-control-center: http://git.gnome.org/browse/gnome-control-center/tree/panels/info/Makefile.am "make test" (which make distcheck calls) would then error out if there were regressions. ::: plugins/keyboard/test-keyboard.c @@ +7,3 @@ + + g_assert (g_strcmp0 (s = make_xkb_source_id ("xkb:aa:bb:cc"), "aa+bb") == 0); + g_free (s); A lot of duplication for something that can be automated. For example: g_assert_cmpstr (make_xkb_source_id ("xkb:aa:bb:"), ==, "aa+bb"); (minus the leak, if you insist). and use an array to store the input and expected output.
Created attachment 223341 [details] [review] keyboard: Add test cases for make_xkb_source_id() -- Something like this?
Review of attachment 223341 [details] [review]: Looks good.
(In reply to comment #9) > ::: gnome-settings-daemon/main.c > @@ +307,3 @@ > set_locale (proxy); > +#ifdef HAVE_IBUS > + set_legacy_ibus_env_vars (proxy); > > Please make a note of set_legacy_ibus_env_vars() calling > register_with_gnome_session() when done. I had to look at where you were > calling it. Maybe a note in set_legacy_ibus_env_vars() would be nice too. Added a couple of comments to make that clear and pushed. Attachment 222954 [details] pushed as ffc1aa5 - main: Set session env vars to make non gtk+ apps work with IBus Attachment 222976 [details] pushed as 3687fb7 - keyboard: Instruct IBus to use its XKB engine for XKB input sources Attachment 223341 [details] pushed as 316baa6 - keyboard: Add test cases for make_xkb_source_id()
It seems the latter commit (316baa6) causes a build failure here (perhaps with --as-needed or no-undefined?) /me is looking.
(In reply to comment #15) > It seems the latter commit (316baa6) causes a build failure here (perhaps with > --as-needed or no-undefined?) /me is looking. See bug 683366.