GNOME Bugzilla – Bug 682318
input sources: add more engine options to keyboard status menu
Last modified: 2013-01-28 15:34:35 UTC
The old ibus shell extension offered a ton of options in its menu - we probably don't want to go back to all of it, but maybe some options will be useful to have in the menu. We discussed this in the input sources bof at guadec and decided that for 3.6, the priority should be to just add hiragana/katakana switching to the menu, nothing else. See bug 682314
*** Bug 688916 has been marked as a duplicate of this bug. ***
Uhm, so the main problem was just that the code was not there? Ok, here it is then!
Created attachment 229762 [details] [review] Keyboard: implement remaining property types Input methods other than Anthy need different property types, so we should support all those exposed by the IBus protocol.
Created attachment 229763 [details] [review] Keyboard: remove the property whitelist It was just a stopgap solution for 3.6, as the necessary UI bits were not implemented.
Xiaojun Ma has reported effect of this bug on Chinese input method. https://bugzilla.gnome.org/show_bug.cgi?id=688916 These mode or keyboard status is critical to Chinese users -- we keep switching them back and forth. He made a video of this as well. http://dl.dropbox.com/u/45139465/gnome36.webm http://dl.dropbox.com/u/45139465/gnome34.webm My investigation of this is it seems that input source is still able to respond to the mode switching shortcut, but not able to show it on the menu.
(In reply to comment #2) > Uhm, so the main problem was just that the code was not there? > Ok, here it is then! Not just that. It is also that we don't want to clutter up the keyboard indicator beyond recognition. Design is still needed to sort out which options absolutely have to be in the indicator, and which are better off elsewhere. Just dumping whatever comes out of ibus into the ui is not the best way forward.
> Not just that. It is also that we don't want to clutter up the keyboard > indicator beyond recognition. Design is still needed to sort out which options > absolutely have to be in the indicator, and which are better off elsewhere. Then what? Have you ever contacted engine upstreams? Show me full links, please. It's the engine authors not you that maintain the engines and thus the input experience under GNOME. If your design idea doesn't break input experience of upstream engine, why don't they accept your idea? > Just dumping whatever comes out of ibus into the ui is not the best way > forward. You are still uninformed of IBus history. Now dropped language panel mode is more popular and I'm still using due to a long-lasting Ubuntu bug. Now that you changed the UI container, you didn't inform engine upstreams explicitly, then you claim that they are low-quality and need censorship from non-IME (I'm sorry if I'm wrong here) users like you? It's natural that people are not following your track currently. IBus 1.4.99 is a pre-release version. GNOME 3.6 with IBus integration is only available in pre-release Fedora 18. (Arch had it for a while and then disabled it.)
(In reply to comment #6) > (In reply to comment #2) > > Uhm, so the main problem was just that the code was not there? > > Ok, here it is then! > > Not just that. It is also that we don't want to clutter up the keyboard > indicator beyond recognition. Design is still needed to sort out which options > absolutely have to be in the indicator, and which are better off elsewhere. > > Just dumping whatever comes out of ibus into the ui is not the best way > forward. Is it not? After all, we do that in places like the app menu or the app chooser, where we recognize the data source "knows better". In this case in particular I believe we should accept that not only the input method is the right technical place to improve the UI, but also, given the variety of languages, cultures and habits, the people behind the IME may be in a better position than gnome-shell developers to decide what to expose in the menu, and how. That is, I'm ok with reducing clutter, but it needs to be done in each supported IME by collaborating with upstreams, not with a downstream whitelist. Not to mention that, while it took me less than one minute to remove the whitelist because I knew exactly where to look, we should not require IM developers to be also shell devs just to test their code.
(In reply to comment #8) > (In reply to comment #6) > > (In reply to comment #2) > > > Uhm, so the main problem was just that the code was not there? > > > Ok, here it is then! > > > > Not just that. It is also that we don't want to clutter up the keyboard > > indicator beyond recognition. Design is still needed to sort out which options > > absolutely have to be in the indicator, and which are better off elsewhere. > > > > Just dumping whatever comes out of ibus into the ui is not the best way > > forward. > > Is it not? After all, we do that in places like the app menu or the app > chooser, where we recognize the data source "knows better". > In this case in particular I believe we should accept that not only the input > method is the right technical place to improve the UI, but also, given the > variety of languages, cultures and habits, the people behind the IME may be in > a better position than gnome-shell developers to decide what to expose in the > menu, and how. Yes, I agree. The concern about integration should be irrelevant here. Exposing the problems of existing ibus-engines will attract maintainers or other contributors to fix the exposed issues. Potentially that will make more engine developers work closely with GNOME. Whitelist out the unofficial engines is permanently saying good bye to them. Developers don't even get a chance to test and fix it! > That is, I'm ok with reducing clutter, but it needs to be done in each > supported IME by collaborating with upstreams, not with a downstream whitelist. Glad to hear that. :) > > Not to mention that, while it took me less than one minute to remove the > whitelist because I knew exactly where to look, we should not require IM > developers to be also shell devs just to test their code. Exactly. Things actually happen is even worse, the whitelist happened at g-c-c. And the g-c-c developers are holding a very conservative view on input engines. see the flame war on bug 688914.
(In reply to comment #4) > Created an attachment (id=229763) [details] [review] > Keyboard: remove the property whitelist > > It was just a stopgap solution for 3.6, as the necessary UI bits > were not implemented. I confirm that these 2 patches work flawlessly with ibus-pinyin backend. Push suggested.
(In reply to comment #7) > It's the engine authors not you that maintain the engines and thus the input > experience under GNOME. > If your design idea doesn't break input experience of upstream engine, why > don't they accept your idea? You can't slice and dice the user experience like that. It needs to be designed as a whole. Saying 'here, networkmanager guys, have this menu in the shell and just put there whatever you want' doesn't work either. That being said, I agree tha tthe whitelist approach is pretty crude and can only be a temporary stopgap measure.
> You can't slice and dice the user experience like that. It needs to be designed > as a whole. Saying 'here, networkmanager guys, have this menu in the shell and > just put there whatever you want' doesn't work either. I don't mind you have a working (this is more important, sir), consistent design and I can probably work on that. But where is it? Can you show me the full link? If you don't have yet, totally fine, but we need to expose what we already have now, otherwise it is *not* working. > That being said, I agree tha tthe whitelist approach is pretty crude and can > only be a temporary stopgap measure. Cool.
(In reply to comment #11) > (In reply to comment #7) > > > It's the engine authors not you that maintain the engines and thus the input > > experience under GNOME. > > If your design idea doesn't break input experience of upstream engine, why > > don't they accept your idea? > > You can't slice and dice the user experience like that. It needs to be designed > as a whole. Saying 'here, networkmanager guys, have this menu in the shell and > just put there whatever you want' doesn't work either. > > That being said, I agree tha tthe whitelist approach is pretty crude and can > only be a temporary stopgap measure. I think I can agree at some point we could temporarily use blacklist instead of whitelist. Modes menu are pretty flexible according to the engines. For instance in Pinyin, you could have shuangpin as a switch while in Wubi, you could have a switch called pinyin-wubi. And whether these mode exist or not, completely depend on engine's configuration, plugins enabled and current implemented features. For your concern, I would say, let's just leave it here right now. If any modes that are affecting user experiences, let's contact the engine developer blacklist these modes. In the long run, I think the way to make it work it's to provide a hint at ibus level, indicating this mode shouldn't show up in gnome-shell or how to show up in gnome-shell. Also, just for the record, Chinese ibus-pinyin doesn't offer any mode that are affecting user experience. I've tried. :)
Created attachment 230795 [details] [review] status/keyboard: Keep IBus properties per engine This creates proper objects to represent input sources which makes it easier to keep track of all the data we have about them and allows us to have different sets of properties for different IBus engines.
Created attachment 230796 [details] [review] status/keyboard: Limit the number of toplevel properties in the menu In order for the menu to not grow out of proportions we limit the number of toplevel IBus properties we display and move the extra ones under a sub-menu. -- This still needs design input as the menu label implies...
*** Bug 687900 has been marked as a duplicate of this bug. ***
Review of attachment 229762 [details] [review]: Please change the summary to "status/keyboard: ..." since we also have a js/ui/keyboard.js . Code is fine and will be a-c-n when we agree on the design.
Review of attachment 229763 [details] [review]: Please change the summary to "status/keyboard: ...". Likewise, looks good and will be a-c-n later.
Review of attachment 229762 [details] [review]: This is basically GMenuModel, hm.
Review of attachment 230795 [details] [review]: OK. ::: js/ui/status/keyboard.js @@ +212,3 @@ + }, + + init: function() { Any reason this is done in a separate step rather than at construct time? As far as I can tell it always calls this sometime after construction. @@ +233,3 @@ + this._inputSources = {}; // InputSource set indexed by InputSource.index + this._ibusSources = {}; // IBus InputSource set indexed by InputSource.id Better comments would be nice. this._inputSources = {}; // All input sources indexed by their index this._ibusSources = {}; // Input sources of type INPUT_SOURCE_TYPE_IBUS // indexed by the IBus ID. @@ +234,3 @@ + this._inputSources = {}; // InputSource set indexed by InputSource.index + this._ibusSources = {}; // IBus InputSource set indexed by InputSource.id + this._currentSourceIndex = 0; Can we lose this in favor of "this._currentSource"? It doesn't seem the index is used anywhere, except to lookup things. @@ +290,3 @@ this.actor.show(); + let currentSource = this._inputSources[this._currentSourceIndex]; let oldSource; [oldSource, this._currentSource] = [this._currentSource, newSource]; @@ +293,3 @@ + if (currentSource) { + currentSource.menuItem.setShowDot(false); + this._container.set_skip_paint(currentSource.indicatorLabel, true); oldSource, etc. @@ +489,3 @@ + if (prop.get_key() == 'InputMode') + this._updateIndicatorLabel(prop); Bad rebase? Separate commit, please.
Review of attachment 230796 [details] [review]: Are there any changes in the indented section? Put a comment in the commit message.
Created attachment 230825 [details] How things work in old days Having 6 entries in property menu wasn't a problem at all in old days. As you can see from the screen shot. Big fonts and big margin/padding used by GNOME Shell is the source of too many entries problem.
Created attachment 230980 [details] [review] status/keyboard: Keep IBus properties per engine -- (In reply to comment #20) > + init: function() { > > Any reason this is done in a separate step rather than at construct time? As > far as I can tell it always calls this sometime after construction. It's was just because shortName can still be changed after creation to disambiguate engines with the same shortName. But we can work around that with a set method. > Better comments would be nice. Done. > + this._currentSourceIndex = 0; > > Can we lose this in favor of "this._currentSource"? It doesn't seem the index > is used anywhere, except to lookup things. Sure. > + let currentSource = this._inputSources[this._currentSourceIndex]; > > let oldSource; > [oldSource, this._currentSource] = [this._currentSource, newSource]; Ok. > + if (prop.get_key() == 'InputMode') > + this._updateIndicatorLabel(prop); > > Bad rebase? Separate commit, please. Well it is tightly related with this patch and wouldn't work correctly otherwise because _updateIndicatorLabel() was calling IBusManager.hasProperties() which was hardcoding 'anthy'. Yes it was an ugly hack.
Created attachment 230981 [details] [review] status/keyboard: Limit the number of toplevel properties in the menu -- (In reply to comment #21) > Are there any changes in the indented section? Put a comment in the commit > message. I changed the condition so there's no need to indent anymore and actually makes the code clearer since it's easier to spot that we're breaking the loop here.
Review of attachment 230981 [details] [review]: ::: js/ui/status/keyboard.js @@ +506,3 @@ + extraProps.append(p); + + item = new PopupMenu.PopupSubMenuMenuItem(_("MOAR PROPs")); Excuse me?
Review of attachment 230980 [details] [review]: ::: js/ui/status/keyboard.js @@ +240,3 @@ + // KEY_INPUT_SOURCES list indexed by their index there. + this._inputSources = {}; + // Input sources of type INPUT_SOURCE_TYPE_IBUS indexed by the Do these have to be in gsettings too? If so, put a comment. If not, say so.
Sorry I did not notice this before, for some reason GMail decided bugzilla spam was real spam :) I updated the commit message, I'm not attaching the patches again as there are no functional changes. Thanks for the review.
Created attachment 231364 [details] [review] status/keyboard: Keep IBus properties per engine -- (In reply to comment #26) > + // KEY_INPUT_SOURCES list indexed by their index there. > + this._inputSources = {}; > + // Input sources of type INPUT_SOURCE_TYPE_IBUS indexed by the > > Do these have to be in gsettings too? If so, put a comment. If not, say so. Clarified this comment. I also added an 'activate' signal to the InputSource class which makes it easier to trigger them from the input sources switcher in bug 682315.
hmm, couldn't get this to apply to master - am I missing some other patches ?
Review of attachment 230981 [details] [review]: We are dropping this for now and instead will encourage the most commonly used engines to behave better where needed.
(In reply to comment #29) > hmm, couldn't get this to apply to master - am I missing some other patches ? Both Giovanni's patches and https://bugzilla.gnome.org/attachment.cgi?id=231364 apply cleanly on origin/master here. Maybe you had something else around?
Review of attachment 231364 [details] [review]: Sorry, but a lot of this is all mixed up. This should be at least three different patches.
Created attachment 231485 [details] [review] status/keyboard: Make input sources be proper objects Introduce an InputSource class which makes it easier to keep track of all the data we have about them.
Created attachment 231486 [details] [review] status/keyboard: Store IBus engine properties per InputSource object Instead of storing them globally and having an hardcoded list of engines which are allowed to change their indicator symbol when the InputMode property changes.
Comment on attachment 231364 [details] [review] status/keyboard: Keep IBus properties per engine (In reply to comment #32) > Review of attachment 231364 [details] [review]: > > Sorry, but a lot of this is all mixed up. This should be at least three > different patches. Ok, I've split it in two. Do you see a third patch in there?
Created attachment 231509 [details] [review] status/keyboard: Store IBus engine properties per InputSource object -- Simplified the patch by not moving code around.
Review of attachment 231485 [details] [review]: OK.
Review of attachment 231509 [details] [review]: OK.
Thanks for the reviews! Attachment 231485 [details] pushed as c41424b - status/keyboard: Make input sources be proper objects Attachment 231509 [details] pushed as 2008feb - status/keyboard: Store IBus engine properties per InputSource object
Question: will these status/options be shown without user have to click it? Because inputing Chinese needs to look at current status before typing. I remember in the old days, there is a status bar in ibus show the current status/options, not sure if that still exists right now.
(In reply to comment #40) > Question: will these status/options be shown without user have to click it? > Because inputing Chinese needs to look at current status before typing. > > I remember in the old days, there is a status bar in ibus show the current > status/options, not sure if that still exists right now. Can you provide a screenshot and a small explanation of that status bar? Right now there is a special case for a property named InputMode and we replace the regular engine symbol in the top panel with that property's symbol. Anthy uses this to display different symbols for Hiragana, Katakana, Half-width Katakana, Latin and Wide Latin modes. You could either use a property like that or we could discuss with other engine authors and the design people how to handle this generically for all engines.
Hi Mike Qin, By saying "status bar", do you mean something like this? "this": http://ibus.googlecode.com/svn/screenshots/3.png Its "official" name is "language panel".
(In reply to comment #42) > Hi Mike Qin, > By saying "status bar", do you mean something like this? > "this": http://ibus.googlecode.com/svn/screenshots/3.png > > Its "official" name is "language panel". Yes, that's exactly what I mean.