Also see bug 682318

Created attachment 223599 [details] [review]
status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode

IBus has a properties API which are basically generic knobs into the
engine which are serialized and presented in a way that allows us to
easily build actionable UI elements with them.

Instead of implementing the whole generic system and accepting
everything coming out of the engines, for now, this patch just adds
support for a couple of important properties of IBus Anthy.

--

There's opportunity here to make this code more generic and splitting
things into different classes, but for now I didn't want to make it
too sophisticated just yet. At least until the need arises to do this
for other engines and we decide if we want to stay with a properties
white list or trust (or fix?) the engines.

(In reply to comment #2)
> Created an attachment (id=223599) [details] [review]
> status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode
> 
> IBus has a properties API which are basically generic knobs into the
> engine which are serialized and presented in a way that allows us to
> easily build actionable UI elements with them.

I'm just saying a thing here, but this sounds like it's an ad-hoc duplicate of GMenuModel.

TypingMode is nice to have but DictMode is required.

Rui, do you have a screenshot ?

Created attachment 223772 [details]
With submenus closed.

Created attachment 223773 [details]
With submenus open.

Created attachment 223861 [details] [review]
status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode

--
(In reply to comment #4)
> TypingMode is nice to have but DictMode is required.

Ok, here it is.

Review of attachment 223861 [details] [review]:

::: js/ui/status/keyboard.js
@@ +124,3 @@
                                                          object_path: IBus.PATH_PANEL });
             this._candidatePopup.setPanelService(this._panelService);
+            // Need to set this to get 'global-engine-changed' emitions

emissions

Review of attachment 223861 [details] [review]:

::: js/ui/status/keyboard.js
@@ +46,3 @@
+        'DictMode.embedded': true,
+        'DictMode.anthy_zipcode': true,
+        'DictMode.ibus_oldchar': true

They would be the system dictionaries but users can register their dictionaries with ibus-setup-anthy so DictMode.* needs to be shown.

Some observations from looking at the screenshot:

- Seems that we are using the overall 'anthy' icon for the InputMode menu. Does that add confusion, when there are multiple submenus that all 'belong to' anthy ?

- I must say the a menuitem labeled 'R' looks broken to my Western eyes... is this strongly expected, or can we come up with a more meaningful label ? Like, say, 'Typing mode' ?

- Do the strings in the menus come directly from anthy ? They're not perfect. I think 'Katakana' should probably be capitalized throughout, and 'Half width' should probably by 'Half-width'.

Created attachment 223903 [details]
Katakana input mode

(In reply to comment #11)
> - Seems that we are using the overall 'anthy' icon for the InputMode menu. Does
> that add confusion, when there are multiple submenus that all 'belong to' anthy
> ?

Right, actually that's not all. The Input Mode actually changes the panel button icon and the menu entry symbol as you can see in this screenshot. I did it that way because that's how the current IBus does it but I'm not sure it's a good idea.

> - I must say the a menuitem labeled 'R' looks broken to my Western eyes... is
> this strongly expected, or can we come up with a more meaningful label ? Like,
> say, 'Typing mode' ?

I really can't answer this one.

> - Do the strings in the menus come directly from anthy ? They're not perfect. I
> think 'Katakana' should probably be capitalized throughout, and 'Half width'
> should probably by 'Half-width'.

Yes, the strings come from IBus and one thing I've noticed is that they aren't translated, even when I run everything in the Japanese locale. Takao, are these strings even marked for translation?

(In reply to comment #11)
> Some observations from looking at the screenshot:
> 
> - Seems that we are using the overall 'anthy' icon for the InputMode menu. Does
> that add confusion, when there are multiple submenus that all 'belong to' anthy
> ?

Actually the behavior is compatible between ibus panel menu and ibus language-bar.
The panel menu label and language-bar button label are same.
Probably I think the solution might be to add a new member in IBusProperty to show the long description in the menu label.

> 
> - I must say the a menuitem labeled 'R' looks broken to my Western eyes... is
> this strongly expected, or can we come up with a more meaningful label ? Like,
> say, 'Typing mode' ?

This also would be the same reason above.

> 
> - Do the strings in the menus come directly from anthy ? They're not perfect. I
> think 'Katakana' should probably be capitalized throughout, and 'Half width'
> should probably by 'Half-width'.

Probably I agree with your suggestion. I can fix those in ibus-anthy if you provides a list or a bug.

I've filed 

http://code.google.com/p/ibus/issues/detail?id=1508

for the string issues

Today I talked with ibus upstream. Probably it's a good time to define IBusProperty.label as the long text.
And probably the short text is moved to a new memeber in IBusProperty.
I will create the patch in ibus side tomorrow and I also think a change also will be needed in gnome-shell to show the icon char on keyboard indicator icon.

Created attachment 224209 [details]
Screenshot of anthy Input mode menu

I'm changing the UI with the attachment.
The menu label will be "Input mode (a char)" and the panel icon will be "a char".
The menu label is shared between the shell and gtk.

That looks better to me, thanks

Created attachment 224339 [details] [review]
status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode

--

* Changed the white list handling to address Takao's comment about
  accepting user defined DictMode entries

* Added support for the new IBusProperty.symbol but falling back on
  using the label if running with an older IBus

can this get reviewed, so that we have a chance to land it before .92 ?

(In reply to comment #18)
> Created an attachment (id=224339) [details] [review]
> status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode

The patch works fine with me.

Review of attachment 224339 [details] [review]:

I don't like how you pass a submenu to a backend object, which is then responsible for building UI. Is it really impossible to split this properly, so that IBusManager emits a 'properties-updated' signal when appropriate (with the whitelist-filtered properties as parameter) and the InputSourceIndicator builds the UI? You'd need some kind of IBusManager.setPropertyState(prop, state) method to use in the items' activate handler, but it still looks like a cleaner split to me.

Created attachment 224522 [details] [review]
status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode

--

This is cleaner indeed. I didn't do the props list filtering on the
backend side though since there's no real benefit AFAICT and it would
actually be more expensive since we'd have to create a new list.

Review of attachment 224522 [details] [review]:

(In reply to comment #22)
> This is cleaner indeed. I didn't do the props list filtering on the
> backend side though since there's no real benefit AFAICT and it would
> actually be more expensive since we'd have to create a new list.

I don't really care where the filtering is done, as long as it's either the backend or the UI. In the current patch both do filtering and (worse) rely on the other one to apply the same filtering - not a big fan of that.

Created attachment 224547 [details] [review]
status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode

--

Ok, I think it's better now.

Review of attachment 224522 [details] [review]:

Actually I found a bug right now - if you select 'Anthy', everything works as expected (as far as I can tell as non-Anthy user). The extra section doesn't appear though when 'Anthy' is already selected when the shell comes up ...

(In reply to comment #25)
> Actually I found a bug right now - if you select 'Anthy', everything works as
> expected (as far as I can tell as non-Anthy user). The extra section doesn't
> appear though when 'Anthy' is already selected when the shell comes up ...

Ah yes, that's actually a problem with ibus because there's no way to ask for the properties. They fixed it by sending the properties when a PanelService connects but you'll have to get the latest from upstream.

Even then, there's still a small problem that I'm trying to fix which is that the property entries are there but the separator above them isn't.

(In reply to comment #26)
> Even then, there's still a small problem that I'm trying to fix which is that
> the property entries are there but the separator above them isn't.

So the fix that was done in ibus isn't enough it seems. We might be getting sent the properties but since the PanelService object connects to ibus on object creation we can't connect to its 'register-properties' signal before the connection is made and thus we end up losing the first register properties that should be triggered when the PanelService connects. I'll discuss with ibus upstream about this.

Anyway, this doesn't seem like a big problem to me since on a regular session start there is no application window yet and whenever an IM using widget gets focused we'll get the properties.

Actually, it's quite unfortunate how the properties work currently, ibus basically presents them when an IM context gets focus and removes them on focus out which actually makes it a bit hard to have those items in the menu in the 1st place since obviously the IM context loses focus when you click the panel menu button to bring down the menu. If you're wondering why the connect/disconnect dance to the 'register-properties' signal in _resetProperties() this is why.

Well, something like the following actually makes the separator behave properly. Want me to squash it or is it too ugly going behind the regular separator hide/show rules like this?

diff --git a/js/ui/status/keyboard.js b/js/ui/status/keyboard.js
index c7a4c3a..5f2e114 100644
--- a/js/ui/status/keyboard.js
+++ b/js/ui/status/keyboard.js
@@ -219,7 +219,8 @@ const InputSourceIndicator = new Lang.Class({
         this._currentSourceIndex = this._settings.get_uint(KEY_CURRENT_INPUT_SOURCE);
         this._xkbInfo = new GnomeDesktop.XkbInfo();
 
-        this.menu.addMenuItem(new PopupMenu.PopupSeparatorMenuItem());
+        this._propSeparator = new PopupMenu.PopupSeparatorMenuItem();
+        this.menu.addMenuItem(this._propSeparator);
         this._propSection = new PopupMenu.PopupMenuSection();
         this.menu.addMenuItem(this._propSection);
         this._propSection.actor.hide();
@@ -445,13 +446,16 @@ const InputSourceIndicator = new Lang.Class({
     },
 
     _buildPropSection: function() {
+        this._propSeparator.actor.hide();
         this._propSection.actor.hide();
         this._propSection.removeAll();
 
         this._buildPropSubMenu(this._propSection, this._properties);
 
-        if (!this._propSection.isEmpty())
+        if (!this._propSection.isEmpty()) {
             this._propSection.actor.show();
+            this._propSeparator.actor.show();
+        }
     },
 
     _buildPropSubMenu: function(menu, props) {

(In reply to comment #27)
> So the fix that was done in ibus isn't enough it seems. [...] I'll discuss with ibus
> upstream about this.

OK.


> Actually, it's quite unfortunate how the properties work currently, ibus
> basically presents them when an IM context gets focus and removes them on focus
> out which actually makes it a bit hard to have those items in the menu in the
> 1st place since obviously the IM context loses focus when you click the panel
> menu button to bring down the menu. If you're wondering why the
> connect/disconnect dance to the 'register-properties' signal in
> _resetProperties() this is why.

Yeah, I was afraid to ask given previous IBus API insanities ;-)

Review of attachment 224547 [details] [review]:

Minor comment left, otherwise looks good to go

::: js/ui/status/keyboard.js
@@ +153,3 @@
+    },
+
+    propertyWhitelisted: function(prop) {

Is there a good reason why this is not a private method on InputSourceIndicator?

(In reply to comment #28)
> Well, something like the following actually makes the separator behave
> properly. Want me to squash it or is it too ugly going behind the regular
> separator hide/show rules like this?

I haven't seen the issue, but apparently there's a bug in popupMenu's separator code which should be fixed. The code looks OK to me as a temporary hack, but I think it's better as a separate commit so it can be reverted when the real problem is fixed.

Created attachment 224602 [details] [review]
status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode

--

(In reply to comment #30)
> ::: js/ui/status/keyboard.js
> @@ +153,3 @@
> +    },
> +
> +    propertyWhitelisted: function(prop) {
>
> Is there a good reason why this is not a private method on
> InputSourceIndicator?

No reason, thanks for remind me.

Created attachment 224603 [details] [review]
status/keyboard: Fix hidden separator when menu changes while open

Due to the way the IBus API works we might get property changes while
the menu is already open. In that case the separator visibility logic
doesn't work since it only applies on menu open/close. This works
around that issue.

Review of attachment 224602 [details] [review]:

Yup - while the UI impact is rather small IMHO (e.g. only users of a particular IM engine are affected), it's still a freeze break, so don't push right away :)

Review of attachment 224603 [details] [review]:

Mmmh, have you tried something like
   this.menu.emit('open-state-changed', true); // trick PopupMenu into updating separator visibility

In any case, the code is fine as-is

Attachment 224602 [details] pushed as 9659d93 - status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode
Attachment 224603 [details] pushed as 5c7da4b - status/keyboard: Fix hidden separator when menu changes while open