GNOME Bugzilla – Bug 679819
3.6: fallback and keyboard configuration
Last modified: 2012-08-29 11:38:10 UTC
As I understand it, http://git.gnome.org/browse/gnome-settings-daemon/commit/?id=b6e011ad16311e2985aa523cf9bceef74168f95e means that there's not a UI for switching between input methods and keyboard layouts in fallback 3.6. For non-English fallback mode users, this could be a serious regression. What to do about it an open question: 1) Implement the new UI design in fallback 2) Resurrect the old keyboard plugin, only use it in fallback 3) Do nothing, document the regression, and move on
How is the control-center panel affected by fallback mode ? Is that not 'a UI for switching between input methods and keyboard layouts' ?
I guess what you are saying is that there will no longer be a status icon to switch keyboard layouts. I'm assuming that the ibus statusicon will work fine for input methods. One idea would be to take the statusicon code from the old keyboard plugin and make it a standalone binary.
Created attachment 222432 [details] [review] keyboard: Don't use IBus if running in fallback mode In fallback mode we don't want to touch IBus since the gnome-shell UI isn't there to provide the full integrated user experience. We expect users to continue using existing IBus UIs and configurations in fallback mode. -- Besides this, we will also need a status icon and menu for keyboard layouts (i.e. XKB input sources) when running in fallback mode. Patch for that will come later.
Review of attachment 222432 [details] [review]: Thanks for looking at this Rui! So my main concern at this point is whether we're opening up any startup race conditions. gnome-settings-daemon and gnome-shell are started in parallel, so the org.gnome.Shell name may or may not be there. The current code looks safe in that it won't crash, but would it be possible to not have ibus set up?
(In reply to comment #4) > Thanks for looking at this Rui! So my main concern at this point is whether > we're opening up any startup race conditions. gnome-settings-daemon and > gnome-shell are started in parallel, so the org.gnome.Shell name may or may not > be there. Actually g-s-d starts before the shell because gnome-session has this concept of phases and g-s-d starts in phase INITIALIZATION while the shell starts in WINDOW_MANAGER. Nonetheless, the race exists because g-s-d's plugins are initialized in idles after g-s-d has registered with gnome-session and thus gnome-session has advanced to the WINDOW_MANAGER phase. > The current code looks safe in that it won't crash, but would it be possible to > not have ibus set up? There's no way for that to happen since we always apply the settings when the shell shows up on the bus. Actually in many cases we apply the same settings twice which is correct but inefficient. There's even an easy fix for that which is calling apply_input_sources_settings() on the shell name vanished handler instead of directly on initialization. New patch coming for that.
Created attachment 222492 [details] [review] keyboard: Don't use IBus if running in fallback mode -- This applies the settings both on the shell name showing up and vanishing instead of directly on plugin init which avoids sometimes applying the same configuration twice during startup.
(In reply to comment #6) > Created an attachment (id=222492) [details] [review] > keyboard: Don't use IBus if running in fallback mode > > -- > > This applies the settings both on the shell name showing up and > vanishing instead of directly on plugin init which avoids sometimes > applying the same configuration twice during startup. Hmm, well, this will apply the same settings twice when g-s-d's plugin init wins the race. I guess we can't win here :-(
(In reply to comment #7) > (In reply to comment #6) > > Created an attachment (id=222492) [details] [review] [details] [review] > > keyboard: Don't use IBus if running in fallback mode > > > > -- > > > > This applies the settings both on the shell name showing up and > > vanishing instead of directly on plugin init which avoids sometimes > > applying the same configuration twice during startup. > > Hmm, well, this will apply the same settings twice when g-s-d's plugin init > wins the race. I guess we can't win here :-( If it's just an efficiency and not correctness thing, let's go forward with this patch now. It's worth noting in a comment in the source code at least though that there's a race condition that leads to non-optimal behavior.
I thought the plan was to look at the session name property (for name != gnome) on the session manager interface ?
(In reply to comment #9) > I thought the plan was to look at the session name property (for name != gnome) > on the session manager interface ? That was for bug 680313. But now that you mention it, it's probably OK to also use that here instead of the shell name. That way this wouldn't be racy anymore.
Comment on attachment 222492 [details] [review] keyboard: Don't use IBus if running in fallback mode As mentioned in the comments, let's use the session name instead.
Created attachment 222673 [details] [review] keyboard: Don't use IBus if running in fallback mode -- It's a bit more code but it's not racy and more efficient. I think I covered all the error cases so that we don't end up in a situation where we don't apply the settings. Also, didn't use a cancellable but it should be safe against the GsdKeyboardManager being destroyed. Nonetheless, please review carefully.
Review of attachment 222673 [details] [review]: ::: plugins/keyboard/gsd-keyboard-manager.c @@ +295,3 @@ + + if (!GSD_IS_KEYBOARD_MANAGER (manager)) + goto out; There's a single line in the out branch, just do it here. FWIW, this is broken though, as the manager pointer could be pointing at anything it wanted. You'll need to use a cancellable, or just ignore the problem. @@ +328,3 @@ + GError *error = NULL; + + if (!GSD_IS_KEYBOARD_MANAGER (manager)) As above.
Created attachment 222726 [details] [review] keyboard: Don't use IBus if running in fallback mode -- Right, here it is now using the cancellable. I'm also now deferring the ibus object creation only to when we are sure we need it since I'm using the same variable to hold the cancellable for the async ibus calls.
Review of attachment 222726 [details] [review]: Apart from the couple of misuses of GErrors ::: plugins/keyboard/gsd-keyboard-manager.c @@ +306,3 @@ + + result = g_dbus_connection_call_finish (connection, res, &error); + if (!result && error) { if (!result) error should be set, otherwise that means that g_dbus_connection_call_finish() is broken. @@ +338,3 @@ + + connection = g_bus_get_finish (res, &error); + if (!connection && error) { Ditto with the error.
Amended and pushed. Thanks Attachment 222726 [details] pushed as fe3f85f - keyboard: Don't use IBus if running in fallback mode