GNOME Bugzilla – Bug 662489
region: implement input sources
Last modified: 2012-07-16 14:26:20 UTC
I've started to put together a prototype implementation for the 'Input sources' tab that is described here: http://live.gnome.org/Design/SystemSettings/RegionAndLanguage The code lives in the wip/input-sources branch: http://git.gnome.org/browse/gnome-control-center/log/?h=wip/input-sources It uses IBus. The basics work ok: - Input sources can be added/removed/rearranged - Configuration gets read from IBus and written back to it - Keyboard layouts can be previewed Things that still need work: - System-wide settings - Overlap between xkb and m17n input sources (these all get reported by IBus, but we only want to show one of them) Some things don't quite work right in IBus: - previous/next shortcuts don't work right - toggling input methods on/off does not really fit in our design
This is the great thing indeed! Just a couple of questions, not clear from that l.g.o page: How do you plan to distinguish between layouts available through IBus and the layouts from xkb? If some layout available from both sources - which one do you plan to use? Will user be allowed to choose? What about xkb options? I could not see them in mockups.
Created attachment 199830 [details] how it looks
Created attachment 199831 [details] how it looks
Created attachment 199832 [details] how it looks
note that the first screenshot still shows the 'layout' tab, because I haven't removed it yet; the end goal is to replace 'Layouts' with 'Input Sources'
This looks great indeed! Only one thing, about the 2nd screenshot. I guess it might make sense to not show the (xkb), (inscript), etc? Maybe we can display a more descriptive string?
The 'xkb' is temporary. I think I described it somewhere above that it is needed to discriminate between 'real' xkb keyboard layouts and 'simulated' keyboard layouts for the same language coming from m17n. I think we just need to filter out the simulated ones, and then the need for '(xkb)' goes away. For other strings, like 'inscript', we simply don't have better strings, and I believe these are well known to the affected users.
Thanks - this looks pretty promising I think for gnome-control-center.
About xkb vs m17n - I think generally yes we can prefer the xkb layout but there may be cases eg some Indic maps where we might want to prefer the m17n map over the xkb map say - anyway that is basically polishing that can be done later on.
I have implemented a bit more of the latest mockups at https://live.gnome.org/Design/SystemSettings/RegionAndLanguage#Input_sources Screenshots: http://mclasen.fedorapeople.org/blackeye.png http://mclasen.fedorapeople.org/input-shortcuts.png The 'Shortcut Settings' link is functional, and editing the shortcuts does correctly update the ibus configuration. This part relies on a small gnome-settings-daemon plugin to translate from 'normal' accelerators to something that ibus can understand, which is not ideal. It would be much better if IBus would just use GTK+ functions for key+mods <> accelstring translations instead the current half-copied half-homegrown code. The black square is supposed to be a 'input method preferences' button - it doesn't do anything yet. The gnome-settings-daemon code can be found in the wip/ibus branch.
(In reply to comment #10) <snip> > The gnome-settings-daemon code can be found in the wip/ibus branch. - Add a print at the end of the configure.ac to show whether ibus is enabled - In org.gnome.settings-daemon.plugins.ibus.gschema.xml.in.in, you don't need to use a sub-dir for the next/prev input source, you should also mention that those are keyboard shortcuts in a format parseable by GTK+ - separate the ibus accel translation function, so that we can have a small test program that can be run as part of make check to verify translations. - ibus doesn't seem to support keycodes as shortcuts, does it? That's a problem, because GTK+ and mutter do. Please post a single combined patch to a separate (g-s-d) bugzilla if you want this reviewed.
Created attachment 214115 [details] [review] Add an initial input sources tab This is just the scaffolding according to https://live.gnome.org/Design/SystemSettings/RegionAndLanguage No code behind it yet, and the XKB tab has not been removed yet.
Created attachment 214116 [details] [review] Region: Removal of the Layouts tab Still to do: system-wide settings for input sources
Created attachment 214117 [details] [review] Keyboard: Make it possible to jump to shortcuts tab To make this work, we need to move the setting up of priv->builder to the instance init, so that it is available when construct properties are set; the other setup needs to remain in the constructor, since it relies on construct properties.
Created attachment 214118 [details] [review] Region: Add a 'Shortcuts' link to the input sources tab Now we just need to make the shortcuts page actually provide previous/next input source shortcuts.
Created attachment 214119 [details] [review] Group tool buttons Also add an (unfunctional for now) 'Input Source Settings' button. Unfortunately, the preferences-system-symbolic icon is broken, so it comes out black. http://mclasen.fedorapeople.org/blackeye.png
Created attachment 214120 [details] [review] region: Remove the IBus engine switch keybindings handling We want to do the keybinding capturing using GNOME's existing infrastructure to keep things consistent.
Created attachment 214121 [details] [review] keyboard: Add shortcuts to switch among input sources
Created attachment 214122 [details] [review] region: Improved keynav and selection handling on the input chooser
Created attachment 214124 [details] [review] region: Fix a couple of memory leaks
Rui, I see the layout switching shortcut is defined here, and can be configured in shortcuts. Does that mean that even for XKB layouts (for example, US and Russian) you are not allowing server-side layout switching?
Also, you are creating hard dependency on ibus. Is it a bit early, considering the hot discussion on d-d-l?
Created attachment 214125 [details] [review] region: Get the available XKB layouts from the XKB rules file Instead of asking IBus, we list the XKB layouts available and then add an hand-picked set of input sources that lists both an XKB layout and an IBus engine that work together for the languages that require it. For now we hard-code the IBus sources set. This also removes the dependency on IBus. -- I'm still a bit undecided about what exactly to put in the chooser list. Some people really want to see all the XKB layouts (which I ended up implementing here) but I personally think that there is a lot of exoteric stuff in there that we shouldn't expose like "Esperanto (Portugal, Nativo)"... Then the input sources which need an IME are just hardcoded right now because just asking IBus will also yeld a lot of noise in my opinion. I'd really appreciate opinions on this.
Some of the patches are from Matthias' original branch and I squashed some of them to make the whole set more comprehensible. I hope that's OK.
Rui, there is also base.extras.xml. Please make it optionally visible (like libxklavier does). That's where really exotic stuff go (and people will complain, and I will support them). What I do not quite understand - why cannot you use libxklavier, essentially duplicating its functionality. Libxklavier is designed to protect people from future base.xml format changes. Looks like NIH syndrom from here;) Minor thing - the default model should be evdev rather than pc105, for modern X.Org on Linux.
(In reply to comment #21) > Rui, I see the layout switching shortcut is defined here, and can be configured > in shortcuts. Does that mean that even for XKB layouts (for example, US and > Russian) you are not allowing server-side layout switching? Exactly, I still haven't seen a totally convincing argument against that. Also, see the patches in bug 676102 to understand a bit better what and why I'm doing this.
When you IBus is switching between "ru" and "us" (both XKB) - is it changing groups or totally reconfiguring XKB (similar to setxkbmap). If latter - it is "no go", because (as it was discussed @d-d-k) it is too expensive. Do you have "smart" XKB management (so for 5 groups you "lazyly" configure XKB on the fly only when absolutely necessary)? One more question - you always add "latin" layout (which is 'us'). What if a person chooses one of the layout like 'fr', 'es', ... - why would you add 'us'? Regarding parsing of base.xml - still not sure it makes sense to duplicate the code. Even if you are not using some of libxklavier functions, the metadata parsing code can be reused independently.
(In reply to comment #25) > Rui, there is also base.extras.xml. Please make it optionally visible (like > libxklavier does). That's where really exotic stuff go (and people will > complain, and I will support them). Personally I want to see *less* stuff in there, and not have an option for more. But let's see what other say. Actually, I think it would be good if we moved more stuff in base.xml into base.extras.xml, that would be a way to have a saner list here. > What I do not quite understand - why cannot you use libxklavier, essentially > duplicating its functionality. Libxklavier is designed to protect people from > future base.xml format changes. Looks like NIH syndrom from here;) 3 reasons: a) Initially I was going to do this as a totally separate list of layouts that we would maintain in GNOME. b) Then I convinced myself that it would just be a duplication of effort that could be done better in xkeyboard-config so I looked into parsing it with libxklavier but that library does a lot of other stuff that we really don't need and the XML file is quite simple to parse so, yeah it's a bit o NIH I'd say. c) If you look at g-s-d patches you'll see that I needed a way to query for an XKB layout that fits well with UI language so I needed to add that functionality as well. > Minor thing - the default model should be evdev rather than pc105, for modern > X.Org on Linux. Ok, I'll change that.
> Actually, I think it would be good if we moved more stuff in base.xml into base.extras.xml, that would be a way to have a saner list here. Here, I offer you a deal. I am willing to discuss moving things from base.xml to base.extras.xml (and I promise to be quite open on that!) - and in return you agree go create gsetting (without UI) that would allow people seeing those extas. What do you think? > yeah it's a bit o NIH I'd say. Ok, do not blame me, if one day that format changes:) > I needed a way to query for an XKB layout It would be quite logical to add that to libxklavier... The XklConfigRegistry is intended for those things.
(In reply to comment #27) > When you IBus is switching between "ru" and "us" (both XKB) - is it changing > groups or totally reconfiguring XKB (similar to setxkbmap). If latter - it is > "no go", because (as it was discussed @d-d-k) it is too expensive. Do you have > "smart" XKB management (so for 5 groups you "lazyly" configure XKB on the fly > only when absolutely necessary)? No, I still don't see it as "too expensive" and if it turns out to be we can either fix the performance problems then or change the implementation here. In my testing I don't see any visible impact from doing it this way. Also, 2 XKB group slots are being filled with a "latin" layout and a "UI language" layout to make sure that accelerators and mnemonics always work, but this is an implementation detail really. > One more question - you always add "latin" layout (which is 'us'). What if a > person chooses one of the layout like 'fr', 'es', ... - why would you add 'us'? It can surely be better. I just haven't found a good way yet to determine what is a "latin layout". I'm happy to hear some ideas about this.
> I still don't see it as "too expensive" Really? Did you write that? http://mail.gnome.org/archives/desktop-devel-list/2012-April/msg00178.html > Also, 2 XKB group slots are being filled with a "latin" layout and a "UI language" layout to make sure that accelerators and mnemonics always work, but this is an implementation detail really. Ok. So if, for my case, only "us" and "ru" are needed - why do you want to reconfigure XKB if you already have 2 layouts I need, and just switching the groups on the server would be just enough for me? > I'm happy to hear some ideas about this. More metainfo to base.xml. I do not see any other way. Any other way would either be hardcoding or moving of useful metainfo out of xkeyboard-config. And, as I said, it is always better to handle that change in one place, libxklavier (it will be there anyway). What about the deal regarding base.extras.xml? Deal or no deal?
(In reply to comment #31) > > I still don't see it as "too expensive" > Really? Did you write that? > http://mail.gnome.org/archives/desktop-devel-list/2012-April/msg00178.html Yes, then after having written that I looked into both mutter and gtk+ and found that: 1) mutter has been doing what Owen says there not once but 10 times. Fix is in bug 674859. If I previously didn't notice it and it was doing 10x more work I think it's fair to say that this isn't much of a performance bottleneck. 2) gtk+ actually handles this gracefully. It will get the event but will only set a flag which then triggers the hard work only at the very last moment, i.e. when the information is actually queried. > Ok. So if, for my case, only "us" and "ru" are needed - why do you want to > reconfigure XKB if you already have 2 layouts I need, and just switching the > groups on the server would be just enough for me? And why not, if it doesn't have any ill effects? > > I'm happy to hear some ideas about this. > More metainfo to base.xml. I do not see any other way. Any other way would > either be hardcoding or moving of useful metainfo out of xkeyboard-config. Sounds like a plan. I'll think about it for a bit and probably submit a patch to you later. > What about the deal regarding base.extras.xml? Deal or no deal? I'm not in a position to do any kind of deal nor do I think you should develop software like that. Besides, I'm not the maintainer of any of these modules and the implementation here might still change substantially after review and/or design input.
> Yes, then after having written that I looked into both mutter and gtk+ and > found that: Perhaps it would make sense to share your results at d-d-l - at least if someone have other concerns, he could fire them. I still think that changing several bits is soo less expensive. Especially in case similar 'us'-'ru' - which I am sure covers huge share of users (I would not be surprised if it is >50% of Europe). > And why not, if it doesn't have any ill effects? CPU, battery etc. Actually, you are asking why not ignore the existing working and efficient approach. Isn't it strange kind of question? For people who do not need the beauties of IM, the XKB was working well enough for years! So, I have to ask my question - why not use what's already out there an was working for GNOME flawlessly (well, ignoring the bugs in libxklavier/libgnomekbd/g-s-d - but these days that stack is mostly solid). Really, I do not understand - what is the fundamental issue that prevents from detecting the use case "Input Sources are XKB only and number of groups <=4" - and just switching groups in that scenario (ok, if you hate XKB shortcuts expressed in XKB options, you can manually lock groups in mutter, if you like). That scenario is SO frequent, that it would be right to support it - considering the performance gain. Even modern desktop can be under heavy load, so selling CPU ticks for nothing(!) does not look wise. > I'm not in a position to do any kind of deal nor do I think you should develop > software like that. Besides, I'm not the maintainer of any of these modules and > the implementation here might still change substantially after review and/or > design input. All I am asking is KEEP the existing functionality, not to add the new one. That is the way GNOME behaves today - it gives access to extras. Since you are the person who is changing that code - I guess it is quite logical for me to ask you to KEEP that feature. The change in the visual design is practically zero (ok, I displayed those layouts in italic, you can change that).
Let just get that 'too expensive' argument out of the way please. It is just flat-out unbelievable at a time where we do fullscreen animations at 60fps that occasionally setting up a few keymaps should be too much overhead. It isn't. And lets also stop the haggling about how many of the xkb options we can 'save'. That is not a way to develop usable software. Allan did quite a bit of research on those options, and which of them may or may not make sense in the panel: https://live.gnome.org/Design/SystemSettings/RegionAndLanguage If you look at that page, that the design envisions that the tweak tool could provide a home for some of the tweaky-but-sometimes-useful-to-programmers options.
> setting up a few keymaps should be too much overhead As it was pointed out by Owen, the question is not the keymap change itself - but the circles on the water... The apps that listen to that event and perform some expensive actions. Ok, let's believe it is cheap enough. BTW, the argument about 60fps animations is not quite relevant - it is about GPU more than CPU. Regarding the options - I am not talking about options here (which is another sad story for XKB lovers). I am talking about layouts and variants. They are going to be presented in the user interface anyway, the stuff from base.xml. All I am asking is keeping the ability to show 200 items instead of 100, adding base.extras.xml
Review of attachment 214115 [details] [review]: A first pass. ::: panels/region/gnome-region-panel-input.c @@ +28,3 @@ + +#include <ibus.h> +/* http://code.google.com/p/ibus/issues/detail?id=1338 */ The bug says it's fixed. @@ +40,3 @@ +#define WID(s) GTK_WIDGET(gtk_builder_get_object (builder, s)) + +/* TODO As this been updated? Could we have that TODO list somewhere else (eg. not in code)? @@ +99,3 @@ + { + if (strcmp (ibus_engine_desc_get_longname (description), lang) == 0) + return g_strdup_printf ("%s (xkb)", lang); xkb in user-visible strings? Eek. @@ +118,3 @@ + +static void +populate_model (GtkListStore *store, Need a test application, and test cases. Wouldn't this need to be shared with gnome-shell code? Probably needs to go in gnome-desktop if so. @@ +146,3 @@ + for (l = list; l; l = l->next) + { + description = (IBusEngineDesc *)l->data; No need to cast here FWIW. @@ +243,3 @@ + + bus = ibus_bus_new (); + config = ibus_bus_get_config (bus); The ibus configuration needs to be set in one place only. In gnome-settings-daemon, in response to the gsettings value changing. @@ +272,3 @@ + *next = NULL; + + value = ibus_config_get_value (config, "general/hotkey", "previous_engine"); I believe we want this code (setting the shortcut) in the keyboard panel, with the current value being displayed on screen (see the universal access panel for an example of that). @@ +375,3 @@ + GtkBuilder *builder = data; + + if (response_id == GTK_RESPONSE_OK) if (response_id != GTK_RESPONSE_OK) return; @@ +776,3 @@ + + gtk_window_set_transient_for (GTK_WINDOW (chooser), + GTK_WINDOW (gtk_widget_get_toplevel (GTK_WIDGET (gtk_builder_get_object (main_builder, "region_notebook"))))); This is pretty gross. We could do with using a "WID" like macro here. @@ +786,3 @@ + G_CALLBACK (filter_clear), NULL); + + selection = No line feeds here. @@ +797,3 @@ + G_CALLBACK (row_activated), builder); + + filtered_model = No need for a line feed. @@ +831,3 @@ + return gtk_tree_selection_get_selected (selection, model, iter); +} +/* Epilogue {{{1 */ Hmm?
Review of attachment 214116 [details] [review]: ::: panels/region/gnome-region-panel-input.c @@ +539,3 @@ -1); + kbd_viewer_args = g_strdup_printf ("gkbd-keyboard-display -l %s", layout); That could probably be a separate commit which we could apply right now. ::: panels/region/gnome-region-panel-system.c @@ +230,3 @@ } +#if 0 #if 0?
Review of attachment 214117 [details] [review]: That should be a separate patch, which we could also apply right now.
(In reply to comment #35) > > setting up a few keymaps should be too much overhead > As it was pointed out by Owen, the question is not the keymap change itself - > but the circles on the water... The apps that listen to that event and perform > some expensive actions. Ok, let's believe it is cheap enough. If it turns out apps do very expensive things in response, we'll have to fix them. > Regarding the options - I am not talking about options here (which is another > sad story for XKB lovers). I am talking about layouts and variants. They are > going to be presented in the user interface anyway, the stuff from base.xml. > All I am asking is keeping the ability to show 200 items instead of 100, adding > base.extras.xml Ah, ok, that sounds a lot more reasonable. Sorry for jumping the gun on options. I have no particular problem with optionally showing more exotic variants in the add-a-language list. We have search and filtering in that dialog, so it is not too painful to find what you need even if the list is large.
Review of attachment 214118 [details] [review]: Need to separate that patch. ::: panels/region/gnome-region-panel.ui @@ +170,3 @@ <property name="use_action_appearance">False</property> <property name="visible">True</property> + <property name="label" translatable="yes">Add Language</property> Adding labels to toolbar items should be a separate patch.
Review of attachment 214119 [details] [review]: Is the symbolic icon still broken? Please change the commit message if so. Are we just adding some buttons or replacing them? Not clear to me.
Review of attachment 214120 [details] [review]: Shouldn't be there in the first place. Merge it with the original patch please.
Review of attachment 214121 [details] [review]: That needs changing, as that gsettings-desktop-schemas patch has been rejected. Best ask the designers where in the shortcuts tree the shortcuts should be.
Review of attachment 214122 [details] [review]: More details in what's improved in the commit message? Something to test for would be useful.
Review of attachment 214124 [details] [review]: I'd like the first commit to include bug free code :) Needs to be merged into an earlier patch.
Review of attachment 214125 [details] [review]: Add the xkb-rules-db.c files separately, and add the code to update it automatically from gnome-settings-daemon (if we're not going with that code in gnome-desktop, see panels/wacom/Makefile.am panel for how to do this). ::: panels/region/gnome-region-panel-input-chooser.ui @@ -4,3 @@ <object class="GtkListStore" id="input_source_model"> <columns> - <!-- column-name id --> Why are you removing this interesting information? ::: panels/region/gnome-region-panel-input.c @@ -28,3 @@ -#include <ibus.h> -/* http://code.google.com/p/ibus/issues/detail?id=1338 */ You're removing this code which we only just added. @@ +63,3 @@ +static const InputSource ibus_sources[] = { + { "Chinese (Bopomofo)", "般", "us", "", "bopomofo" }, Translations?
A few more generic comments: - I'd rather you merged the bug fix patches into the original patch if really required, as I'm not too interested in how you got to the final code, but rather what gets merged (for example, the ibus specific hack in the includes from comment 46). Make yourself the author of the patch and write "with original code from Matthias" or something. - Prefix all the commit subject lines with the panel name, in lower case. - Split out what is applicable right now, so we can try and merge as much code as possible in 3.6, even if we don't end up merging the IBus support.
(In reply to comment #47) > A few more generic comments: > - I'd rather you merged the bug fix patches into the original patch if really > required, as I'm not too interested in how you got to the final code, but > rather what gets merged (for example, the ibus specific hack in the includes > from comment 46). Make yourself the author of the patch and write "with > original code from Matthias" or something. > - Prefix all the commit subject lines with the panel name, in lower case. > - Split out what is applicable right now, so we can try and merge as much code > as possible in 3.6, even if we don't end up merging the IBus support. I'm pretty sure that we want to merge the ibus support in 3.6... but sure, splitting out things for immediate application is a good idea.
Created attachment 214701 [details] [review] keyboard: Make it possible to jump to shortcuts tab -- I rebased this to the bottom of the patch stack so it applies cleanly on master.
Created attachment 214706 [details] [review] region: Add an initial input sources tab This is just the scaffolding according to https://live.gnome.org/Design/SystemSettings/RegionAndLanguage No code behind it yet. Original code from Matthias Clasen. -- This consolidates several of the previous patches. I also cleaned it up a bit.
Created attachment 214707 [details] [review] region: Removal of the Layouts tab -- Rebased on consolidated parts of other patches into one big removal patch.
Created attachment 214708 [details] [review] region: Improved selection handling on the input chooser This makes the input chooser list always have a selected row and be centered on it when the filter is applied.
Created attachment 214709 [details] [review] region: Improved keynav on the input chooser This makes the dialog return when the user presses Enter on the filter entry and prevents the GtkTreeView search popup from being used since we already handle searching on that tree view.
Created attachment 214710 [details] [review] region: Add XKB input sources We allow the user to choose from all XKB layouts installed and keep a user picked list in gsettings.
Created attachment 214711 [details] [review] region: Add IBus input sources We query IBus for the available engines and present them alongside XKB layouts.
(In reply to comment #55) > Created an attachment (id=214711) [details] [review] > region: Add IBus input sources > > We query IBus for the available engines and present them alongside XKB > layouts. Some notes about this one: 1) I'm not showing all IBus engines that might be available, that's basically because all the xkb* and m17n* ones add a lot of noise and even more redundancy to the chooser list. I'll discuss with the designers and the IBus developers how best to choose which engines to provide. 2) The IBus calls can be made async and I'm not trying to reconnect if IBus goes away.
Review of attachment 214701 [details] [review]: It applies, but does it compile? :) ::: panels/keyboard/cc-keyboard-panel.c @@ +137,3 @@ +{ + keyboard_general_dispose (CC_PANEL (object)); + keyboard_shortcuts_dispose (CC_PANEL (object)); Pretty certain that doesn't work.
(In reply to comment #57) > ::: panels/keyboard/cc-keyboard-panel.c > @@ +137,3 @@ > +{ > + keyboard_general_dispose (CC_PANEL (object)); > + keyboard_shortcuts_dispose (CC_PANEL (object)); > > Pretty certain that doesn't work. It does compile and run here with latest git master.
Created attachment 214900 [details] [review] region: Add XKB input sources -- Updated for changes in the schema and GnomeXkbInfo API.
Created attachment 214901 [details] [review] region: Add IBus input sources -- Updated to changes in the schema. The whole patch stack should be good to review again. Thanks.
Created attachment 215175 [details] [review] keyboard: Add key bindings to switch input sources
Created attachment 215372 [details] [review] region: Add XKB input sources -- Updated for schema changes.
Created attachment 215373 [details] [review] keyboard: Add key bindings to switch input sources -- Updated for schema changes.
Created attachment 215374 [details] [review] region: Add IBus input sources -- Updated for schema changes.
Rui, sorry, I am a bit confused - I do not see if/when you allow showing "extras". Please enlighten
Created attachment 215458 [details] [review] region: Update the shortcuts labels on startup
The keyboard shortcuts portion will have to be revisited, as the sections don't match the design: https://live.gnome.org/Design/SystemSettings/Keyboard Attachment 214701 [details] pushed as e59dc8d - keyboard: Make it possible to jump to shortcuts tab Attachment 214706 [details] pushed as bfaeb6a - region: Add an initial input sources tab Attachment 214707 [details] pushed as 0fb0d61 - region: Removal of the Layouts tab Attachment 214708 [details] pushed as e38f9ac - region: Improved selection handling on the input chooser Attachment 214709 [details] pushed as 375bf23 - region: Improved keynav on the input chooser Attachment 215372 [details] pushed as 0a78c35 - region: Add XKB input sources Attachment 215373 [details] pushed as 5e4376c - keyboard: Add key bindings to switch input sources Attachment 215458 [details] pushed as e880784 - region: Update the shortcuts labels on startup
*** Bug 654617 has been marked as a duplicate of this bug. ***
Review of attachment 215374 [details] [review]: The patch looks ok for what it does. A few things I noticed while playing with it: 1) My initial work had names like 'Japanese' or 'Japanese (Anthy)' for input methods, now you seem to just show 'Anthy' - is that intentional ? 2) Why do we have a small whitelist of input methods to show ? Wouldn't it be enough to filter out xkb / m17n stuff ? 3) I notice that as I rearrange the list using the up and down buttons, the currently selected input method changes. We should probably update the 'current' index in parallel to avoid that. 4) The 'settings' button never does anything - we should make it insensitive for now. 5) We never free the ibus and ibus_engines hash table. Doing that when the panel is unloaded would be nice, but is probably an independent cleanup.
Created attachment 217794 [details] [review] region: Fix a couple of memory leaks Unref the GSettings object and build the GnomeXkbInfo only once. There's no need to free and keep rebuilding the latter since it doesn't keep any state and is a bit expensive to build. -- (In reply to comment #69) > 5) We never free the ibus and ibus_engines hash table. Doing that when the > panel is unloaded would be nice, but is probably an independent cleanup. Yes, and the settings and xkb info objects were also being leaked. This patch fixes these. The IBus fix is squashed into the main IBus patch coming next. But note that I'm not freeing them there since I don't think it's worth it to do so on a usually short lived app like g-c-c and to not slow down further visits to this tab on the process' life time.
Created attachment 217795 [details] [review] region: Try to keep the current input source when modifying the list When modifying the input sources list the currently active source's index might change. We must change the current setting accordingly to keep it active. -- (In reply to comment #69) > 3) I notice that as I rearrange the list using the up and down buttons, the > currently selected input method changes. We should probably update the > 'current' index in parallel to avoid that. Should be fixed here.
Created attachment 217796 [details] [review] region: Add IBus input sources -- Rebased and fixed the memory leaks. (In reply to comment #69) > 1) My initial work had names like 'Japanese' or 'Japanese (Anthy)' for input > methods, now you seem to just show 'Anthy' - is that intentional ? > > 2) Why do we have a small whitelist of input methods to show ? Wouldn't it be > enough to filter out xkb / m17n stuff ? Both issues are related and it is intentional for now since I still haven't discussed how this list should be presented in detail with Allan Day. I'll do so ASAP.
Created attachment 217797 [details] [review] region: Wire up the input source settings button For XKB input sources the settings button remains unsensitive. For IBus sources we make it sensitive and launch the engine's setup tool on clicked if there is one. -- (In reply to comment #69) > 4) The 'settings' button never does anything - we should make it insensitive > for now. Yup, here it is. I'll discuss with the IBus folks if there's a better way to launch their engine settings tools (as hinted in a FIXME in this patch). Thanks for the comments!
Review of attachment 217795 [details] [review]: Haven't reviewed the code in detail, but it works as expected in my testing
Review of attachment 217794 [details] [review]: Looks good to me
Review of attachment 217797 [details] [review]: Playing with this, I couldn't find any ibus engine that actually gave me a 'setup_path'. So the button never turned sensitive :-(
One more observation from playing with this: All the whitelisted ibus engines show up with their 'method name' in the list - except for hangul, which shows up as 'Korean'. That is unfortunate, since there is a 'Korean' coming from xkb as well...
Created attachment 218129 [details] [review] region: Add IBus input sources -- (In reply to comment #78) > One more observation from playing with this: All the whitelisted ibus engines > show up with their 'method name' in the list - except for hangul, which shows > up as 'Korean'. That is unfortunate, since there is a 'Korean' coming from xkb > as well... I've modified the patch to display names like "Chinese (Pinyin)" but the korean engine ofcourse shows up as "Korean (Korean)". I'll try to poke the upstream maintainer. But really, it's because of this kind of stuff that my initial approach was to have a white list with all the meta info on our side so that this kind of iconsistencies wouldn't show up on the UI...
Created attachment 218130 [details] [review] region: Wire up the input source settings button -- (In reply to comment #77) > Playing with this, I couldn't find any ibus engine that actually gave me a > 'setup_path'. > So the button never turned sensitive :-( Right, I've modified the approach and am now requiring .desktop files to turn the button sensitive and launch the setup apps. I'm filing patches upstream for them. The Korean engine already has one so you can test with that one for now.
Created attachment 218482 [details] [review] region: Try to keep the current input source when modifying the list -- Added a g_settings_delay/apply pair to make the change atomic.
Created attachment 218483 [details] [review] region: Add IBus input sources -- Use ibus_bus_new_async() instead so that we get ibus activated if it's not running yet and don't block the UI.
Created attachment 218484 [details] [review] region: Wire up the input source settings button -- Rebased.
Review of attachment 218482 [details] [review]: ::: panels/region/gnome-region-panel-input.c @@ +198,1 @@ + g_settings_delay (input_sources_settings); Calling g_settings_delay() means that you will need to call g_settings_apply() for any _set() calls on the GSettings object. Have you reviewed the other users for that? I see uses in update_configuration().
Review of attachment 218483 [details] [review]: ::: configure.ac @@ +243,3 @@ USER_ACCOUNTS_PANEL_LIBS="$USER_ACCOUNTS_PANEL_LIBS $KRB5_LIBS" +# IBus support This is the wrong way to use pkg-config. Check for iBus being requested before the "PKG_CHECK_MODULES(REGION_PANEL,..." line, and set IBUS_MODULE to contain either "ibus-1.0" or be empty. Add $IBUS_MODULE to the REGION_PANEL pkg-config check. (And I know that other panels don't do it that way, be the first to do it properly!) ::: panels/region/Makefile.am @@ +30,3 @@ +libregion_la_LIBADD = \ + $(IBUS_LIBS) \ After the configure.ac changes, this change shouldn't be necessary. ::: panels/region/gnome-region-panel-input.c @@ +70,3 @@ + NULL +}; +#endif /* HAVE_IBUS */ @@ +112,3 @@ + +static gchar * +engine_display_name (IBusEngineDesc *engine_desc) engine_get_display_name().
Review of attachment 218484 [details] [review]: ::: panels/region/gnome-region-panel-input.c @@ +462,3 @@ + index = -1; + + settings_sensitive = (index >= 0 && setup_app_info_from_model_iter (model, &iter) != NULL); That's heavy handed. Why don't you add the name of the .desktop file to the treeview's model instead? Then only create the app info when you need it.
Created attachment 218603 [details] [review] region: Add IBus input sources -- Modified the approach of launching and keeping track of ibus according to https://bugzilla.gnome.org/show_bug.cgi?id=676102#c54 . Addressed Bastien's comments.
(In reply to comment #84) > Review of attachment 218482 [details] [review]: > > ::: panels/region/gnome-region-panel-input.c > @@ +198,1 @@ > + g_settings_delay (input_sources_settings); > > Calling g_settings_delay() means that you will need to call g_settings_apply() > for any _set() calls on the GSettings object. > Have you reviewed the other users for that? > > I see uses in update_configuration(). The _apply() is there at the end of that same function.
(In reply to comment #86) > Review of attachment 218484 [details] [review]: > > ::: panels/region/gnome-region-panel-input.c > @@ +462,3 @@ > + index = -1; > + > + settings_sensitive = (index >= 0 && setup_app_info_from_model_iter (model, > &iter) != NULL); > > That's heavy handed. Why don't you add the name of the .desktop file to the > treeview's model instead? Then only create the app info when you need it. I don't do that because the desktop file might not exist and I don't want to set the button sensitive in that case.
(In reply to comment #89) > (In reply to comment #86) > > Review of attachment 218484 [details] [review] [details]: > > > > ::: panels/region/gnome-region-panel-input.c > > @@ +462,3 @@ > > + index = -1; > > + > > + settings_sensitive = (index >= 0 && setup_app_info_from_model_iter (model, > > &iter) != NULL); > > > > That's heavy handed. Why don't you add the name of the .desktop file to the > > treeview's model instead? Then only create the app info when you need it. > > I don't do that because the desktop file might not exist and I don't want to > set the button sensitive in that case. Then store the app itself in the tree. That piece of code is pretty ugly, it should be straight forward.
Review of attachment 217794 [details] [review]: .
Review of attachment 218603 [details] [review]: ::: panels/region/gnome-region-panel-input.c @@ +136,3 @@ +{ + /* DBus activate it if the well known name isn't there. */ + g_bus_unwatch_name (g_bus_watch_name (G_BUS_TYPE_SESSION, I want a big warning here. @@ +778,3 @@ + ibus = ibus_bus_new_async (); + g_signal_connect_swapped (ibus, "connected", + G_CALLBACK (fetch_ibus_engines), builder); I'm pretty sure this code is racy. How do you populate the tree model with the IM engine's real name if you haven't had the chance to populate the ibus_engines hash table?
For the record: Rui and me had a chat today. Since libgnomekbd is used only for the keyboard layout drawing, the code has to be stripped and perhaps merged to g-c-c. I am happy to pass the maintainership of that code to Rui.
(In reply to comment #92) > ::: panels/region/gnome-region-panel-input.c > @@ +136,3 @@ > +{ > + /* DBus activate it if the well known name isn't there. */ > + g_bus_unwatch_name (g_bus_watch_name (G_BUS_TYPE_SESSION, > > I want a big warning here. Sure. > @@ +778,3 @@ > + ibus = ibus_bus_new_async (); > + g_signal_connect_swapped (ibus, "connected", > + G_CALLBACK (fetch_ibus_engines), builder); > > I'm pretty sure this code is racy. How do you populate the tree model with the > IM engine's real name if you haven't had the chance to populate the > ibus_engines hash table? Here's what happens: we go on at first and populate the tree model at first with only XKB sources since we don't have the hash table yet. Then when we get IBusBus::connected and finally have the engines descriptions, the sources list is populated again. So, I don't think there's any race, it's just not super efficient but for that we'd need to block waiting for the ibus engines descriptions.
(In reply to comment #94) > (In reply to comment #92) <snip> > > I'm pretty sure this code is racy. How do you populate the tree model with the > > IM engine's real name if you haven't had the chance to populate the > > ibus_engines hash table? > > Here's what happens: we go on at first and populate the tree model at first > with only XKB sources since we don't have the hash table yet. Then when we get > IBusBus::connected and finally have the engines descriptions, the sources list > is populated again. So, I don't think there's any race, it's just not super > efficient but for that we'd need to block waiting for the ibus engines > descriptions. That's pretty bad. That means that you go through the list of XKB items twice, and that if you're too quick and make changes to the list whilst the ibus engines are loading, you've lost everything. Add everything in the list to the treeview, and resolve the nice names when you get the full list of engines from iBus.
Created attachment 218786 [details] [review] region: Add IBus input sources -- (In reply to comment #95) > > Here's what happens: we go on at first and populate the tree model at first > > with only XKB sources since we don't have the hash table yet. Then when we get > > IBusBus::connected and finally have the engines descriptions, the sources list > > is populated again. So, I don't think there's any race, it's just not super > > efficient but for that we'd need to block waiting for the ibus engines > > descriptions. > > That's pretty bad. That means that you go through the list of XKB items twice, > and that if you're too quick and make changes to the list whilst the ibus > engines are loading, you've lost everything. You're right. We wouldn't lose the XKB configured sources but the IBus ones yes, they'd be lost from the setting list. > Add everything in the list to the treeview, and resolve the nice names when you > get the full list of engines from iBus. Done here. BTW, I uncovered a gtk+ bug doing this since I'm now using a filter model to hide the unresolved entries. It prevents re-ordering the 2nd row into the 1st. See bug 679910.
Created attachment 218787 [details] [review] region: Wire up the input source settings button -- (In reply to comment #90) > > > That's heavy handed. Why don't you add the name of the .desktop file to the > > > treeview's model instead? Then only create the app info when you need it. > > > > I don't do that because the desktop file might not exist and I don't want to > > set the button sensitive in that case. > > Then store the app itself in the tree. That piece of code is pretty ugly, it > should be straight forward. Done this now. (I'm a bit embarassed that I wasn't being able to stuff NULL into tree model columns of type G_TYPE_OBJECT. I was getting a crash deep inside gtk_tree_model_get() but is was because of something else and not the NULLs...)
Review of attachment 218482 [details] [review]: Is this patch still needs-work?
Created attachment 218809 [details] [review] region: Add IBus input sources -- I intended to make the autoconf bits the same as in g-s-d but forgot. Done now.
Created attachment 218810 [details] [review] region: Wire up the input source settings button -- Rebased and fixed a minor code style issue.
Attachment 218809 [details] pushed as 412e530 - region: Add IBus input sources Attachment 218810 [details] pushed as 5e73790 - region: Wire up the input source settings button
Attachment 217794 [details] pushed as 49f7d37 - region: Fix a couple of memory leaks Attachment 218482 [details] pushed as 3579d7b - region: Try to keep the current input source when modifying the list I also fixed the "delayed-mode" usage, it's not a pair of delay/apply, delay applies to the settings object and is irreversible.