After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 682314 - input sources: add hiragana/katakana items to keyboard status menu
input sources: add hiragana/katakana items to keyboard status menu
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2012-08-21 01:09 UTC by Matthias Clasen
Modified: 2012-11-23 09:07 UTC
See Also:
GNOME target: 3.6
GNOME version: ---


Attachments
status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode (12.51 KB, patch)
2012-09-06 02:07 UTC, Rui Matos
none Details | Review
With submenus closed. (47.96 KB, image/png)
2012-09-07 16:23 UTC, Rui Matos
  Details
With submenus open. (59.37 KB, image/png)
2012-09-07 16:24 UTC, Rui Matos
  Details
status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode (12.58 KB, patch)
2012-09-09 19:26 UTC, Rui Matos
none Details | Review
Katakana input mode (40.27 KB, image/png)
2012-09-10 12:23 UTC, Rui Matos
  Details
Screenshot of anthy Input mode menu (29.61 KB, image/png)
2012-09-13 10:44 UTC, Takao Fujiwara
  Details
status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode (12.64 KB, patch)
2012-09-14 15:48 UTC, Rui Matos
reviewed Details | Review
status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode (13.05 KB, patch)
2012-09-17 15:07 UTC, Rui Matos
needs-work Details | Review
status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode (13.27 KB, patch)
2012-09-17 19:33 UTC, Rui Matos
reviewed Details | Review
status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode (13.25 KB, patch)
2012-09-18 10:26 UTC, Rui Matos
committed Details | Review
status/keyboard: Fix hidden separator when menu changes while open (1.85 KB, patch)
2012-09-18 10:26 UTC, Rui Matos
committed Details | Review

Description Matthias Clasen 2012-08-21 01:09:46 UTC
We've discussed the need for this in the input sources bof at guadec.

The agreement was that we should only add hiragana/katakana switching to the keyboard status menu for 3.6, not other options and actions that input methods might support, like 'add to dictionary' etc.

Changing between these modes should also update the status icon to reflect the current mode.
Comment 1 Matthias Clasen 2012-08-21 01:26:29 UTC
Also see bug 682318
Comment 2 Rui Matos 2012-09-06 02:07:40 UTC
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.
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-09-06 02:30:47 UTC
(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.
Comment 4 Takao Fujiwara 2012-09-07 07:14:55 UTC
TypingMode is nice to have but DictMode is required.
Comment 5 Matthias Clasen 2012-09-07 15:18:28 UTC
Rui, do you have a screenshot ?
Comment 6 Rui Matos 2012-09-07 16:23:48 UTC
Created attachment 223772 [details]
With submenus closed.
Comment 7 Rui Matos 2012-09-07 16:24:16 UTC
Created attachment 223773 [details]
With submenus open.
Comment 8 Rui Matos 2012-09-09 19:26:15 UTC
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.
Comment 9 Matthias Clasen 2012-09-09 23:20:53 UTC
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
Comment 10 Takao Fujiwara 2012-09-10 01:56:41 UTC
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.
Comment 11 Matthias Clasen 2012-09-10 11:44:43 UTC
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'.
Comment 12 Rui Matos 2012-09-10 12:23:11 UTC
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?
Comment 13 Takao Fujiwara 2012-09-10 12:40:32 UTC
(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.
Comment 14 Matthias Clasen 2012-09-10 12:53:12 UTC
I've filed 

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

for the string issues
Comment 15 Takao Fujiwara 2012-09-11 10:42:15 UTC
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.
Comment 16 Takao Fujiwara 2012-09-13 10:44:24 UTC
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.
Comment 17 Matthias Clasen 2012-09-13 23:29:39 UTC
That looks better to me, thanks
Comment 18 Rui Matos 2012-09-14 15:48:33 UTC
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
Comment 19 Matthias Clasen 2012-09-15 04:41:08 UTC
can this get reviewed, so that we have a chance to land it before .92 ?
Comment 20 Takao Fujiwara 2012-09-16 18:09:25 UTC
(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.
Comment 21 Florian Müllner 2012-09-17 13:13:47 UTC
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.
Comment 22 Rui Matos 2012-09-17 15:07:31 UTC
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.
Comment 23 Florian Müllner 2012-09-17 18:41:01 UTC
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.
Comment 24 Rui Matos 2012-09-17 19:33:23 UTC
Created attachment 224547 [details] [review]
status/keyboard: Add menu items for IBus Anthy's InputMode, TypingMode

--

Ok, I think it's better now.
Comment 25 Florian Müllner 2012-09-17 19:40:16 UTC
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 ...
Comment 26 Rui Matos 2012-09-17 19:52:35 UTC
(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.
Comment 27 Rui Matos 2012-09-17 21:15:28 UTC
(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.
Comment 28 Rui Matos 2012-09-17 22:01:52 UTC
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) {
Comment 29 Florian Müllner 2012-09-17 22:31:32 UTC
(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 ;-)
Comment 30 Florian Müllner 2012-09-17 22:33:38 UTC
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?
Comment 31 Florian Müllner 2012-09-17 22:37:17 UTC
(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.
Comment 32 Rui Matos 2012-09-18 10:26:46 UTC
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.
Comment 33 Rui Matos 2012-09-18 10:26:58 UTC
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.
Comment 34 Florian Müllner 2012-09-18 11:01:01 UTC
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 :)
Comment 35 Florian Müllner 2012-09-18 11:06:03 UTC
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
Comment 36 Rui Matos 2012-09-18 19:36:07 UTC
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