GNOME Bugzilla – Bug 683875
Add IBus support to "System" tab
Last modified: 2012-09-17 12:11:18 UTC
I can spot are a couple of minor consistency issues with the strings we display in the "Your Settings" section of the System tab, compared to what we show in the tabs that those settings mirror. - "Display Language" says "English (United States)", but the Language tab says "English" - "Input source" is empty, but the Input Sources tab shows I selected "English (US)" See attached screenshots.
Created attachment 224118 [details] system tab
Created attachment 224119 [details] language tab
Created attachment 224120 [details] input sources tab
Or special-case "IBus" rather. Until we have a way of passing through the IBus setup to GDM, I would only make the layouts button available if there was no IBus input source setup.
I'm not sure how IBus enters the picture here, but some of these string mismatches might be orthogonal to using IBus or not.
(In reply to comment #5) > I'm not sure how IBus enters the picture here, but some of these string > mismatches might be orthogonal to using IBus or not. The language label in the language tab vs. the language label in the system tab is expected. We show, on purpose, the short name for the language instead of the full one. If you added a language that's not in that list, the language names would match. I was more concerned about the empty "input sources" field.
(In reply to comment #6) > The language label in the language tab vs. the language label in the system tab > is expected. We show, on purpose, the short name for the language instead of > the full one. If you added a language that's not in that list, the language > names would match. > > I was more concerned about the empty "input sources" field. Okay, makes sense...thanks for the clarification.
Created attachment 224173 [details] [review] region: Handle IBus sources in the system tab Unfinished: - Needs to specifically handle IBus sources (eg. disabling copy to system, etc.) - Needs to get names properly - Needs to handle variants
Created attachment 224229 [details] [review] region: Handle IBus sources in the system tab
Created attachment 224230 [details] [review] region: Handle IBus sources in the system tab
Review of attachment 224230 [details] [review]: I think that trying to set > 1 XKB sources as XKB groups in xorg.conf isn't right. After all, users won't be able to switch group anyway. I think the best approach for now is to just store the first xkb input source layout+variant in the system setting. Likewise when getting the system setting I think we can just ignore layouts and variants after the first comma. ::: panels/region/gnome-region-panel-system.c @@ +157,3 @@ + sources = g_settings_get_value (input_sources_settings, "sources"); + if (sources == NULL) { + //FIXME I don't think this can happen. Did you intend to handle the empty sources list case? In that case I could make the new function I just added in gnome-region-panel-input.c public so that we could use it here. @@ +161,3 @@ + } + + xkb_info = gnome_xkb_info_new (); We really have to share this object, doesn't make sense to have several instances of it since it only has immutable data. I have a patch to make it a singleton in gnome-desktop but I generally don't like forcing classes to be singletons like that, I think it would be better to have it accessible in a global get method to the whole g-c-c. @@ +181,3 @@ + g_string_append (disp, name); + + split = g_strsplit_set (id, " \t", 2); g_strsplit (id, "+", 2) That's the separator used in GnomeXkbInfo. @@ +316,3 @@ + label = WID ("system_input_source"); + v = g_dbus_proxy_get_cached_property (proxy, "X11Layout"); There's also "X11Variant" to deal with here. @@ +326,3 @@ + } + + xkb_info = gnome_xkb_info_new (); Idem.
(In reply to comment #11) > Review of attachment 224230 [details] [review]: > > I think that trying to set > 1 XKB sources as XKB groups in xorg.conf isn't > right. After all, users won't be able to switch group anyway. That depends on what the xorg settings are actually... > ::: panels/region/gnome-region-panel-system.c > @@ +157,3 @@ > + sources = g_settings_get_value (input_sources_settings, "sources"); > + if (sources == NULL) { > + //FIXME > > I don't think this can happen. Did you intend to handle the empty sources list > case? In that case I could make the new function I just added in > gnome-region-panel-input.c public so that we could use it here. That would be great. > @@ +161,3 @@ > + } > + > + xkb_info = gnome_xkb_info_new (); > > We really have to share this object, doesn't make sense to have several > instances of it since it only has immutable data. I have a patch to make it a > singleton in gnome-desktop but I generally don't like forcing classes to be > singletons like that, I think it would be better to have it accessible in a > global get method to the whole g-c-c. Either/or, not fussed. > @@ +181,3 @@ > + g_string_append (disp, name); > + > + split = g_strsplit_set (id, " \t", 2); > > g_strsplit (id, "+", 2) > > That's the separator used in GnomeXkbInfo. OK. > @@ +316,3 @@ > > + label = WID ("system_input_source"); > + v = g_dbus_proxy_get_cached_property (proxy, "X11Layout"); > > There's also "X11Variant" to deal with here. > > @@ +326,3 @@ > + } > + > + xkb_info = gnome_xkb_info_new (); > > Idem. That wasn't handled before.
Created attachment 224291 [details] [review] region: Handle input sources in the system tab -- * Changed the commit summary: s/IBus/input * Addressed my comments except the GnomeXkbInfo singleton which doens't belong here * Kept copying > 1 XKB sources to system in a XKB group * Changed to skipping non-XKB sources instead of refusing to copy when one non-XKB source is present.
Created attachment 224314 [details] [review] region: Handle input sources in the system tab Unfortunately we don't have a way yet to make gsettings system wide defaults so we are just using the localed API to export the XKB input sources and ignore the IBus ones. -- * Added a commit message * Disabled the copy button when we don't have any user sources, e.g. when the user only has IBus sources configured
Review of attachment 224314 [details] [review]: Looks good to me