GNOME Bugzilla – Bug 683798
keyboard: Use the async IBus global engine setter
Last modified: 2012-09-13 12:37:58 UTC
Especially when first switching to a python engine like Anthy which can take some seconds to load (slow HDDs and all) we could find ourselves blocked here. This fixes that.
Created attachment 224023 [details] [review] keyboard: Use the async IBus global engine setter
Review of attachment 224023 [details] [review]: Other than that, the patch looks good to me. ::: plugins/keyboard/gsd-keyboard-manager.c @@ +378,3 @@ + g_cancellable_cancel (priv->ibus_cancellable); + g_clear_object (&priv->ibus_cancellable); + priv->ibus_cancellable = g_cancellable_new (); Looking through the code, the handling of ibus_cancellable is not 100% consistent - in some places you just g_clear_object, in others you just set it, and it is only sporadically cancelled - you should probably review that and make sure it makes sense.
Nothing more to add.
(In reply to comment #2) > Looking through the code, the handling of ibus_cancellable is not 100% > consistent - in some places you just g_clear_object, in others you just set it, > and it is only sporadically cancelled - you should probably review that and > make sure it makes sense. Right, it's not consistent mainly because in most cases there is no way that a cancellable instance is being used and we want to create another one. That only happens now with this patch in case the user is changing input sources quickly. But let's go though all the cancellable uses: 1. we create one for g_bus_get() on plugin start a) this might get cancelled and cleared via clear_ibus() on plugin stop b) regularly it will be cleared in got_bus() 2. we create a second one in got_bus() a) same as 1.a) b) gets cleared in got_session_name() 3. got_session_name() goes on to call apply_input_sources_settings() which calls maybe_start_ibus(). Note that apply_input_sources_settings() might be called at other various points but maybe_start_ibus() won't do anything while session_if_fallback is TRUE which we do set on plugin start 4. when we get connected to ibus fetch_ibus_engines() runs and creates another cancellable a) same as 1.a) but might also happen on disconnection from ibus b) gets cleared in fetch_ibus_engines_result() 5. apply_input_sources_settings() is called and goes on to call either set_ibus_xkb_engine() or set_ibus_engine() which creates another cancellable a) same as 4.a) but now another engine might be set by the user so set_ibus_engine() also cancels and clears a possible existing cancellable before creating a new one b) finally this one always gets cleared in set_ibus_engine_finish() I think we are safe here but I can add some assertions if you'd like.
Adding some assertions (along with some thorough testing) would probably be a good idea.
(In reply to comment #5) > Adding some assertions (along with some thorough testing) would probably be a > good idea. That commit can be separate from this patch, just make sure it's filed as a bug before closing this one.
Created attachment 224165 [details] [review] keyboard: Check invariants on async code paths -- It all seems to work as expected.
Review of attachment 224165 [details] [review]: Looks good.
Attachment 224023 [details] pushed as 205c8af - keyboard: Use the async IBus global engine setter Attachment 224165 [details] pushed as 9d715bb - keyboard: Check invariants on async code paths