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 641531 - Integration of keyboard indicator with input methods
Integration of keyboard indicator with input methods
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: 676101 676583 678164 678337
Blocks: 648749
 
 
Reported: 2011-02-04 19:01 UTC by Takao Fujiwara
Modified: 2012-11-28 08:47 UTC
See Also:
GNOME target: 3.6
GNOME version: ---


Attachments
Patch for panel.js (4.46 KB, patch)
2011-02-04 19:08 UTC, Takao Fujiwara
none Details | Review
Patch for panel.js (4.45 KB, patch)
2011-02-04 19:27 UTC, Takao Fujiwara
none Details | Review
Patch for panel.js (3.70 KB, patch)
2011-02-09 08:26 UTC, Takao Fujiwara
none Details | Review
Patch for panel.js (4.65 KB, patch)
2011-02-15 10:43 UTC, Takao Fujiwara
none Details | Review
Patch for panel.js (4.45 KB, patch)
2011-02-28 17:36 UTC, Takao Fujiwara
none Details | Review
Patch for panel.js (4.55 KB, patch)
2011-02-28 17:40 UTC, Takao Fujiwara
reviewed Details | Review
Patch for panel.js (4.67 KB, patch)
2011-03-04 11:13 UTC, Takao Fujiwara
none Details | Review
Patch for panel.js (4.89 KB, patch)
2011-03-16 13:30 UTC, Takao Fujiwara
needs-work Details | Review
Patch for statusIconDispatcher.js (882 bytes, patch)
2011-03-25 19:05 UTC, Takao Fujiwara
committed Details | Review
Patch for panel.js (6.22 KB, patch)
2011-06-02 04:07 UTC, Takao Fujiwara
none Details | Review
Patch for panel.js (7.06 KB, patch)
2011-07-14 11:08 UTC, Takao Fujiwara
reviewed Details | Review
Patch for panel.js (135.43 KB, patch)
2011-08-06 15:27 UTC, Takao Fujiwara
none Details | Review
Patch for panel.js (134.60 KB, patch)
2011-08-14 03:29 UTC, Takao Fujiwara
needs-work Details | Review
Patch for panel.js (123.82 KB, patch)
2011-08-18 03:38 UTC, Takao Fujiwara
needs-work Details | Review
Patch for panel.js (114.54 KB, patch)
2011-08-23 15:27 UTC, Takao Fujiwara
reviewed Details | Review
screenshot1 (60.82 KB, image/png)
2011-10-07 00:51 UTC, Matthias Clasen
  Details
screenshot2 (92.97 KB, image/png)
2011-10-07 00:52 UTC, Matthias Clasen
  Details
screenshot3 (91.54 KB, image/png)
2011-10-07 01:47 UTC, Matthias Clasen
  Details
screenshot4 (100.62 KB, image/png)
2011-10-07 02:04 UTC, Matthias Clasen
  Details
Patch for panel.js (118.72 KB, patch)
2011-10-07 09:23 UTC, Takao Fujiwara
none Details | Review
Patch for panel.js (149.42 KB, patch)
2011-11-18 11:31 UTC, Takao Fujiwara
none Details | Review
Patch for panel.js (145.13 KB, patch)
2011-11-29 04:20 UTC, Takao Fujiwara
none Details | Review
Patch for panel.js (152.76 KB, patch)
2012-02-04 05:21 UTC, Takao Fujiwara
none Details | Review
status/keyboard: Port to the new input sources settings (14.76 KB, patch)
2012-05-29 17:50 UTC, Rui Matos
needs-work Details | Review
status/keyboard: Add support for IBus input sources (3.26 KB, patch)
2012-05-29 17:51 UTC, Rui Matos
reviewed Details | Review
candidatePanel: A candidate popup for IBus input methods (31.65 KB, patch)
2012-05-29 17:51 UTC, Rui Matos
none Details | Review
candidatePanel: Remove hardcoded styling (2.75 KB, patch)
2012-05-29 17:51 UTC, Rui Matos
none Details | Review
candidatePanel: Remove the PopupSeparatorMenuItem copy (2.97 KB, patch)
2012-05-29 17:51 UTC, Rui Matos
none Details | Review
candidatePanel: Use the Lang.Class framework (1.64 KB, patch)
2012-05-29 17:51 UTC, Rui Matos
none Details | Review
candidatePanel: A candidate popup for IBus input methods (29.70 KB, patch)
2012-05-29 17:56 UTC, Rui Matos
needs-work Details | Review
status/keyboard: Port to the new input sources settings (15.97 KB, patch)
2012-06-01 16:06 UTC, Rui Matos
committed Details | Review
Patch for IME menu items (26.00 KB, patch)
2012-06-08 09:47 UTC, Takao Fujiwara
none Details | Review
status/keyboard: Add support for IBus input sources (4.50 KB, patch)
2012-06-18 16:55 UTC, Rui Matos
reviewed Details | Review
ibusCandidatePopup: A candidate popup for IBus input methods (12.54 KB, patch)
2012-06-18 17:04 UTC, Rui Matos
reviewed Details | Review
status/keyboard: Add support for IBus input sources (6.29 KB, patch)
2012-06-23 00:27 UTC, Rui Matos
reviewed Details | Review
ibusCandidatePopup: A candidate popup for IBus input methods (14.26 KB, patch)
2012-06-23 00:31 UTC, Rui Matos
reviewed Details | Review
status/keyboard: Add support for IBus input sources (6.14 KB, patch)
2012-06-27 00:25 UTC, Rui Matos
accepted-commit_now Details | Review
ibusCandidatePopup: A candidate popup for IBus input methods (14.63 KB, patch)
2012-06-27 00:26 UTC, Rui Matos
accepted-commit_now Details | Review
status/keyboard: Add support for IBus input sources (5.73 KB, patch)
2012-06-28 13:59 UTC, Rui Matos
accepted-commit_now Details | Review
ibusCandidatePopup: A candidate popup for IBus input methods (14.58 KB, patch)
2012-06-28 14:00 UTC, Rui Matos
reviewed Details | Review
status/keyboard: Add support for IBus input sources (5.78 KB, patch)
2012-07-09 15:22 UTC, Rui Matos
committed Details | Review
ibusCandidatePopup: A candidate popup for IBus input methods (14.69 KB, patch)
2012-07-09 15:22 UTC, Rui Matos
committed Details | Review

Description Takao Fujiwara 2011-02-04 19:01:10 UTC
Downstream bug:
https://bugzilla.redhat.com/show_bug.cgi?id=657165

The original ibus panel uses GtkStatusIcon and gnome-shell shows it on bottom.
We'd like to show the ibus icon on top instead.

My understanding is we need to modify panel.js to show ibus and ibus needs to implement the gnome-shell UI.

Now I rewrote the basic ibus panel for GJS:
https://github.com/fujiwarat/ibus/tree/gjs
https://github.com/fujiwarat/ibus/commit/8d057045bd0e31046745d601855418a21e7d5f5d

The following patch is for the part of gnome-shell.
https://bugzilla.redhat.com/attachment.cgi?id=476733&action=diff&context=patch&collapsed=&headers=1&format=raw
Probably it's an idea to have the plugin as the patch since the part of ibus gjs has several files.

I also pushed the tarball of the part of ibus gjs only:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/gjs.tar.bz2

Could you review the patch?
Comment 1 Takao Fujiwara 2011-02-04 19:08:26 UTC
Created attachment 180101 [details] [review]
Patch for panel.js

(In reply to comment #0)
> The following patch is for the part of gnome-shell.
> https://bugzilla.redhat.com/attachment.cgi?id=476733&action=diff&context=patch&collapsed=&headers=1&format=raw

The same patch is attached.
Comment 2 Takao Fujiwara 2011-02-04 19:27:59 UTC
Created attachment 180103 [details] [review]
Patch for panel.js

The chars before sed lines were space chars in the previous patch.
I changed the space chars to a tab char.
Comment 3 Takao Fujiwara 2011-02-04 19:41:13 UTC
(In reply to comment #0)
> Now I rewrote the basic ibus panel for GJS:
> https://github.com/fujiwarat/ibus/tree/gjs
> https://github.com/fujiwarat/ibus/commit/8d057045bd0e31046745d601855418a21e7d5f5d

I reset the previous commit to fix a typo:
I just renamed ibuscandidator.js to ibusindicator.js
https://github.com/fujiwarat/ibus/commit/9d9709b69c19772961a46cdd98abf7ddfa746981
Comment 4 Bastien Nocera 2011-02-04 19:51:20 UTC
I believe this should actually be integrated in the layout selection icon:
http://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/InputLanguage

and the panel for gnome-control-center:
http://live.gnome.org/Design/SystemSettings/RegionAndLanguage
Comment 5 Matthias Clasen 2011-02-04 21:15:01 UTC
Yes, indeed, we need this integrated in the keyboard indicator.
Comment 6 Giovanni Campagna 2011-02-05 13:44:03 UTC
Review of attachment 180101 [details] [review]:

::: configure.ac
@@ +192,3 @@
+    ,
+    [with_ibus_pkgdatadir='$(datadir)/ibus']
+)

IBus has a pkgconfig. Can't you put pkgdatadir there?

::: js/ui/status/ibus.js.in
@@ +25,3 @@
+const GLib = imports.gi.GLib;
+
+const SystemStatusButton = imports.ui.panelMenu.SystemStatusButton;

Usually, style guide says to import modules, not single classes.

@@ +27,3 @@
+const SystemStatusButton = imports.ui.panelMenu.SystemStatusButton;
+
+if (GLib.file_test('@IBUS_PKGDATADIR@/ui/gjs', GLib.FileTest.IS_DIR)) {

Use imports.misc.config and patch misc/config.js.in
Also put the check inside configure.ac, rather than runtime.

@@ +29,3 @@
+if (GLib.file_test('@IBUS_PKGDATADIR@/ui/gjs', GLib.FileTest.IS_DIR)) {
+    imports.searchPath.push('@IBUS_PKGDATADIR@/ui/gjs');
+    const ibusindicator = imports.ibusindicator;

Is it possible to avoid the native JS module, importing IBus from GI and having UI stuff here?
Does it need C code?

@@ +32,3 @@
+}
+
+Indicator.prototype = {

While not relied upon currently, I expect every indicator to be a SystemStatusButton (rather than just expose actor and menu), or at least a PanelMenu.Button.
Comment 7 Takao Fujiwara 2011-02-07 05:48:49 UTC
(In reply to comment #6)
> IBus has a pkgconfig. Can't you put pkgdatadir there?

OK, probably it's a good idea. I'll change ibus.pc in upstream.

> 
> ::: js/ui/status/ibus.js.in
> @@ +25,3 @@
> +const GLib = imports.gi.GLib;
> +
> +const SystemStatusButton = imports.ui.panelMenu.SystemStatusButton;
> 
> Usually, style guide says to import modules, not single classes.

The GLib checks if /usr/share/ibus/ui/gjs exists. If not, it returns SystemStatusButton not to be crashed.

> 
> @@ +27,3 @@
> +const SystemStatusButton = imports.ui.panelMenu.SystemStatusButton;
> +
> +if (GLib.file_test('@IBUS_PKGDATADIR@/ui/gjs', GLib.FileTest.IS_DIR)) {
> 
> Use imports.misc.config and patch misc/config.js.in
> Also put the check inside configure.ac, rather than runtime.

OK, I see.

> 
> @@ +29,3 @@
> +if (GLib.file_test('@IBUS_PKGDATADIR@/ui/gjs', GLib.FileTest.IS_DIR)) {
> +    imports.searchPath.push('@IBUS_PKGDATADIR@/ui/gjs');
> +    const ibusindicator = imports.ibusindicator;
> 
> Is it possible to avoid the native JS module, importing IBus from GI and having
> UI stuff here?
> Does it need C code?

I think the ibusindicator needs to accesse JS GUI likes popupMenu.js, lightbox.js, tweener.js, statusMenu.js


> While not relied upon currently, I expect every indicator to be a
> SystemStatusButton (rather than just expose actor and menu), or at least a
> PanelMenu.Button.

I'm thinking a plugin module so that gnome-shell works when ibus does not exist since there are several ibus JS files.

The ibusindicator inherits SystemStatusButton.
The proble is here:

Indicator.prototype = {
    __proto__: SystemStatusLabeLButton.prototype,

...
}

If the __proto__ doesn't exist, I think the code causes a crash.
My understanding is we cannot enclose __proto__ with if GLib.file_test().
So I moved the the inherited class from gnome-shell/js/ui/status/ibus.js to ibus/ui/gjs/ibusindicator.js

I'm not sure but it might be good to try to combine ibus and keyboard after ibus is integrated into gnome-shell. There might be several discussions.
Comment 8 Takao Fujiwara 2011-02-09 08:26:56 UTC
Created attachment 180446 [details] [review]
Patch for panel.js

I revised the patch for IBus.
Do you have any ideas?

We upstreamed the pkgdatadir for ibus.pc.
https://github.com/ibus/ibus/commit/0501756a1e51469849eca064aeb1e340afbf8be2
Comment 9 Giovanni Campagna 2011-02-14 16:51:02 UTC
Actually, I think the question is whether IBus should be considered a hard-dependency or optional.
In the first case, all this checking machinery is not needed, and configure.ac must require ibus to be present (and of course pointing to a valid directory).
This also means importing IBus unconditionally, including all your special variation on standard components.
Also, SystemStatusLabelButton might be put inside the gnome-shell tree, it would be useful to XkbIndicator (keyboard) as well.

Finally, I'd say that this would be the first case of loading external JS files, among all the status items - that's way I proposed full integration (that is, putting everything under js/status/ibus/) in comment 6.
Consider that the GNOME Shell is very unstable API-side, you'd better do some kind of versioning if you want to support development versions.
Comment 10 Owen Taylor 2011-02-14 17:25:10 UTC
There are two questions here:

 Question 1: Do we want to hard-depend on IBus now? Do we we want to hard-depend on a very new very of IBus?

   My answer: No. I don't want to add a new hard dependency at this point right before the 3.0 release. 

 Question 2: Do we want tight integration with IBus as the only supported input framework.

  My answer: Yes. To get a good input method experience, we need to work closely together with the input method framework on a design and code level. There's no way we can provide a good experience supporting IBus and also other input methods.

So, we do need configure checks. But I'd like to have the code for the indicator inside the GNOME Shell tree. It doesn't make sense to have one part in the tree, and one part of the code distributed with IBus.

We've consistently said at this point that JS code that forms part of the GNOME UI should be shipped with GNOME Shell - for the system status dialogs, for the Bluetooth indicator, etc. We don't have a stable JS API or set of CSS class names.
Comment 11 Takao Fujiwara 2011-02-15 10:43:03 UTC
Created attachment 180888 [details] [review]
Patch for panel.js

I moved the all js files into gnome-shell with this patch.
I put the part of ibus files here:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/gnome-shell-ibus-plugins-20110215.tar.bz2

I can merge the patch and files into a long patch upon the request.
Probably I think most ibus APIs are integrated into ibus gir.

To work that patch with ibus, the following patch is also needed.
I put the ibus part in gjs git branch:
https://github.com/fujiwarat/ibus/commit/4c390e4259cf3f78ef1fd685925d275be4952ed1
https://github.com/fujiwarat/ibus/commits/gjs/

Fedora 15 or later ibus includes the patch for the test purpose.
Comment 12 Giovanni Campagna 2011-02-15 19:41:31 UTC
Comments from a cursory review:

- Your patch includes a lot of Gtk code for running the indicator standalone.
This would not be needed once this is merged inside gnome-shell.
- Even if that code remains, you should not use Gtk.main_*. Use imports.mainloop.
- Your code expects to be able to create Gtk toplevels. This won't work unless the windows are marked override-redirect, because it's inside a window manager.
- Place a configure variable like "HAVE_IBUS", and place the only config-dependent code in panel.js like it is done for bluetooth. This way indicator.js is not needed, and you can directly instantiate the real indicator.
- UIApplication should not kill gnome-shell when IBus dies. Instead it should just hide, possibly with a warning.
- The correct way to instantiate objects is "new Namespace.Class({ param: value, param2: value2 })". Use the "Namespace.Class.new" syntax only when absolutely needed (should be never in GTK+3).
- The type_name property inside prototypes is not needed (I suppose it is reminiscent of pygobject inheritance).
Comment 13 Takao Fujiwara 2011-02-28 17:36:26 UTC
Created attachment 182109 [details] [review]
Patch for panel.js

I revised the patch.

The part of js files are available below:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/gnome-shell-ibus-plugins-20110228.tar.bz2

(In reply to comment #12)
> - Your patch includes a lot of Gtk code for running the indicator standalone.
> This would not be needed once this is merged inside gnome-shell.
> - Even if that code remains, you should not use Gtk.main_*. Use
> imports.mainloop.

OK, I removed the almost GTK codes.

> - Your code expects to be able to create Gtk toplevels. This won't work unless
> the windows are marked override-redirect, because it's inside a window manager.

If I set window.set_override_redirect(false), I could not set a focus on the window. I'd need a popup window for IM lookup window. Maybe the current Gtk.Window is ok with me.

> - Place a configure variable like "HAVE_IBUS", and place the only
> config-dependent code in panel.js like it is done for bluetooth. This way
> indicator.js is not needed, and you can directly instantiate the real
> indicator.

OK, I set the HAVE_IBUS. However I think gnome-shell will get SEGV, if the try is removed and ibus-daemon is not running.

if (Config.HAVE_IBUS) {
    try {
        STANDARD_TRAY_ICON_SHELL_IMPLEMENTATION['ibus'] = imports.ui.status.ibus.indicator.Indicator;
    } catch (exp) {
    }
}

> - UIApplication should not kill gnome-shell when IBus dies. Instead it should
> just hide, possibly with a warning.

OK, I see.

> - The correct way to instantiate objects is "new Namespace.Class({ param:
> value, param2: value2 })". Use the "Namespace.Class.new" syntax only when
> absolutely needed (should be never in GTK+3).

OK, I changed the GTK part. IBus needs the format since some classes doesn't use the properties.

> - The type_name property inside prototypes is not needed (I suppose it is
> reminiscent of pygobject inheritance).

OK, I see.
Comment 14 Takao Fujiwara 2011-02-28 17:40:10 UTC
Created attachment 182110 [details] [review]
Patch for panel.js

I uploaded the old patch.
Now I attached the patch again.
Comment 15 Giovanni Campagna 2011-02-28 18:47:46 UTC
(In reply to comment #13)
> Created an attachment (id=182109) [details] [review]
> Patch for panel.js
> 
> > - Your code expects to be able to create Gtk toplevels. This won't work unless
> > the windows are marked override-redirect, because it's inside a window manager.
> 
> If I set window.set_override_redirect(false), I could not set a focus on the
> window. I'd need a popup window for IM lookup window. Maybe the current
> Gtk.Window is ok with me.

You must call window.set_override_redirect(true), or mutter will deadlock (you can try it from the looking glass, 100% reproducible).
If that interferes with setting focus, I'm afraid you'll need either to have the panel as a separate binary, or to use Shell/St/Clutter popups (that don't create new X11 windows).

> 
> > - Place a configure variable like "HAVE_IBUS", and place the only
> > config-dependent code in panel.js like it is done for bluetooth. This way
> > indicator.js is not needed, and you can directly instantiate the real
> > indicator.
> 
> OK, I set the HAVE_IBUS. However I think gnome-shell will get SEGV, if the try
> is removed and ibus-daemon is not running.
> 
> if (Config.HAVE_IBUS) {
>     try {
>         STANDARD_TRAY_ICON_SHELL_IMPLEMENTATION['ibus'] =
> imports.ui.status.ibus.indicator.Indicator;
>     } catch (exp) {
>     }
> }

That try has no effect if dbus-daemon is not running (Indicator is instantiated later, in Panel.startStatusArea). On the other hand, it will make an installation error go unnoticed. I think it should go away.

> > - The correct way to instantiate objects is "new Namespace.Class({ param:
> > value, param2: value2 })". Use the "Namespace.Class.new" syntax only when
> > absolutely needed (should be never in GTK+3).
> 
> OK, I changed the GTK part. IBus needs the format since some classes doesn't
> use the properties.

I'd say "fix them on the IBus part", but then it's ok like that.

Some more comments:

- use log instead of printerr, so messages end up nicely formatted with "JS LOG: ".
- differently from python, methods are not bound in JS, unless you use Lang.bind(object, method). (I'm referring to passing this.menu.actor.hide in Indicator.prototype._init).
- I see no problem in creating a Gdk.Cursor with Gdk.Cursor.new (top of candidatePanel.js)
- I see no problem with Pango.AttrList (same file)
- use single quotes for strings that are not translated (signal names, style classes, icon names), and double quotes for everything else
- it's not that GJS does not use gtk_main, it's just that you don't want to restart the shell (panel.js, line 146)
- in line 202, it happens that a menu is opened without pushModal (if you navigate from one to another with the keyboard, for example), in which case your popModal won't match anything known
- please remove all commented code that is not relevant (shellMenu.js)
- PopupMenuItems don't have a set_sensitive, use the reactive property on .actor (line 174)
- menu_position seems to be never called; if so, just remove it (line 452)
- ShellEngineAbout should be a ModalDialog
- there is no 'always-show-image' for St actors (even less so for PopupImageMenuItems) (languageBar.js, line 62)
- you either use "new Namespace.Class({ properties })", or "Namespace.Class.new(args)", "new Namespace.Class.new" is not valid syntax (languageBar.js, line 168)
- typo: GOBject should be GObject (handle.js, line 23)
- what's a Handle? what it is used for?
- never invoke explicitly clutter_actor_paint, use queue_redraw (panelMenu.js, lines 76 and 86)
Comment 16 Giovanni Campagna 2011-02-28 18:49:17 UTC
Comment on attachment 182110 [details] [review]
Patch for panel.js

Looks mostly ok, except that I think ibus should be near keyboard, not at the end.
Comment 17 Takao Fujiwara 2011-03-04 11:13:48 UTC
Created attachment 182450 [details] [review]
Patch for panel.js

Revised the patch since I forgot to update misc/config.js.in .

I revised the gjs files:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/gnome-shell-ibus-plugins-20110304.tar.bz2

(In reply to comment #15)
> (In reply to comment #13)
> > Created an attachment (id=182109) [details] [review] [details] [review]
> > Patch for panel.js
> > 
> > > - Your code expects to be able to create Gtk toplevels. This won't work unless
> > > the windows are marked override-redirect, because it's inside a window manager.
> > 
> > If I set window.set_override_redirect(false), I could not set a focus on the
> > window. I'd need a popup window for IM lookup window. Maybe the current
> > Gtk.Window is ok with me.
> 
> You must call window.set_override_redirect(true), or mutter will deadlock (you
> can try it from the looking glass, 100% reproducible).
> If that interferes with setting focus, I'm afraid you'll need either to have
> the panel as a separate binary, or to use Shell/St/Clutter popups (that don't
> create new X11 windows).

If you suggest window.set_override_redirect(true), I think the CandidatePanel already has override_redirect = TRUE because the window type is Gtk.WindowType.POPUP .
So probably we don't have to update anything here or do I miss anything?

> 
> > 
> > > - Place a configure variable like "HAVE_IBUS", and place the only
> > > config-dependent code in panel.js like it is done for bluetooth. This way
> > > indicator.js is not needed, and you can directly instantiate the real
> > > indicator.
> > 
> > OK, I set the HAVE_IBUS. However I think gnome-shell will get SEGV, if the try
> > is removed and ibus-daemon is not running.
> > 
> > if (Config.HAVE_IBUS) {
> >     try {
> >         STANDARD_TRAY_ICON_SHELL_IMPLEMENTATION['ibus'] =
> > imports.ui.status.ibus.indicator.Indicator;
> >     } catch (exp) {
> >     }
> > }
> 
> That try has no effect if dbus-daemon is not running (Indicator is instantiated
> later, in Panel.startStatusArea). On the other hand, it will make an
> installation error go unnoticed. I think it should go away.

OK, I agree the try is not needed.
If users don't install ibus, gnome-shell will be crash because of no typelib?
I think the installers can disable input-methods.

> > OK, I changed the GTK part. IBus needs the format since some classes doesn't
> > use the properties.
> 
> I'd say "fix them on the IBus part", but then it's ok like that.

OK, we will think this later.


> - differently from python, methods are not bound in JS, unless you use
> Lang.bind(object, method). (I'm referring to passing this.menu.actor.hide in
> Indicator.prototype._init).

I modified the code. Probably I think the latest patch is ok.

> - I see no problem in creating a Gdk.Cursor with Gdk.Cursor.new (top of
> candidatePanel.js)
> - I see no problem with Pango.AttrList (same file)

I have the problem in Gdk.Cursor and Pango.AttrList.
I put the uncommented file candidatePanel.js:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/candidatePanel.js

% gjs candidatePanel.js
(gjs:1904): GLib-GObject-WARNING **: unable to set property `attributes' of type `PangoAttrList' from value of type `GObject'
    JS ERROR: !!!   Exception was: Error: Object is not a dynamically-registered class based on expected static class pointer
([object Array],0,true)@candidatePanel.js:199

After I commented out line 199 again:
% gjs candidatePanel.js
(gjs:1944): GLib-GObject-WARNING **: cannot create instance of abstract (non-instantiatable) type `GdkCursor'


> - it's not that GJS does not use gtk_main, it's just that you don't want to
> restart the shell (panel.js, line 146)

My comment was wrong. I'd like to enable the restart but current I get SEGV in GJS when restart DBus.
I updated the comments.

> - in line 202, it happens that a menu is opened without pushModal (if you
> navigate from one to another with the keyboard, for example), in which case
> your popModal won't match anything known

If pushModel is enabled there, I got the problems.
Currently I use ibus.popupMenu.PopupMenuNoOpenStateChanged so I removed those lines.

> - PopupMenuItems don't have a set_sensitive, use the reactive property on
> .actor (line 174)
> - menu_position seems to be never called; if so, just remove it (line 452)
> - ShellEngineAbout should be a ModalDialog
> - there is no 'always-show-image' for St actors (even less so for
> PopupImageMenuItems) (languageBar.js, line 62)

Thanks. I forgot those.

> - you either use "new Namespace.Class({ properties })", or
> "Namespace.Class.new(args)", "new Namespace.Class.new" is not valid syntax
> (languageBar.js, line 168)

OK, I modified.

> - what's a Handle? what it is used for?

I have the screen shot of the CandidatePanel:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/img/g-s4.png

Handle is located to the left in CandidatePanel.
The default position of CandidatePanel is near typing chars but users can move the position of CandidatePanel dragging Handle.
So the candidate window is always foremost and it can be moved.

BTW, when I type chars in gnome-shell search box, the CandidatePanel is not shown. Probably we need to fix it later (or create the shell base candidate window?).

When I set signal_subscribe for NameLost signal for GDBus + GJS, I could not receive the signal.
https://github.com/fujiwarat/ibus/blob/gjs/ui/gjs-g-s/indicator.js#L83

When I set add_signal_receiver for NameLost signal for DBus + PyGTK2, I could receive the signal.
https://github.com/fujiwarat/ibus/blob/gjs/ui/gtk/main.py#L59

Currently I don't know what is wrong in indicator.py .
Thanks.
Comment 18 Giovanni Campagna 2011-03-08 14:34:25 UTC
(In reply to comment #17)
> Created an attachment (id=182450) [details] [review]
> Patch for panel.js
> 
> Revised the patch since I forgot to update misc/config.js.in .
> 
> I revised the gjs files:
> http://fujiwara.fedorapeople.org/ibus/gnome-shell/gnome-shell-ibus-plugins-20110304.tar.bz2
> 
> (In reply to comment #15)
> If you suggest window.set_override_redirect(true), I think the CandidatePanel
> already has override_redirect = TRUE because the window type is
> Gtk.WindowType.POPUP .
> So probably we don't have to update anything here or do I miss anything?

You're right, set_override_redirect is automatic for that window type. So no problem.

> OK, I agree the try is not needed.
> If users don't install ibus, gnome-shell will be crash because of no typelib?
> I think the installers can disable input-methods.

If gnome-shell is configured with HAVE_IBUS, but then ibus is missing, yes it will be unable to load. But this is packaging problem: once you configure with ibus enabled, it becomes a dependency, like a shared library.
 
> > - differently from python, methods are not bound in JS, unless you use
> > Lang.bind(object, method). (I'm referring to passing this.menu.actor.hide in
> > Indicator.prototype._init).
> 
> I modified the code. Probably I think the latest patch is ok.

Yes, that way is works.

> > - I see no problem in creating a Gdk.Cursor with Gdk.Cursor.new (top of
> > candidatePanel.js)
> > - I see no problem with Pango.AttrList (same file)
> 
> I have the problem in Gdk.Cursor and Pango.AttrList.
> I put the uncommented file candidatePanel.js:
> http://fujiwara.fedorapeople.org/ibus/gnome-shell/candidatePanel.js
> 
> % gjs candidatePanel.js
> (gjs:1904): GLib-GObject-WARNING **: unable to set property `attributes' of
> type `PangoAttrList' from value of type `GObject'
>     JS ERROR: !!!   Exception was: Error: Object is not a
> dynamically-registered class based on expected static class pointer
> ([object Array],0,true)@candidatePanel.js:199

Check more what is "attrs", it needs to be a PangoAttrList, not a GObject. Also,
use "label.attributes = attrs" instead of "label.set_property('attributes', attrs)" (which avoids problems with marshalling of GValues).

> After I commented out line 199 again:
> % gjs candidatePanel.js
> (gjs:1944): GLib-GObject-WARNING **: cannot create instance of abstract
> (non-instantiatable) type `GdkCursor'

It does not work with "new Gdk.Cursor", but it works with "Gdk.Cursor.new(type)".
(It may be worthwhile to file a bug on GTK+)

> 
> > - it's not that GJS does not use gtk_main, it's just that you don't want to
> > restart the shell (panel.js, line 146)
> 
> My comment was wrong. I'd like to enable the restart but current I get SEGV in
> GJS when restart DBus.
> I updated the comments.

You're calling exportObject twice with the same path. If you want to restart the panel (I'm not sure why you would want that), you need to first unexport the existing exposed object.

> BTW, when I type chars in gnome-shell search box, the CandidatePanel is not
> shown. Probably we need to fix it later (or create the shell base candidate
> window?).

The CandidatePanel is a GtkWindow, thus a X window. All X11 windows but the one corresponding to the Shell's Clutter stage are covered while in the overview and not visible.
To support the search box, and entries for notifications, and shell modal dialogs, you need to rewrite CandidatePanel to be a ClutterActor, and then use Main.chrome.addActor to make it appear.

> When I set signal_subscribe for NameLost signal for GDBus + GJS, I could not
> receive the signal.
> https://github.com/fujiwarat/ibus/blob/gjs/ui/gjs-g-s/indicator.js#L83
> 
> When I set add_signal_receiver for NameLost signal for DBus + PyGTK2, I could
> receive the signal.
> https://github.com/fujiwarat/ibus/blob/gjs/ui/gtk/main.py#L59
> 
> Currently I don't know what is wrong in indicator.py .
> Thanks.

You're mixing a lot of stuff in that code, manual add_match, request_name, signal_subscribe on the GDBusConnection...
Why don't you just use Gio.bus_own_name_on_connection()?

In general, the patch is fine from an implementation perspective.
I'm not sure if it can go in, because of UI freeze. I believe an exception can be granted in this case, but it is not my call.
Comment 19 Owen Taylor 2011-03-09 23:50:49 UTC
I've been doing some testing of this code (using a RPM I built locally from the tip of fujiwarat/gjs branch with the XKB layout patch from fujiwarat/master added on.)

It works quite well, and the integration is definitely a lot better than with the old status icon.

Some immediate bugs noted:

 * The first time I pop up the menu, it doesn't work properly, and I get warnings:

(lt-gnome-shell-real:17768): IBUS-CRITICAL **: ibus_serializable_deserialize: 
 assertion `g_type_is_a (type, IBUS_TYPE_SERIALIZABLE)' failed
 
 * I also get warnings:

    JS ERROR: !!!   Exception was: Error: Don't know how to convert GType GVariant to JavaScript object

  Not sure how to reproduce.

 * When no input method is selected, I just get a blank space in the status area

 * When the menu is up and I click off elsewhere, the menu isn't properly dismissed but sticks up. Some times when this happens I get a stuck grab that makes the shell entirely unusable.

Some simple UI improvements I'd like to see in the short term:

 * The About menu item should be removed from the menu. It's not something that a user needs fast access to.

 * We also should remove the About menu items for individual input methods. If you chose an input method, so you should know what it is. The same information is available from preferences.

 *  When there is only one input method for a language, we should display only the language name. If I have Hindi in the menu, I should just see:

  hi    Hindi

Not hi Hindi (inscript (m17n)). In the unusual case where I have multiple input methods selected for the same language, then extra information can be displayed to distinguish them.

 * The language codes and names should be lined up in columns. See the XKB indicator and https://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/InputLanguage

 * "Turn off input method" should be renamed to "No input method" and displayed as selected when it's selected, not disabled when it's selected.

 * The selected input method should be indicated with a bullet as in XKB indicator. (Or like in https://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/InputLanguage)

 * "Radio" choices presented by an input method, like the different modes and options of Anthy should be indicated with a bullet, not with a row of slider switches.

 * If possible, when we have an input method with modes, the current mode should display in the panel rather than the generic 'ja' (or whatever). So when you have Anthy, you see あ ア, etc depending on the current mode.

 * If possible, "Turn off input method" should be changed to look more like another another input method. I don't really have any ideas of what would be displayed in the panel and the menu, however. My only idea would be to make localized strings with a translator comment. Some examples:

 en_US - en / English
 fr_FR - fr / French
 hi_EN - en / <English translated to Hindi>

 * We need to display the old status icon when in fallback mode. You can do this one of two ways:

 - Just always create the old status icon, and we'll hide it and not display it as we do for other legacy status icons.
 - Check for org.gnome.Shell on the bus when starting to decide if to display it or not. (Though if ibus is started *before* gnome-session and gnome-shell, I guess this would be problematical.)

So the idea is that they would represent the language you'd typically be entering in that locale. Not perfect. This may need to wait.

Longer term work, post 3.0:

 * The XKB and IBus indicators should be integrated together. My initial impression trying out ibus-xkb is that it isn't the right way to do that, since there are some significant differences between a keyboard layout and an input method. We'll be better off if we integrate them at the UI level rather than at the framework level.

 * Configuration of IBus input methods should be moved into the "Regions & Languages" panel as in https://live.gnome.org/Design/SystemSettings/RegionAndLanguage

 * We need to have stripped down GNOME-compliant versions of the user interfaces for input methods. Something like the Anthy preferences dialog doesn't fit in with the general approach we take to settings within GNOME.

 * We need to remove the distinction between "Input method switched off" and "Input method is in a mode which allows direct input of keyboard characters". E.g, the distinction between Anthy in Latin mode and having the input method turned off isn't something that is going to make sense to users.
Comment 20 Takao Fujiwara 2011-03-16 13:30:59 UTC
Created attachment 183536 [details] [review]
Patch for panel.js

I put the latest ibus gjs files:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/gnome-shell-ibus-plugins-20110316.tar.bz2

(In reply to comment #18)
> > OK, I agree the try is not needed.
> > If users don't install ibus, gnome-shell will be crash because of no typelib?
> > I think the installers can disable input-methods.
> 
> If gnome-shell is configured with HAVE_IBUS, but then ibus is missing, yes it
> will be unable to load. But this is packaging problem: once you configure with
> ibus enabled, it becomes a dependency, like a shared library.

I mean ibus is an i18n package and gnome-shell is a base package.
Probably I think gnome-shell don't have to depend on ibus for English users and they could install gnome-shell without ibus using the distro-installers.
I guess gnome-shell will have BuildRequire = ibus-devel but don't have to have Require = ibus ?

> Check more what is "attrs", it needs to be a PangoAttrList, not a GObject.
> Also,
> use "label.attributes = attrs" instead of "label.set_property('attributes',
> attrs)" (which avoids problems with marshalling of GValues).

Thanks for the evaluation.
I fixed it in the latest tarball.

> It does not work with "new Gdk.Cursor", but it works with
> "Gdk.Cursor.new(type)".
> (It may be worthwhile to file a bug on GTK+)

Thanks. I fixed it.


> > My comment was wrong. I'd like to enable the restart but current I get SEGV in
> > GJS when restart DBus.
> > I updated the comments.
> 
> You're calling exportObject twice with the same path. If you want to restart
> the panel (I'm not sure why you would want that), you need to first unexport
> the existing exposed object.

OK, I understood.
I called bus.destroy() and fixed this problem.

> To support the search box, and entries for notifications, and shell modal
> dialogs, you need to rewrite CandidatePanel to be a ClutterActor, and then use
> Main.chrome.addActor to make it appear.

OK, I see.
I'll implement it.

> 
> > When I set signal_subscribe for NameLost signal for GDBus + GJS, I could not
> > receive the signal.
> > https://github.com/fujiwarat/ibus/blob/gjs/ui/gjs-g-s/indicator.js#L83
> > 
> > When I set add_signal_receiver for NameLost signal for DBus + PyGTK2, I could
> > receive the signal.
> > https://github.com/fujiwarat/ibus/blob/gjs/ui/gtk/main.py#L59
> > 
> > Currently I don't know what is wrong in indicator.py .
> > Thanks.
> 
> You're mixing a lot of stuff in that code, manual add_match, request_name,
> signal_subscribe on the GDBusConnection...
> Why don't you just use Gio.bus_own_name_on_connection()?

IBus uses the private bus. OK, I'll investigate it later.

> In general, the patch is fine from an implementation perspective.
> I'm not sure if it can go in, because of UI freeze. I believe an exception can
> be granted in this case, but it is not my call.

Great. Probably I think it's better to integrate ibus into gnome-shell in GNOME 3.
Comment 21 Takao Fujiwara 2011-03-16 14:56:58 UTC
(In reply to comment #19)
> I've been doing some testing of this code (using a RPM I built locally from the
> tip of fujiwarat/gjs branch with the XKB layout patch from fujiwarat/master
> added on.)
> 
> It works quite well, and the integration is definitely a lot better than with
> the old status icon.

Thanks much for your tests and time.

> 
> Some immediate bugs noted:
> 
>  * The first time I pop up the menu, it doesn't work properly, and I get
> warnings:
> 
> (lt-gnome-shell-real:17768): IBUS-CRITICAL **: ibus_serializable_deserialize: 
>  assertion `g_type_is_a (type, IBUS_TYPE_SERIALIZABLE)' failed
> 

I don't get the fix yet.
ibus_serializable_deserialize() calls g_return_val_if_fail (g_type_is_a (type, IBUS_TYPE_SERIALIZABLE), NULL) for IBusEngineDesc.
I don't know why g_type_is_a was failed. I will try to investigate it again.

>  * I also get warnings:
> 
>     JS ERROR: !!!   Exception was: Error: Don't know how to convert GType
> GVariant to JavaScript object
> 
>   Not sure how to reproduce.

Yes, I guess GJS needs to support GVariant.

>  * When no input method is selected, I just get a blank space in the status
> area

I hide ibus icon when ibus-daemon is not running with the latest tarball.
Probably I think this is fixed.

>  * When the menu is up and I click off elsewhere, the menu isn't properly
> dismissed but sticks up. Some times when this happens I get a stuck grab that
> makes the shell entirely unusable.

Yes, I'd like to keep the focus on the text applications even though the menu is open.
So I implemented PopupMenuNoOpenStateChanged in js/ui/status/ibus/popupMenu.js, which does not emit 'open-state-changed' due to Main.pushModal.
Probably we need to handle the mouse events.

> 
> Some simple UI improvements I'd like to see in the short term:
> 
>  * The About menu item should be removed from the menu. It's not something that
> a user needs fast access to.

OK, I removed it.
However I wonder if users like to get the product version from GUI.
ibus-setup is used by this ibus gjs and ibus gtk.
I think the GtkAboutDialog is a standalone dialog and I guess the menu item is needed.

>  * We also should remove the About menu items for individual input methods. If
> you chose an input method, so you should know what it is. The same information
> is available from preferences.

OK, I see. I removed it.

>  *  When there is only one input method for a language, we should display only
> the language name. If I have Hindi in the menu, I should just see:
> 
>   hi    Hindi
> 
> Not hi Hindi (inscript (m17n)). In the unusual case where I have multiple input
> methods selected for the same language, then extra information can be displayed
> to distinguish them.

OK, probably I fixed it.

>  * The language codes and names should be lined up in columns. See the XKB
> indicator and
> https://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/InputLanguage

I'm not sure if I could understand it.
I modified the code slightly.


>  * "Turn off input method" should be renamed to "No input method" and displayed
> as selected when it's selected, not disabled when it's selected.

I removed the menu item "Turn off input method" when IM engines are not available.
The menu item is used for IM on/off likes Ctrl+Space.

>  * The selected input method should be indicated with a bullet as in XKB
> indicator. (Or like in
> https://live.gnome.org/GnomeShell/Design/Guidelines/SystemStatus/InputLanguage)
> 
>  * "Radio" choices presented by an input method, like the different modes and
> options of Anthy should be indicated with a bullet, not with a row of slider
> switches.

Thanks. I didn't notice setShowDot() function.
Probably I think this is fixed.

>  * If possible, when we have an input method with modes, the current mode
> should display in the panel rather than the generic 'ja' (or whatever). So when
> you have Anthy, you see あ ア, etc depending on the current mode.

Yes, we like this idea.
Probably we need to create a new property name. The current property names are different between IM engines. E.g Anthy provides 'InputMode' but Pinyin provides 'mode.chinese'.
https://bugzilla.redhat.com/show_bug.cgi?id=545695#c5
I'll address the problem again.

>  * If possible, "Turn off input method" should be changed to look more like
> another another input method. I don't really have any ideas of what would be
> displayed in the panel and the menu, however. My only idea would be to make
> localized strings with a translator comment. Some examples:
> 
>  en_US - en / English
>  fr_FR - fr / French
>  hi_EN - en / <English translated to Hindi>

How about check menu item?
e.g.

 * ja Japanese  on
   zh Chinese  off
   ko Korean   off

When I click the ja menu item, it will be switched to 'off'.
This way may be able to remove the "Turn off input method" menu item.

>  * We need to display the old status icon when in fallback mode. You can do
> this one of two ways:
> 
>  - Just always create the old status icon, and we'll hide it and not display it
> as we do for other legacy status icons.
>  - Check for org.gnome.Shell on the bus when starting to decide if to display
> it or not. (Though if ibus is started *before* gnome-session and gnome-shell, I
> guess this would be problematical.)

I updated the ibus gtk panel.py to check org.gnome.Shell.

> 
> So the idea is that they would represent the language you'd typically be
> entering in that locale. Not perfect. This may need to wait.
> 
> Longer term work, post 3.0:
> 
>  * The XKB and IBus indicators should be integrated together. My initial
> impression trying out ibus-xkb is that it isn't the right way to do that, since
> there are some significant differences between a keyboard layout and an input
> method. We'll be better off if we integrate them at the UI level rather than at
> the framework level.

OK, thanks for the comment.
Probably I think we will need the furthermore discussions.
Originally I thought:
 - input methods and keyboard layouts could be shown in the same GUI.
 - the keyboard layout in input method could be customized.

>  * Configuration of IBus input methods should be moved into the "Regions &
> Languages" panel as in
> https://live.gnome.org/Design/SystemSettings/RegionAndLanguage

Which im-chooser or ibus-setup are you talking about?
The current im-chooser is available in gnome-control-center.

>  * We need to have stripped down GNOME-compliant versions of the user
> interfaces for input methods. Something like the Anthy preferences dialog
> doesn't fit in with the general approach we take to settings within GNOME.

Probably I don't understand your suggestion.
Probably I'd like to know what is the problem in the Anthy preferences dialog.

>  * We need to remove the distinction between "Input method switched off" and
> "Input method is in a mode which allows direct input of keyboard characters".
> E.g, the distinction between Anthy in Latin mode and having the input method
> turned off isn't something that is going to make sense to users.

Yes, it's a nice idea.
However Anthy also provides WideLatin mode so I guess providing Latin mode might not be so wrong in Anthy?
Comment 22 Takao Fujiwara 2011-03-17 02:46:34 UTC
OK, I updated the gjs files again:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/gnome-shell-ibus-plugins-20110317.tar.bz2

The part of ibus libraries is saved in my gjs branch.
https://github.com/fujiwarat/ibus/tree/gjs

(In reply to comment #21)
> > 
> > Some immediate bugs noted:
> > 
> >  * The first time I pop up the menu, it doesn't work properly, and I get
> > warnings:
> > 
> > (lt-gnome-shell-real:17768): IBUS-CRITICAL **: ibus_serializable_deserialize: 
> >  assertion `g_type_is_a (type, IBUS_TYPE_SERIALIZABLE)' failed
> > 
> 
> I don't get the fix yet.
> ibus_serializable_deserialize() calls g_return_val_if_fail (g_type_is_a (type,
> IBUS_TYPE_SERIALIZABLE), NULL) for IBusEngineDesc.
> I don't know why g_type_is_a was failed. I will try to investigate it again.

We fixed this issue in the latest tarball:
--- js-20110316/ui/status/ibus/indicator.js
+++ js/ui/status/ibus/indicator.js
@@ -51,6 +51,7 @@
 
 UIApplication.prototype = {
     _init: function(indicator) {
+        IBus.init();
         this._bus = new IBus.Bus();
         this._indicator = indicator;
         this._has_inited = false;
Comment 23 Takao Fujiwara 2011-03-25 19:05:13 UTC
Created attachment 184196 [details] [review]
Patch for statusIconDispatcher.js

I'd like to think if this patch can be integrated into GNOME 3.0.
The patch is very small at the moment.

We upstreamed the IBus program name:
https://github.com/ibus/ibus/commit/5ef29602141945ed1255662576c2e8194af78325

Regarding to the ibus icon, GtkStatusIcon supports the image file only but not text.
How about a dark icon at the moment?
https://github.com/fujiwarat/ibus/raw/gjs/data/icons/16/ibus-keyboard.png
Now the part of ibus was integrated in ibus-1.3.99.20110228-5.fc15.
Probably I think we can continue to discuss the better icon during Fedora beta?
The change effects both gnome-panel and gnome-shell.
Comment 24 Giovanni Campagna 2011-03-25 19:34:11 UTC
(In reply to comment #23)
> Created an attachment (id=184196) [details] [review]
> Patch for statusIconDispatcher.js
> 
> I'd like to think if this patch can be integrated into GNOME 3.0.
> The patch is very small at the moment.
> 
> We upstreamed the IBus program name:
> https://github.com/ibus/ibus/commit/5ef29602141945ed1255662576c2e8194af78325
> 
> Regarding to the ibus icon, GtkStatusIcon supports the image file only but not
> text.
> How about a dark icon at the moment?
> https://github.com/fujiwarat/ibus/raw/gjs/data/icons/16/ibus-keyboard.png
> Now the part of ibus was integrated in ibus-1.3.99.20110228-5.fc15.
> Probably I think we can continue to discuss the better icon during Fedora beta?
> The change effects both gnome-panel and gnome-shell.

You can use GtkNumerableIcon and gtk_status_icon_new_from_gicon()
Comment 25 Owen Taylor 2011-03-25 23:18:49 UTC
(In reply to comment #23)
> Created an attachment (id=184196) [details] [review]
> Patch for statusIconDispatcher.js
> 
> I'd like to think if this patch can be integrated into GNOME 3.0.
> The patch is very small at the moment.

Yes, I'll work on getting this in.

> We upstreamed the IBus program name:
> https://github.com/ibus/ibus/commit/5ef29602141945ed1255662576c2e8194af78325

Thanks!
 
> Regarding to the ibus icon, GtkStatusIcon supports the image file only but not
> text.
> How about a dark icon at the moment?
> https://github.com/fujiwarat/ibus/raw/gjs/data/icons/16/ibus-keyboard.png

Can you see what you think of using:

 ICON_KEYBOARD = "input-keyboard-symbolic"

using an icon from the standard gnome-icon-theme ? This will give an appearance that is best aligned with the JS indicators. Unfortunately, the icon won't be shown at a size matching the JS indicators because there is no way to pass a size over the status icon protocol, but I think it will look better than using a non-symbolic icon in any case.
Comment 26 Owen Taylor 2011-03-25 23:23:40 UTC
Review of attachment 184196 [details] [review]:

Looks safe and good, I'll fix up the commit message when pushing to something that matches our style a bit better, perhaps:

 statusIconDispatcher: add ibus-ui-gtk to standard implementations

 As a stop-gap measure until we have a native input method tray icon, add ibus-ui-gtk
 to STANDARD_TRAY_ICON_IMPLEMENTATIONS so that the IBus status icon shows up in the
 status area rather than in the message tray. The message tray location doesn't really
 work for the function of showing the current input method when switching between
 windows or changing input methods.
Comment 27 Owen Taylor 2011-03-26 01:21:54 UTC
Comment on attachment 184196 [details] [review]
Patch for statusIconDispatcher.js

Pushed statusIconDispatcher patch with GNOME release team approval
Comment 28 Takao Fujiwara 2011-04-01 04:29:56 UTC
Thanks much for the integration.
I added an internal patch of "input-keyboard-symbolic" in Fedora.

The patch is under the discussion in IBus:
https://github.com/fujiwarat/ibus/commit/c0440392863fa000225fac066af3232227f6ee0e
Probably myself think it should be good to use the theme's icon for any distributions rather than the ibus panel icon name.

I will continue to implement IBus GJS files and hope to integrate it in GNOME 3.2.
Once I'll implement the candidate window for shell, I'll update the bug report again.

I updated my blog:
http://desktopi18n.wordpress.com/2011/04/01/ibus-and-shell-for-gnome-3-0/
Comment 29 Takao Fujiwara 2011-04-01 04:34:50 UTC
(In reply to comment #24)
> > How about a dark icon at the moment?
> > https://github.com/fujiwarat/ibus/raw/gjs/data/icons/16/ibus-keyboard.png
> > Now the part of ibus was integrated in ibus-1.3.99.20110228-5.fc15.
> > Probably I think we can continue to discuss the better icon during Fedora beta?
> > The change effects both gnome-panel and gnome-shell.
> 
> You can use GtkNumerableIcon and gtk_status_icon_new_from_gicon()

Thanks for the info. I didn't know the numerable icon.
However personally I guess the "input-keyboard-symbolic" is enough for GNOME 3.0 since the gtk icon would be a workaround.
If we also should change each engine icon for GNOME 3.0, I'll try to implement it with this idea.
Comment 30 Dan Winship 2011-05-19 16:45:52 UTC
Comment on attachment 183536 [details] [review]
Patch for panel.js

not sure what the current thinking here is, but this patch is wrong anyway because you forgot to "git add" the new files
Comment 31 Takao Fujiwara 2011-05-20 01:48:29 UTC
I just waited to release GNOME 3.0. OK, I'll start to work on this patch next week.
Maybe I need to change the lookup window to the clutter base.
Comment 32 Takao Fujiwara 2011-05-24 10:04:43 UTC
(In reply to comment #18)
> > BTW, when I type chars in gnome-shell search box, the CandidatePanel is not
> > shown. Probably we need to fix it later (or create the shell base candidate
> > window?).
> 
> The CandidatePanel is a GtkWindow, thus a X window. All X11 windows but the one
> corresponding to the Shell's Clutter stage are covered while in the overview
> and not visible.
> To support the search box, and entries for notifications, and shell modal
> dialogs, you need to rewrite CandidatePanel to be a ClutterActor, and then use
> Main.chrome.addActor to make it appear.

I tried to implement CandidatePanel with ModalDialog but the CandidatePanel could not show any IM candidate strings because the strings are shown from the current input focus but ModalDialog takes the focus.
Probably I think the focus cannot be changed when CandidatePanel is shown but I'm searching any way to show CandidatePanel with the gnome-shell search box.
Comment 33 Takao Fujiwara 2011-06-01 05:35:01 UTC
Currently I try to implement XKB on IBus.
But it seems GJS cannot extract GLib.Variant.

I have two test cases:
Test Case #1:
const IBus = imports.gi.IBus;

function main() {
    let bus = new IBus.Bus();
    let config = bus.get_config();
    let value = config.get_value('general', 'xkb_latin_layouts', []);
    let name = value.dup_string(0);
    print (name);
}

main();

Test Case #2:
const Gio = imports.gi.Gio;
const GObject = imports.gi.GObject;

function main() {
    GObject.type_init();
    let settings = new Gio.Settings({ schema: 'org.gnome.desktop.interface' });
    let value = settings.get_value ('font-name');
    let name = value.dup_string(0);
    print (name);
}

main();

Both case is failed with the same error:
    JS ERROR: !!!   Exception was: Error: Unable to find module implementing foreign type GLib.Variant
    JS ERROR: !!!     stack = '("Unable to find module implementing foreign type GLib.Variant")@gjs_throw:0
Error: Unable to find module implementing foreign type GLib.Variant

Is this issue addressed in bug 622344 ?
Comment 34 Giovanni Campagna 2011-06-01 12:40:04 UTC
(In reply to comment #33)
> Both case is failed with the same error:
>     JS ERROR: !!!   Exception was: Error: Unable to find module implementing
> foreign type GLib.Variant
>     JS ERROR: !!!     stack = '("Unable to find module implementing foreign
> type GLib.Variant")@gjs_throw:0
> Error: Unable to find module implementing foreign type GLib.Variant
> 
> Is this issue addressed in bug 622344 ?

That, and bug 646635 (which removes the (foreign) marking for GLib.Variant)

(By the way, with GSettings you can use setting.get_string(), skipping the GVariant altogether)
Comment 35 Takao Fujiwara 2011-06-02 04:07:48 UTC
Created attachment 189056 [details] [review]
Patch for panel.js

I updated the ibus plugin files:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/gnome-shell-ibus-plugins-20110601.tar.bz2
 - keyboard icon is input-keyboard-symbolic
 - Added xkbLayout.js

(In reply to comment #34)
> > Is this issue addressed in bug 622344 ?
> 
> That, and bug 646635 (which removes the (foreign) marking for GLib.Variant)
> 
> (By the way, with GSettings you can use setting.get_string(), skipping the
> GVariant altogether)

OK, I see. I'll wait for the fix.
Actually I'll try to get GList in GLib.Variant and currently commented out some lines in xkbLayout.js.
Currently ibus provides ibus_config_get_value() & ibus_config_set_value().
Probably I think it would depend on when GLib.Variant will work if we add ibus_config_get_[list|string].
I had some discussions internally recently and probably I think ibus status icon shows language names, e.g. "en", when input-method engine is OFF and specific multi-byte characters when input-method engine is ON.
If input-method engine is OFF, keyboard layout only could effect the typing.
Currently Control+Space can toggle IME on and off in IBus.
If IME becomes ON, probably it's better to assign the previous keyboard layout to IME by default.
Comment 36 Takao Fujiwara 2011-07-01 08:21:49 UTC
OK, I updated the tarball:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/gnome-shell-ibus-plugins-20110630.tar.bz2

Now this can shows both keyboard layouts and input methods in the same list on ibus icon status menu using a bridge hotkey feature in ibus.
Two chars from language names are shown for keyboard layouts and a multi-byte symbol is shown for input methods.
I saved all features in gjs branch:
https://github.com/fujiwarat/ibus/commits/gjs/

Currently /usr/share/gnome-shell/extensions is available for the plugins and I'm not sure which gnome-shell or a new module has these files.
The latest ibus-1.3.99.20110419-7 in Fedora 15 has the new package of ibus-gnome3 which install the extensions directory.
Comment 37 Takao Fujiwara 2011-07-14 11:08:11 UTC
Created attachment 191950 [details] [review]
Patch for panel.js

I revised the patch to update data/theme/gnome-shell.css .
I also fixed some bugs:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/gnome-shell-ibus-plugins-20110714.tar.bz2
Comment 39 Jasper St. Pierre (not reading bugmail) 2011-08-03 20:04:44 UTC
Review of attachment 191950 [details] [review]:

You need to include the ibus files in a patch here so it is easier to review.

::: js/misc/config.js.in
@@ +19,3 @@
+const IBUS_PKGDATADIR = '@IBUS_PKGDATADIR@';
+/* The path of ibus-xkb */
+const IBUS_XKB = '@IBUS_XKB@';

You don't seem to use any of these besides HAVE_IBUS. Do you use them in the status/ibus/ files, or is this vestigial?

::: js/ui/panel.js
@@ +53,3 @@
+        STANDARD_TRAY_ICON_SHELL_IMPLEMENTATION['ibus'] = imports.ui.status.ibus.indicator.Indicator;
+} catch(e) {
+    log('IBus is not installed. Maybe you would disable i18n features during the distro installation');

Omit the try/catch. It's better that we see the actual error in the code than try to hide and log it.

::: js/ui/statusIconDispatcher.js
@@ -21,3 @@
     'kbd-numlock': 'keyboard',
-    'kbd-capslock': 'keyboard',
-    'ibus-ui-gtk': 'input-method'

This needs to be 'ibus', not 'input-method'.
Comment 40 Takao Fujiwara 2011-08-06 15:27:42 UTC
Created attachment 193348 [details] [review]
Patch for panel.js

OK, I merged the contents of the tarball into a patch.

(In reply to comment #39)
> ::: js/misc/config.js.in
> @@ +19,3 @@
> +const IBUS_PKGDATADIR = '@IBUS_PKGDATADIR@';
> +/* The path of ibus-xkb */
> +const IBUS_XKB = '@IBUS_XKB@';
> 
> You don't seem to use any of these besides HAVE_IBUS. Do you use them in the
> status/ibus/ files, or is this vestigial?

Yes, they are used by ibus gjs.

> ::: js/ui/panel.js
> @@ +53,3 @@
> +        STANDARD_TRAY_ICON_SHELL_IMPLEMENTATION['ibus'] =
> imports.ui.status.ibus.indicator.Indicator;
> +} catch(e) {
> +    log('IBus is not installed. Maybe you would disable i18n features during
> the distro installation');
> 
> Omit the try/catch. It's better that we see the actual error in the code than
> try to hide and log it.

The reason is cleared for me. The users don't install ibus.
The build requirement is different from install one. I think users need to be uninstall ibus typelib but gnome-shell should not be crashed.

> 
> ::: js/ui/statusIconDispatcher.js
> @@ -21,3 @@
>      'kbd-numlock': 'keyboard',
> -    'kbd-capslock': 'keyboard',
> -    'ibus-ui-gtk': 'input-method'
> 
> This needs to be 'ibus', not 'input-method'.

The ibus gtk icon is no longer used by this patch.

I'll provide ibus-1.3.99.20110419-13.fc16 for Fedora 16 soon.
Comment 41 Takao Fujiwara 2011-08-14 03:29:06 UTC
Created attachment 193802 [details] [review]
Patch for panel.js

Revised the patch for gnome-shell 3.1.14.
Comment 42 Jasper St. Pierre (not reading bugmail) 2011-08-14 08:09:48 UTC
Review of attachment 193802 [details] [review]:

Nits:

  * Do not add a comma after the last field in a prototype.
  * Please use camelCase for variable and method names, except for items from gobject-introspection and CONSTANT_CASE.
  * Import modules instead of importing objects/classes from them.
  * You seem to use "this._init.apply(this, arguments);" a lot. I'd explicitly pass the arguments.
  * Please use '' for non-translatable text, and "" for translatable text.

::: js/ui/status/ibus/candidatePanel.js
@@ +224,3 @@
+        this._lookup_table_visible = false; 
+        this._preedit_string = '';
+        this._preedit_attrs = new Pango.AttrList();

You don't seem to use preedit_attrs nor aux_attrs after setting them.

::: js/ui/status/ibus/common.js
@@ +20,3 @@
+
+const Gettext = imports.gettext.domain('gnome-shell');
+const _ = Gettext.gettext;

We already put "_" in the global scope (see js/ui/environment.js), so you don't need this.

::: js/ui/status/ibus/handle.js
@@ +30,3 @@
+Handle.prototype = {
+    _init: function() {
+        this._eventbox = new Gtk.EventBox();

Why are you using GTK+ instead of a Clutter widget?

::: js/ui/status/ibus/panelBase.js
@@ +69,3 @@
+
+    set_cursor_location: function(panel, x, y, w, h) {
+    },

A lot of these are empty... are they required?

::: js/ui/status/ibus/xkbLayout.js
@@ +287,3 @@
+        }
+        let args = [];
+        args[args.length] = this._command;

Use args.push here.

@@ +357,3 @@
+        }
+        if (option == 'default') {
+            this._default_option = self.get_option();

self?
Comment 43 Takao Fujiwara 2011-08-18 03:38:57 UTC
Created attachment 194097 [details] [review]
Patch for panel.js

OK, I revised the patch.

(In reply to comment #42)
> ::: js/ui/status/ibus/candidatePanel.js
> @@ +224,3 @@
> +        this._lookup_table_visible = false; 
> +        this._preedit_string = '';
> +        this._preedit_attrs = new Pango.AttrList();
> 
> You don't seem to use preedit_attrs nor aux_attrs after setting them.

I removed those parameters.

> ::: js/ui/status/ibus/handle.js
> @@ +30,3 @@
> +Handle.prototype = {
> +    _init: function() {
> +        this._eventbox = new Gtk.EventBox();
> 
> Why are you using GTK+ instead of a Clutter widget?

OK, I removed handle.js at the moment but it would be great if we could have the similar widget likes GtkEventBox (a handle GUI) to be able to move candidatePanel.CandidatePanel with mouse.

> ::: js/ui/status/ibus/panelBase.js
> @@ +69,3 @@
> +
> +    set_cursor_location: function(panel, x, y, w, h) {
> +    },
> 
> A lot of these are empty... are they required?

The class is an interface and the actual entity is panel.js which overrides set_cursor_location().
Comment 44 Jasper St. Pierre (not reading bugmail) 2011-08-18 04:55:36 UTC
Review of attachment 194097 [details] [review]:

More simple style nits:

* Please do not use a comma at the end of an object initializer.

    let foo = {
      bar: 1,
      baz: 2,
    };

    Foo.prototype = {
      bar: 1,
      baz: function() {
      },
    };

  The commas after both "baz"es are wrong.

* Do not include commented out code in your patches. If it is unfinished or broken, why bother keeping it around?

Other things:

* You seem to add a lot of functionality to methods that you never seem to use through arguments that you never pass. I've outlined some of those cases in the detailed review below. I'd remove all this extra functionality, as it makes the code more complex supporting things you do not use.

* You seem to have a bit of confusion about the JS equality operator. "foo == null" and "foo == undefined" are both testing for the exact same condition. It's likely that "!foo", while having slightly different semantics, is more in line with what you want.

::: js/ui/status/ibus/candidatePanel.js
@@ +246,3 @@
+    },
+
+    _pack_all_st_widgets: function() {

_packAllStWidgets

@@ +411,3 @@
+    setCursorLocation: function(x, y, w, h) {
+        // if cursor location is changed, we reset the moved cursor location
+        if (this._cursorLocation != [x, y, w, h]) {

You can't compare arrays this way in JavaScript.

::: js/ui/status/ibus/indicator.js
@@ +170,3 @@
+
+Signals.addSignalMethods(UIApplication.prototype);
+DBus.conformExport(UIApplication.prototype, UIApplicationIface);

You cannot use JS signals and DBus at the same time.

::: js/ui/status/ibus/languageBar.js
@@ +36,3 @@
+        this._indicator = indicator;
+
+        this._properties = [];

Having both "this._props" and "this._properties" is confusing. Add a comment (or rename one of them)

::: js/ui/status/ibus/panel.js
@@ +40,3 @@
+
+
+function Panel(bus, indicator) {

So we don't fight with js/ui/panel.js, can we rename this "IBusPanel" (and ibusPanel.js?)

@@ +103,3 @@
+        }
+
+        PanelBase.PanelBase.prototype._init.call(this, bus);

Don't split your _init into two pieces, especially if you do things like this.

@@ +142,3 @@
+
+        this._xkblayout = new XKBLayout.XKBLayout(this._config);
+        /* Currently GLib.Variant is not implemented. bug #622344 */

This bug has been fixed.

::: js/ui/status/ibus/panelBase.js
@@ +24,3 @@
+
+
+function PanelBase(bus) {

Given that you don't use this anywhere but panel.js, I don't see the advantage of this class.

::: js/ui/status/ibus/pangoAttrList.js
@@ +40,3 @@
+
+PangoAttrList.prototype = {
+    _init: function(attrs, str) {

Pango Attributes aren't GObjects, just structs. You won't be able to create them with gjs unless we add some overrides. Instead of munching on UTF8 characters, I'd suggest building Pango markup directly. It would be relatively easy if the attributes are not overlapping. If you can do it, have the IBus library give you the markup instead of an attribute list.

::: js/ui/status/ibus/propItem.js
@@ +19,3 @@
+ */
+
+function PropItem(prop) {

If this is wrapping something, please put a comment. Additionally, as far as I can tell, you don't seem to use this except as a base class for ShellMenu, so it should probably go in there.

::: js/ui/status/ibus/shellMenu.js
@@ +33,3 @@
+}
+
+ShellMenu.prototype = {

A comment with a quick overview the property system in IBus would be nice.

::: js/ui/status/ibus/xkbLayout.js
@@ +57,3 @@
+function _handleSpawnError(command, err) {
+    let title = _("Execution of '%s' failed:").format(command);
+    Main.notifyError(title, err.message);

This is a useless error message for the user.

@@ +60,3 @@
+}
+
+function _spawn_with_pipes(argv) {

_spawnWithPipes

@@ +70,3 @@
+
+function XKBLayout(config, command) {
+    this._init(config, command);

You don't ever pass "command" to the constructor.

@@ +91,3 @@
+        this._defaultModel = this.getModel();
+        this._defaultOption = this.getOption();
+        this._timeLagSessionXkbLayout = true;

A comment describing the purpose of the timer would be helpful.

@@ +106,3 @@
+    },
+
+    _getModelFromLayout: function(layout) {

Even a simple comment like:

  // "Foo (model) [option]" => "model"

would be extremely helpful. This is just a guess at the layout syntax.

@@ +116,3 @@
+    },
+
+    _getOptionFromLayout: function(layout) {

Can there be more than one option per layout?

@@ +180,3 @@
+    },
+
+    setLayout: function(layout, model, option) {

You never use the model or option parameters. It would make the code a lot simpler to junk them.

@@ +193,3 @@
+            option = 'default';
+        }
+        if (layout == null) {

When could this happen? In the case that 'layout' is false-y, it would be set to 'default'.

@@ +263,3 @@
+        }
+        let needUsLayout = false;
+        for (let latinLayout in this._xkbLatinLayouts) {

Do not use for...in loops on arrays.

@@ +305,3 @@
+    },
+
+    getDeefaultOption: function() {

"getDeefaultOption"?
Comment 45 Takao Fujiwara 2011-08-23 15:27:33 UTC
Created attachment 194491 [details] [review]
Patch for panel.js

OK, I revised the patch.

(In reply to comment #44)
> * You seem to have a bit of confusion about the JS equality operator. "foo ==
> null" and "foo == undefined" are both testing for the exact same condition.

Oh, I didn't notice null and undefined are same.

> ::: js/ui/status/ibus/xkbLayout.js
> @@ +57,3 @@
> +function _handleSpawnError(command, err) {
> +    let title = _("Execution of '%s' failed:").format(command);
> +    Main.notifyError(title, err.message);
> 
> This is a useless error message for the user.

I replaced notifyError with log.
However the original code was copied from js/misc/util.js .
Comment 46 Jasper St. Pierre (not reading bugmail) 2011-08-28 00:17:39 UTC
Review of attachment 194491 [details] [review]:

Much, much better. This looks good enough to land for now, after these small fixes.

::: js/ui/status/ibus/ibusPanel.js
@@ +242,3 @@
+                                           GLib.Variant.new_int32(0)).get_int32();
+        let orientation = Common.ORIENTATION_VERTICAL;
+        if (value in [Common.ORIENTATION_HORIZONTAL,

This Python feature does not exist in JS. Use:

  if (value == Common.ORIENTATION_VERTICAL || value == Common.ORIENTATION_HORIZONTAL)

@@ +269,3 @@
+
+    _createShellMenuForPopup: function() {
+        let item = new PopupMenu.PopupImageMenuItem(_("Preferences"),

We'll probably have to revise these strings.

@@ +306,3 @@
+        }
+
+        engine.disabledEnginesID = -1

;
Comment 47 Owen Taylor 2011-08-28 14:24:55 UTC
(In reply to comment #46)
> Review of attachment 194491 [details] [review]:
> 
> Much, much better. This looks good enough to land for now, after these small
> fixes.

I really think this needs to land at the beginning of a development cycle rather than at the very last minute, and I really want to see it synchronized with work on System Settings for proper integration of configuration.

In addition to code review, we really need to do design level review of the user interface.

I really apologize that we (the GNOME Shell team) didn't have time to work with you on this feature for this cycle, but we're going to have to wait for 3.4 for tight IBus integration at this point.
Comment 48 Matteo Settenvini 2011-09-03 11:41:54 UTC
This bug appears to be a blocker of bug 648749. If appropriate, please add it in the dependency tree.
Comment 49 Matthias Clasen 2011-10-07 00:51:50 UTC
Created attachment 198495 [details]
screenshot1
Comment 50 Matthias Clasen 2011-10-07 00:52:28 UTC
Created attachment 198496 [details]
screenshot2
Comment 51 Matthias Clasen 2011-10-07 01:00:39 UTC
I've installed ibus-gnome3 today and tried it out.
Unless the patch here is substantially different from that package, I don't think it is quite 'ready to land'. 

Issues I've seen:

1. Changing the selected language did not work. No matter what item I activate, the active language always stays the same

2. The menu shows not only the 'active engines' that are configured in ibus, but also my xkb layouts (thats the items with that keyboard icon behind them). It should not do that. We want a single list of available languages, not duplication like that. And since ibus includes xkb layouts now, it is no longer necessary to include the xkb layouts extra.

3. After login, the menu shows up in a wierd state ('no input window'). We don't want that. There should always be a global active input language.

4. There's a few extra menu items that we don't need: Restart/Quit/Preferences
   For preferences, we need to make 'Region and Language' configure the available languages that are shown in the menu. For Restart/Quit, ibus better be stable enough that they are not necessary, else we cannot rely on it.

5. The menu items currently show text that is a little too complicated:
 'en English(English(US))' should just be 'English   en'
 'ge German' should be 'German   de'
Comment 52 Matthias Clasen 2011-10-07 01:47:45 UTC
Created attachment 198498 [details]
screenshot3
Comment 53 Matthias Clasen 2011-10-07 01:50:05 UTC
Things got a little more interesting after I managed to activate Chinese.
I don't think we want any input-method-specific items like that in the menu. In particular, two 'Preferences' are bad news. If required, we can have a per-input-method 'Options' button in the 'Region and Language' panel. But not in the menu.
Comment 54 Matthias Clasen 2011-10-07 02:04:21 UTC
Created attachment 198499 [details]
screenshot4
Comment 55 Matthias Clasen 2011-10-07 02:05:46 UTC
Another thing I noticed: the candidate window is drawn by the shell as well.
Comment 56 Jasper St. Pierre (not reading bugmail) 2011-10-07 02:13:35 UTC
(In reply to comment #55)
> Another thing I noticed: the candidate window is drawn by the shell as well.

Is this a bad thing? I'd rather have one candidate window in the shell that works in the overview, etc. than one Shell one and one GTK+ one
Comment 57 Matthias Clasen 2011-10-07 02:16:33 UTC
(In reply to comment #56)
> (In reply to comment #55)
> > Another thing I noticed: the candidate window is drawn by the shell as well.
> 
> Is this a bad thing? I'd rather have one candidate window in the shell that
> works in the overview, etc. than one Shell one and one GTK+ one

No. Just an observation.
Comment 58 Matthias Clasen 2011-10-07 02:16:50 UTC
A proposal for how we get to something mergeable:

- Take the current keyboard menu, and do nothing but replace the code that gets
and sets a list of xkb layouts by code that gets and sets the active ibus
engines. No changes to the UI.

- Make sure that is working as well as the current menu for switching among xkb
layouts (represented by ibus engines).

- Then add the code for candidate window back.

- Parallel to this, we need to work on replacing the 'Layouts' tab in the
region panel with the 'Input Sources' tab that Jon mocked up. It should show
the list of ibus engines with the active engines checked, and the default
engine at the top of the list.
(https://picasaweb.google.com/111918808231494612371/InputLanguage?authkey=Gv1sRgCO3Tgo-qpZvMZw#5475781037988411826).

I may be able to help with the control-center part.
Comment 59 Takao Fujiwara 2011-10-07 09:23:49 UTC
Created attachment 198514 [details] [review]
Patch for panel.js

I forgot to update the patch with the latest.


Currently I have two problems.

One is, when I click the ibus shell status icon, the input focus is moved from the text application to the shell icon.
ibus needs to keep the focus to get the GtkIMContext on the text application.
Previously I removed 'open-state-changed' signal in open/close method in js/ui/status/ibus/popupMenu.js to keep the focus but that way don't work with the latest gnome-shell because ibus icon menu is not accessible by mouse without 'open-state-changed' signal.
Do you have any ideas to keep the input focus?

Another is, when PopupSubMenu.actor.vscrollbar_policy is Gtk.PolicyType.AUTOMATIC, gnome-shell crashes.
Do you look at the same problem?

Gtk.PolicyType.AUTOMATIC could be set when input-method-specific items are shown.


(In reply to comment #51)
> 1. Changing the selected language did not work. No matter what item I activate,
> the active language always stays the same

Probably I don't understand this problem.
Which selected language do you mean?
The default XKB layouts are decided by the order of layouts in gsettings org.gnome.libgnomekbd.keyboard and the order is changed by 'gnome-control-center region'.

> 5. The menu items currently show text that is a little too complicated:
>  'en English(English(US))' should just be 'English   en'
>  'ge German' should be 'German   de'

It's because there are the duplicated menu items, default xkb layout(en,ge,ar) and a single layout(en), with your screenshot2 so if you remove the single layout 'en' or remove 'en' in the default xkb layout(en,ge,ar), ibus can shows 'en English' only.
So probably I think #5 and #2 could be the same problem.

(In reply to comment #53)
> Things got a little more interesting after I managed to activate Chinese.
> I don't think we want any input-method-specific items like that in the menu. In
> particular, two 'Preferences' are bad news. If required, we can have a
> per-input-method 'Options' button in the 'Region and Language' panel. But not
> in the menu.

Each ibus engine has two kind of the settings.
One is the input-method-specific items on shell menu and another is the settings with /usr/libexec/ibus-setup-${engine}.

The settings in ibus-setup-${engine} are saved in disk and it could be the default settings in the engine.
The input-method-specific items are provided to change the settings by user sentences. E.g. U+61 to U+7a (ASCII) or U+FF41 to U+FF5A (fullwidth), U+3042 (Hiragana 'a') or U+30A2 (Katakana 'a').
So if the input-method-specific items are moved to gnome-control-center, I think it's not useful for users because the settings need to be accessible instantly by sentence.

Do you have any ideas?

Currently I'm thinking a sub menu for the input-method-specific items on ibus icon menu and create the two level menus:

| Chinese menu ->                  |
| ---------------- | | CN          |
| cn Chinese       | | Aa          |
| en English       | | Preferences |
| ge German        |
| ar Arabic        |

(In reply to comment #58)
> A proposal for how we get to something mergeable:
> 
> - Take the current keyboard menu, and do nothing but replace the code that gets
> and sets a list of xkb layouts by code that gets and sets the active ibus
> engines. No changes to the UI.

Where is the current keyboard menu?

> I may be able to help with the control-center part.

Thanks. It would be great.
Comment 60 Florian Müllner 2011-10-07 09:44:53 UTC
(In reply to comment #59)
> (In reply to comment #53)
> > Things got a little more interesting after I managed to activate Chinese.
> > In particular, two 'Preferences' are bad news. If required, we can have a
> > per-input-method 'Options' button in the 'Region and Language' panel. But 
> > not in the menu.
> 
> Each ibus engine has two kind of the settings.
> One is the input-method-specific items on shell menu and another is the
> settings with /usr/libexec/ibus-setup-${engine}.

But does it make a difference to the user? It sounds like an implementation detail which should not be exposed in the UI.


(In reply to comment #59)
> Where is the current keyboard menu?

It is shown when more than one keyboard layout is configured (in the "Region and Language" panel)
Comment 61 Takao Fujiwara 2011-10-07 10:25:44 UTC
(In reply to comment #60)
> (In reply to comment #59)
> > (In reply to comment #53)
> > > Things got a little more interesting after I managed to activate Chinese.
> > > In particular, two 'Preferences' are bad news. If required, we can have a
> > > per-input-method 'Options' button in the 'Region and Language' panel. But 
> > > not in the menu.
> > 
> > Each ibus engine has two kind of the settings.
> > One is the input-method-specific items on shell menu and another is the
> > settings with /usr/libexec/ibus-setup-${engine}.
> 
> But does it make a difference to the user? It sounds like an implementation
> detail which should not be exposed in the UI.

Yes, it makes a difference to the user.
Probably I don't understand why it should not be exposed.
E.g. if you think a search box, the dialog has check buttons of 'Match case' and 'Match Entire Word Only' and the settings are different by sentences. I think users expect to access the check buttons instantly instead of another GUI.

> 
> 
> (In reply to comment #59)
> > Where is the current keyboard menu?
> 
> It is shown when more than one keyboard layout is configured (in the "Region
> and Language" panel)

I mean ibus menu shows the current keyboard menu when the layouts are configured by 'gnome-control-center region'.
Comment 62 Matthias Clasen 2011-10-07 11:17:36 UTC
(In reply to comment #61)
> (In reply to comment #60)
> > (In reply to comment #59)
> > > (In reply to comment #53)
> > > > Things got a little more interesting after I managed to activate Chinese.
> > > > In particular, two 'Preferences' are bad news. If required, we can have a
> > > > per-input-method 'Options' button in the 'Region and Language' panel. But 
> > > > not in the menu.
> > > 
> > > Each ibus engine has two kind of the settings.
> > > One is the input-method-specific items on shell menu and another is the
> > > settings with /usr/libexec/ibus-setup-${engine}.
> > 
> > But does it make a difference to the user? It sounds like an implementation
> > detail which should not be exposed in the UI.
> 
> Yes, it makes a difference to the user.
> Probably I don't understand why it should not be exposed.
> E.g. if you think a search box, the dialog has check buttons of 'Match case'
> and 'Match Entire Word Only' and the settings are different by sentences. I
> think users expect to access the check buttons instantly instead of another
> GUI.

The user experience we want is 
1. you set up a list of input languages and the default input language in the control center
2. optionally, you may tweak settings of individual input languages in the control-center
3. after login, the default input language is active
4. you can switch between input languages using the shell menu

The goal is to implement that experience, or something very close to it.

The goal is not: integrate ibus, and keep all its existing options and interactions'. 

For this, the 'input languages' should be relatively simple entities that you switch between ('I'm typing in English now - switch - now I'm typing in Chinese' - switch - now I'm in  English again...).

Not complicated entities that have lots of internal state I need to keep track of while using a single one ('I'm using input language Chinese'...did I just switch back to option Aa ? Yes, but it is subtly different to real English input, so now I need to switch to input source 'English' ... When I switch back to Chinese, will Aa still be on, and which state will the other 10 options of Chinese be in ?').
Comment 63 Matthias Clasen 2011-10-07 11:22:46 UTC
> One is, when I click the ibus shell status icon, the input focus is moved from
> the text application to the shell icon.
> ibus needs to keep the focus to get the GtkIMContext on the text application.

Why ? We want the input language to be a global, session-wide setting. Not anything that has a relation to the currently focused text entry. When I move the focus into a text entry, it should use the current input language. It would be awkward to have to move all the way up to the shell menu to select the input language after I moved the focus to the entry where I want to type. For that, there should be 'Previous Input Language'/'Next Input Language' keyboard shortcuts.
Comment 64 Takao Fujiwara 2011-10-11 10:18:12 UTC
(In reply to comment #62)
> 

I thought our goal is to implement the similar UI with MS-Windows, Mac OS X, ChoromeOS.

> The goal is to implement that experience, or something very close to it.

Hmm.., currently I'm thinking the IME specific menu items are very close to users.
Did you see the GUI on MS-Windows or Mac OS X?
The current my patch is similar with Mac OS X.

Mac OS X shows the IME specific menu items:
http://arata.page.ne.jp/pinyin/

MS-Windows shows the multiple icon status buttons for languages and IME specific menus:
http://www.tjf.or.jp/ringo/migaku/post-22.php

ChromeOS has the similar implementation with Mac OS X.

Do you like to minimize the menu items against Mac OS X and MS-Windows' ones?
I guess the DPI might be different and the shell menu would be long.
Note: IME specific menus are shown for the specific input methods only (ja, zh, ko, in). If you choose 'en' keyboard layout, it does not need any IME specific menus.

Generally I think the IME specific menu items need to be accessible instantly besides the command line for the usability to sync other platforms.
But if you don't, probably I think it's a new idea and I'm not sure if users will accept it but I can follow your suggestion.


(In reply to comment #63)
> > One is, when I click the ibus shell status icon, the input focus is moved from
> > the text application to the shell icon.
> > ibus needs to keep the focus to get the GtkIMContext on the text application.
> 
> Why ? We want the input language to be a global, session-wide setting. Not
> anything that has a relation to the currently focused text entry. When I move
> the focus into a text entry, it should use the current input language. It would
> be awkward to have to move all the way up to the shell menu to select the input
> language after I moved the focus to the entry where I want to type. For that,
> there should be 'Previous Input Language'/'Next Input Language' keyboard
> shortcuts.

OK, I understood you'd like to remove the focus level settings but move to the global settings completely.
I will change the ibus behavior.
Comment 65 Matthias Clasen 2011-10-17 17:48:14 UTC
Can you give me some explanation of what these IME-specific menu items do, and why they are important for users ?
Comment 66 Jens Petersen 2011-10-19 07:58:59 UTC
Perhaps I can try.

Basically since IMEs are complicated they have various modes,
which users need to switch between for different types of input.

Eg in Japanese we have input for hiragana, katakana, wide Latin
characters, etc.  They need to be selectable from the IME menu.
Similarly there are features like dictionary tools and
OSK access, and configuration which are all traditionally
appearing in the IME menu (or toolbar as on Windows).

Note they would only be visible to users of IMEs.
GNOME users who are not using ibus, say, will never see them.
Additionally in this context I trying to see if we can do
some cleanup and improvements to some of the IME menu entries.
I think some of the basic, frequently used IME input modes
can be treated as sub-IMEs (or pseudo IMEs) perhaps
for a cleaner interface.  So perhaps some rationalization
is possible but we can't get rid of the IME menus completely:
in contrast look at the large menu being used in MacOS.
However I think we should try to use submenus less in
the IME menu since it leads to poorer usability for users IMHO.
(Originally these were "ported" from the ibus language bar
popup menus), but I don't feel submenus are optimal
for frequently used input modes.)

I hope this helps to make the need a little clearer. :)
Comment 67 Jens Petersen 2011-10-19 08:53:17 UTC
BTW I think this bug should really by renamed to something
like "Integration of keyboard indicator with input methods".


I had another comment/idea that might help:

Perhaps we could move the IME menu to a separate
ibus IME status indicator, this will leave the main
"keyboard" input indicator cleaner and simpler.

Users would still select a Japanese IME say
from the keyboard/input indicator but
any additional features like changing
IME modes etc would be handled by ibus
in an adjacent IME status indicator.

I think this compromise could satisfy all sides
and actually give a cleaner UI/UX to GNOME IM users. :)

(This is still a conceptual idea - not doubt it
will need discussion with ibus upstream also,
but I feel it might work well.)
Comment 68 Matthias Clasen 2011-10-20 12:33:31 UTC
(In reply to comment #66)
> Perhaps I can try.
> 
> Basically since IMEs are complicated they have various modes,
> which users need to switch between for different types of input.
> 
> Eg in Japanese we have input for hiragana, katakana, wide Latin
> characters, etc.  They need to be selectable from the IME menu.
> Similarly there are features like dictionary tools and
> OSK access, and configuration which are all traditionally
> appearing in the IME menu (or toolbar as on Windows).

I understand that this is how things have been traditionally done. However, we are not just trying to wedge the traditional setup into the shell here, so some adjustments will be necessary. 

Lets look at these individually:

- hiragana/katakana/latin 'modes': Is there any reason why these need to be modes of a single input method, rather than different input languages that you switch between ? Having such modes leads to a two-level model where you first have to select the right IM, and then select the right mode. The ideal we were going for here was to do away with this two-level selection, and just have input languages that can be switched between.

In particular the latin mode leads to the situation where you have the regular 'English' input language and then you have this wierd 'English-through-a-japanese-im'. That is really not a great situation. Admittedly, Japanese computer users may be used to this state of affairs, but why not improve it, rather than perpetuate it ?

- 'need to be selectable from the IME menu': This is just an unsubstantiated claim, as far as I am concerned. Isn't it far more common to use shortcuts for such mode switches ? Shortcuts would also be far more convenient when you are typing things already. To reach the menu, you have to move your hand off the keyboard, to the mouse, then navigate all the way from the app in which you are typing to the top of the screen, open the menu, select a mode, go all the way back, move your hand back to the keyboard, and start typing again. 

Even if shortcuts are not sufficient, we should find a way to expose this functionality close to the spot.

- 'features like dictionary tools': This is somewhat interesting. Do you mean that you just launch a regular dictionary app ? In that case, I'd say app launching is well covered in gnome-shell already and has no place in the input menu. Or do you mean tightly integrated dictionary completion for the current input. In that case, wouldn't you rather want that in the completion window that pops up close to the spot where you are editing ?

- 'and OSK access': gnome-shell has an integrated OSK now. And it is accessible from the universal access menu already. We cannot have an item in the menu right next to it popping up another OSK. It is quite possible (likely, in fact) that the current gnome-shell OSK is not great for languages that need IMs, and I would really encourage you to take a look at it and make some proposals for additional features we need there to make the shell OSK work well with e.g Japanese).

> 
> I hope this helps to make the need a little clearer. :)

Thanks for the input. Lets keep this discussion going, I feel that we are making progress here.
Comment 69 Matthias Clasen 2011-10-23 02:45:18 UTC
I've spent a day implementing a prototype 'input sources' tab for the control-center region panel, see bug 662489.

I've noticed a few problematic things while doing so:

- There is a big overlap in the set of engines that ibus offers. Most xkb keyboard layouts also show up as an m17n:kbd variant. We almost certainly only want to show one of these.

- The previous/next engine shortcuts don't seem to work. At least, I was totally mystified as to what happens when I hit them. Looking at the ibus panels popup to track the active engine, I see the 'next' shortcut work for a while, but then it gets stuck in the xkb layouts (why are these listed there, anyway ?). And the 'previous' shortcut seemed to just toggle an input method on and off, without ever going to a prevous one. What we want here is a really simple, reliable way to circle through the active engines, without special cases.

- The ibus-setup tool also has keyboard shortcuts for turning ims on and off. That adds an extra confusion. With xkb layouts integrated in the list of active engines, it doesn't make much sense to 'turn off' input methods - which layout will you get when you do that ?
Comment 70 Takao Fujiwara 2011-10-24 11:58:56 UTC
(In reply to comment #68)
> - hiragana/katakana/latin 'modes': Is there any reason why these need to be
> modes of a single input method, rather than different input languages that you
> switch between ? Having such modes leads to a two-level model where you first
> have to select the right IM, and then select the right mode. The ideal we were
> going for here was to do away with this two-level selection, and just have
> input languages that can be switched between.
> 
> In particular the latin mode leads to the situation where you have the regular
> 'English' input language and then you have this wierd
> 'English-through-a-japanese-im'. That is really not a great situation.
> Admittedly, Japanese computer users may be used to this state of affairs, but
> why not improve it, rather than perpetuate it ?

The idea of different input methods per hiragana/katakana/latin is interesting.
I think the current spec is to show language names only if the languages are not duplicated.
If I think your suggestion, probably I think input methods shows duplicated languages. e.g.

en English
ja Japanese Anthy Hiragana
ja Japanese Anthy Katanaka
ja Japanese Anthy Half Katakana
ja Japanese Anthy Wide Latin
ja Japanese SKK Hiragana
ja Japanese keyboard layout
cn Chinese ....

It would be a verbose list, isn't it?

The latin is paired with wide latin (U+FF21 -).
ja IM engine also provides katakana (U+30A1 -) and half katakana (U+FF66 -)
I'd think switching half and wide chars is useful for users on GUI.

> - 'need to be selectable from the IME menu': This is just an unsubstantiated
> claim, as far as I am concerned. Isn't it far more common to use shortcuts for
> such mode switches ? Shortcuts would also be far more convenient when you are
> typing things already. To reach the menu, you have to move your hand off the
> keyboard, to the mouse, then navigate all the way from the app in which you are
> typing to the top of the screen, open the menu, select a mode, go all the way
> back, move your hand back to the keyboard, and start typing again. 

Yes, shortcuts are useful but probably I think it's hard to remember the shortcuts for many people.
If you run /usr/libexec/ibus-setup-anthy , the 'Key binding' tab shows the shortcut keys. It provides F6 - F10 keys to convert hiragana/katakana/... .
In fact, I don't remember the shortcut keys :).

> Even if shortcuts are not sufficient, we should find a way to expose this
> functionality close to the spot.

Currently I have no good idea.
Previously ibus provides language bar when IME has been enabled. Now the IME specific buttons are integrated as IME menu items on ibus indicator menu.
Probably we don't prefer to go back the previous languagebar.

Personally I may like the two indicators; one is for the selection of input methods and keymaps, another is for the IME specific menu upon the IME's request. MS-IME also have the similar icons.

Anthy also provides a menu item of converting by words or by whole the sentence.

Anthy also provides to select the different dictionaries. Two are system dictionaries and others are user dictionaries.
Probably I think different dictionaries have the two reasons.
One is for the different conversions and another is for the performance to convert the chars.

> - 'features like dictionary tools': This is somewhat interesting. Do you mean
> that you just launch a regular dictionary app ? In that case, I'd say app
> launching is well covered in gnome-shell already and has no place in the input
> menu. Or do you mean tightly integrated dictionary completion for the current
> input. In that case, wouldn't you rather want that in the completion window
> that pops up close to the spot where you are editing ?

Yes, maybe it's nice to have. The launching dict tool and selecting dicts are different menu item.

> - 'and OSK access': gnome-shell has an integrated OSK now. And it is accessible
> from the universal access menu already. We cannot have an item in the menu
> right next to it popping up another OSK. It is quite possible (likely, in fact)
> that the current gnome-shell OSK is not great for languages that need IMs, and
> I would really encourage you to take a look at it and make some proposals for
> additional features we need there to make the shell OSK work well with e.g
> Japanese).

I think there is no alternative of ibus-handwrite which search the char from mouse-writing and output the chars.
Personally I may not think the OSK menu per engine but the global menu items.
Comment 71 Daiki Ueno 2011-10-25 07:44:26 UTC
(In reply to comment #69)

> I've noticed a few problematic things while doing so:
> 
> - There is a big overlap in the set of engines that ibus offers. Most xkb
> keyboard layouts also show up as an m17n:kbd variant. We almost certainly only
> want to show one of these.

I added a patch to hide m17n:kbd variants:
https://github.com/ueno/ibus-m17n/commit/583629de
however I think we will eventually need to go through the xkb /
m17n engines to see which ones should be blacklisted.

(In reply to comment #68)

> - 'and OSK access': gnome-shell has an integrated OSK now. And it is accessible
> from the universal access menu already. We cannot have an item in the menu
> right next to it popping up another OSK. It is quite possible (likely, in fact)
> that the current gnome-shell OSK is not great for languages that need IMs, and
> I would really encourage you to take a look at it and make some proposals for
> additional features we need there to make the shell OSK work well with e.g
> Japanese).

I personally think, in the context of IME, typical users would rather want a character map (like gucharmap) than actual OSK, to browse characters in the current input language/script.  Actually, in the first link in comment #64 (Mac OS), there are three menu items - "Show hiragana-only character palette", "Show keyboard viewer", and "Show character palette".
Comment 72 Allan Day 2011-11-04 12:43:12 UTC
(In reply to comment #70)
> (In reply to comment #68)
...
> The idea of different input methods per hiragana/katakana/latin is interesting.
> I think the current spec is to show language names only if the languages are
> not duplicated.
> If I think your suggestion, probably I think input methods shows duplicated
> languages. e.g.
> 
> en English
> ja Japanese Anthy Hiragana
> ja Japanese Anthy Katanaka
> ja Japanese Anthy Half Katakana
> ja Japanese Anthy Wide Latin
> ja Japanese SKK Hiragana
> ja Japanese keyboard layout
> cn Chinese ....
> 
> It would be a verbose list, isn't it?

Is it typical for someone to use all of these modes, or do they tend to use a subset of them? How frequently do people tend to switch between them?
Comment 73 Jens Petersen 2011-11-09 12:44:55 UTC
I think Mathias' prototype looks promising and a good start.

(In reply to comment #72)
> > ja Japanese Anthy Hiragana
> > ja Japanese Anthy Katanaka
> > ja Japanese Anthy Half Katakana
> > ja Japanese Anthy Wide Latin

These 4 are all commonly used - in the above order of frequency I would say,
Hiragana being the default mode.  Personally I tend to convert
Hiragana input to Katakana, etc.  But it also seems unthinkable
not to have these mode easily selectable by mouse - I think
that is an absolute minimum, of course whether they are exposed
by Gnome or iBus is another issue.

Currently ibus-anthy's Dictionary Add entry just runs "kasumi --add"
AFAICT, which grabs the current X selection.
Comment 74 Allan Day 2011-11-11 11:21:46 UTC
(In reply to comment #73)

Thanks for the info, Jens.

Having explored this issue, Matthias's approach does seem to have a number of advantages.

 * It means that users can switch between say, Hiragana and Katanaka, using the same shortcuts that they use to change between the rest of their input methods. This makes things much easier for them, since it is less to remember and simpler to operate.

 * Users only have the input methods (or modes) in the menu that are interesting to them. There will be nothing there that they don't use.

 * Presenting each of the existing 'modes' as separate input methods works rather well with the mental model we're pushing for input sources, where each input source is (roughly speaking) a single method of entering text.

It's worth noting that this approach is close to how OS X presents input methods.
Comment 75 Murray Cumming 2011-11-14 19:30:17 UTC
I filed bug #664065 about the icon's menu not working in overview mode.
Comment 76 Murray Cumming 2011-11-14 19:37:29 UTC
(In reply to comment #10), Owen wrote:
> There's no way we can provide a good experience supporting IBus and also other
> input methods.

Does this mean that you propose removing (or strongly discouraging the installation of) the non-IBus GtkIMContext immodules:
http://git.gnome.org/browse/gtk+/tree/modules/input ?
Comment 77 Jens Petersen 2011-11-15 09:05:00 UTC
It seems Chinese users want to be able to switch easily
between width and normal punctuation, etc - and Japanese
users need to be able to access narrow katakana and wide
Latin characters so I think we will still need an input
method menu below the list of different input sources
of the input indicator menu.
Comment 78 Allan Day 2011-11-15 09:55:06 UTC
Hi Jens,

(In reply to comment #77)
> It seems Chinese users want to be able to switch easily
> between width and normal punctuation, etc - and Japanese
> users need to be able to access narrow katakana and wide
> Latin characters

I agree. This is important functionality for these users.

> so I think we will still need an input
> method menu below the list of different input sources
> of the input indicator menu.

Won't an extra menu make it more difficult to access these modes though? It means users will need to learn another keyboard shortcut to switch between them. If they are using a mouse, it will mean these modes will be one step (and potentially one click) further away when they open the menu.

Ping me if you want to discuss.
Comment 79 Takao Fujiwara 2011-11-16 02:33:21 UTC
Sorry for the late response.
Currently I'm thinking a modal dialog might be useful, which is launched by a menu item on ibus indicator menu because the current implementation would make ibus menu too long.
Recently I fixed some bugs so I'd try to update the patch in this week.
Comment 80 Takao Fujiwara 2011-11-18 11:31:09 UTC
Created attachment 201656 [details] [review]
Patch for panel.js

I updated the patch to add a modal dialog for IME menu items.
I also updated Fedora 16 ibus.
Comment 81 Takao Fujiwara 2011-11-29 04:20:25 UTC
Created attachment 202345 [details] [review]
Patch for panel.js

Revised the patch.
I built ibus-1.4.0-13.fc16/fc17 with this patch in Fedora
But it's not available in updates-testing yet.

That build fixes some of your request:
1. Enabled ibus-dconf
Now Fedora ibus switched to use dconf.

2. Provided the IME setup.
Now ibus-setup has a button to launch each IM engine's setup dialog and the patch is also upstreamed in ibus.


(In reply to comment #51)
> 1. Changing the selected language did not work. No matter what item I activate,
> the active language always stays the same

I fixed some bugs. Is this issue still happened?

> 4. There's a few extra menu items that we don't need: Restart/Quit/Preferences
>    For preferences, we need to make 'Region and Language' configure the
> available languages that are shown in the menu. For Restart/Quit, ibus better
> be stable enough that they are not necessary, else we cannot rely on it.

I removed those menu items.

(In reply to comment #69)
> - The previous/next engine shortcuts don't seem to work. At least, I was
> totally mystified as to what happens when I hit them. Looking at the ibus
> panels popup to track the active engine, I see the 'next' shortcut work for a
> while, but then it gets stuck in the xkb layouts (why are these listed there,
> anyway ?). And the 'previous' shortcut seemed to just toggle an input method on
> and off, without ever going to a prevous one. What we want here is a really
> simple, reliable way to circle through the active engines, without special
> cases.

I think this is fixed.

> - The ibus-setup tool also has keyboard shortcuts for turning ims on and off.
> That adds an extra confusion. With xkb layouts integrated in the list of active
> engines, it doesn't make much sense to 'turn off' input methods - which layout
> will you get when you do that ?

OK, I removed the Enable/Disable labels on ibus-setup.

I will try to enhance several ibus XKB issues.
Comment 82 Takao Fujiwara 2011-11-29 04:24:02 UTC
(In reply to comment #81)
> Created an attachment (id=202345) [details] [review]
> Patch for panel.js
> 
> Revised the patch.
> I built ibus-1.4.0-13.fc16/fc17 with this patch in Fedora
> But it's not available in updates-testing yet.
> 
> That build fixes some of your request:
> 1. Enabled ibus-dconf
> Now Fedora ibus switched to use dconf.
> 
> 2. Provided the IME setup.
> Now ibus-setup has a button to launch each IM engine's setup dialog and the
> patch is also upstreamed in ibus.
> 

The patch also ignores 'setup' property so if all names of IM engines' setup dialogs are changed to 'setup', the menu item won't be shown on ibus shell menu.
Currently ibus-anthy's property is 'setup'.
Comment 83 Takao Fujiwara 2012-02-04 05:21:23 UTC
Created attachment 206748 [details] [review]
Patch for panel.js

I revised the patch. I think now almost issues are fixed.
I also updated Fedora rawhide ibus today.

- The indicator works without ibus-daemon
- ibus lookup window was enhanced to follow the suggested design.
- New ibus Control+Space window was integrated. It works likes Control+Tab.

https://github.com/fujiwarat/ibus/tree/gtk3-vala
https://github.com/fujiwarat/ibus-gjs/tree/gtk3-vala
Comment 84 Rui Matos 2012-03-16 00:38:34 UTC
This is just a short (and belated) status update:

I was finally able to run this patch on the latest gnome-shell and the latest ibus bits. It's looking good and moves us closer to the intended design but there are still things to do and lots of rough edges like having 2 language menus in the top panel and having repeated keyboard layouts listed there.

So, it goes without saying that this won't unfortunately make it to gnome 3.4, but we certainly want to land a working implementation as close to the designers mockups as possible for 3.6.

Takao, thank you for your work here. I'll follow up with you on IRC on how we should proceed going forward. For a start I think it's best if we create a branch on gnome-shell's git repository and do the development there trying to keep patches logically self contained which will be both easier to review and to merge to the mainline later on.
Comment 85 Rui Matos 2012-05-29 17:50:42 UTC
Created attachment 215202 [details] [review]
status/keyboard: Port to the new input sources settings
Comment 86 Rui Matos 2012-05-29 17:51:02 UTC
Created attachment 215203 [details] [review]
status/keyboard: Add support for IBus input sources
Comment 87 Rui Matos 2012-05-29 17:51:10 UTC
Created attachment 215204 [details] [review]
candidatePanel: A candidate popup for IBus input methods

This is an implementation of the org.freedesktop.IBus.Panel API which
shows a shell style popup (BoxPointer) when using an IBus input
method.

The original code is from the ibus-gjs project[1] with minor
modifications.

[1] https://github.com/fujiwarat/ibus-gjs
Comment 88 Rui Matos 2012-05-29 17:51:18 UTC
Created attachment 215205 [details] [review]
candidatePanel: Remove hardcoded styling

The styling should be done in the CSS file.
Comment 89 Rui Matos 2012-05-29 17:51:27 UTC
Created attachment 215206 [details] [review]
candidatePanel: Remove the PopupSeparatorMenuItem copy

No reason to not use the real deal.
Comment 90 Rui Matos 2012-05-29 17:51:36 UTC
Created attachment 215207 [details] [review]
candidatePanel: Use the Lang.Class framework
Comment 91 Rui Matos 2012-05-29 17:56:07 UTC
Created attachment 215208 [details] [review]
candidatePanel: A candidate popup for IBus input methods

--
Sorry for the churn, I forgot to squash the candidatePanel patches I
had locally.
Comment 92 Florian Müllner 2012-05-31 18:27:26 UTC
Review of attachment 215202 [details] [review]:

::: configure.ac
@@ +124,3 @@
 PKG_CHECK_MODULES(TRAY, gtk+-3.0)
 PKG_CHECK_MODULES(GVC, libpulse libpulse-mainloop-glib gobject-2.0)
+PKG_CHECK_MODULES(DESKTOP_SCHEMAS, gsettings-desktop-schemas >= 3.5.1)

Just a reminder: please make sure that the module is actually bumped (so you don't require a non-existent version)

::: js/ui/status/keyboard.js
@@ -102,2 @@
-    _syncConfig: function() {
-        this._showFlags = this._config.if_flags_shown();

BTW - yay for killing a "but we've had it for years in GNOME 2.x!!!1!" option

@@ +78,3 @@
+        let newCurrent = this._settings.get_uint(KEY_CURRENT_INPUT_SOURCE);
+
+        if (newCurrent < nSources) {

You could save braces/indentation by doing

if (newCurrent >= nSources)
    return;

(obviously feel free to ignore :)

@@ +111,3 @@
+
+        for (let i = 0; i < nSources; ++i) {
+            let [, , shortName, , ] = this._xkbInfo.get_layout_info(sources[i]);

Is

let shortName = this._xkbInfo.get_layout_info(sources[i])[2];

more readable?

@@ +117,2 @@
             else
+                uniqueShortNames[shortName] = { count: 0, subscript : 0 };

Ugh.

if (uniqueShortNames[shortName].count == 0)
  log("There's *one* source with short name " + shortName);

To be honest though, I don't really like the subscript property either - would it be clearer to do something like the following?

  let infos = [];
  let infosByShortName = {};

  for (let i = 0; i < nSources; i++) {
      let info = {};
      [info.exists, info.displayName, info.shortName, , ] = this._xkbInfo.get_layout_info(sources[i]);

      if (!(info.shortName in infosByShortName))
          infosByShortName[info.shortName] = [];
      infosByShortName[info.shortName].push(info);
      infos.push(info);
  }

  for (i = 0; i < infos.length; i++) {
      let info = infos[i];
      if (infosByShortName[info.shortName].length > 1) {
          let sub = infosByShortName[info.shortName].indexOf(info) + 1;
          info.shortName += String.fromCharCode(0x2080 + sub);
      }

      ...
  }

@@ +120,3 @@
+
+        for (let i = 0; i < nSources; ++i) {
+            let [xkbInfoExists, displayName, shortName, , ] = this._xkbInfo.get_layout_info(sources[i]);

Nit: xkbInfoExists is not used (yet) - though actually, there's a pretty good use case for it already:

Currently, you add a blank menu item for each non-xkb source (with the next patch: in case ibus is missing). So please add

if (!xkbInfo)
  continue;

(I guess the check for whether to show the keyboard item should be adjusted as well then).

@@ +145,3 @@
+            this._currentInputSourceChanged();
+        else
+            this._current = 0;

Is there a good reason to not have the else{} part in _currentInputSourceChanged()?

@@ +153,3 @@
+        let sources = this._settings.get_value(KEY_INPUT_SOURCES).get_strv();
+        let current = this._settings.get_uint(KEY_CURRENT_INPUT_SOURCE);
+        let [xkbInfoExists, , , xkbLayout, xkbVariant] = this._xkbInfo.get_layout_info(sources[current]);

xkbInfoExists again :)

(Here I don't see how the variable is useful in *this* patch, so I suggest moving it to the next one)
Comment 93 Florian Müllner 2012-05-31 18:28:04 UTC
Review of attachment 215203 [details] [review]:

Commit message could use a body, code looks good.

::: js/ui/status/keyboard.js
@@ +156,3 @@
 
+            if (!xkbInfoExists)
+                if (IBus && this._ibus) {

I'd prefer

  this._ibus = null;

in _init(), and then a simple

  if (this._ibus)

here.

@@ +192,3 @@
 
+        if (!xkbInfoExists)
+            if (IBus && this._ibus) {

... and here
Comment 94 Florian Müllner 2012-05-31 18:28:32 UTC
Review of attachment 215208 [details] [review]:

OK, so I really hate this :)

I'm aware that this is "foreign" code, and I'm OK with leaving the patch as-is and doing modifications on top, but I really want follow-up patches to land together with the main patch.

::: js/Makefile.am
@@ +85,3 @@
 	ui/statusIconDispatcher.js	\
 	ui/status/accessibility.js	\
+	ui/status/candidatePanel.js	\

I guess you put this in status/ because it's only used from the keyboard item, but I still don't like it - I'd say either put the code directly into keyboard.js or move the file to ui/ (in the latter case, it might make sense to call it ibusCandidatePanel or something, without the prefix it sounds like some election event to an ignorant westener like me :)

::: js/ui/status/candidatePanel.js
@@ +17,3 @@
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.

We hardly ever use copyright notices in js files - on the other hand, the file has been adapted from a different code base, so the notice contains information not in git-log, so maybe it is a good idea to have it. Dunno.

@@ +36,3 @@
+
+const StCandidateArea = new Lang.Class({
+    Name: 'StCandidateArea',

Unless 'St' stands for anything other than "shell toolkit" here, the namespace prefix is completely wrong and should be dropped

@@ +56,3 @@
+        let hbox = null;
+        if (this._orientation == ORIENTATION_VERTICAL) {
+            vbox = new St.BoxLayout({ vertical: true,

I really don't see why vertical and horizontal code paths need to be *that* different (what are those freaking bins for?). I'd rather see something along the lines of (rough sketch):

this.actor = new St.BoxLayout({ vertical: this:_orientation == ORIENTATION_VERTICAL });

for (let i = 0; i < 16; i++) {
    let candidateBox = new St.BoxLayout ();
    this.actor.add_child(candidateBox);

    let numButton = new St.Button({ label: '%x'.format(i) });
    let textButton = new St.Button();
    candidateBox.add_child(numButton);
    candidateBox.add_child(textButton);
}

setOrientation() can then just update this.actor.vertical. If there are actual style differences, it should be possible to add/remove appropriate classes on this.actor and use selectors like

.candidate-area-vertical .candidate {}

@@ +73,3 @@
+        }
+        for (let i = 0; i < 16; i++) {
+            let label1 = new St.Label({ text: '1234567890abcdef'.charAt(i) + '.',

Ugh. '%x'.format(i)

@@ +94,3 @@
+                                          x_fill: false,
+                                          y_fill: true
+                                        });

add_child() only takes one argument, so the child properties parameter is actually ignored

@@ +111,3 @@
+            }
+
+            this._labels.push([label1, label2]);

I really dislike the multidimensional array here. Can we please use something like

this._labels.push({ numLabel: label1, textLabel: label2 });

(preferably with better names than _labels, numLabel and textLabel)

@@ +124,3 @@
+                widget.connect('enter-event',
+                               function(widget, event) {
+                                   widget.add_style_pseudo_class('hover');

Argggh, please - just use Buttons instead of Labels!

@@ +143,3 @@
+                  widget.candidateIndex,
+                  event.get_button(),
+                  event.get_state());

Oh, so that's why we're using labels instead of buttons - ok then, or maybe it makes sense to add modifierMask to StButton::clicked?
(Apart from the whole click/hover/etc stuff not working anyway due to wrong StageInputMode anyway)

@@ +157,3 @@
+            /* Use a ClutterActor attribute of Shell's theme instead of
+             * Pango.AttrList for the lookup window GUI and
+             * can ignore 'attrs' simply from IBus engines?

I have no idea what that's supposed to mean

@@ +172,3 @@
+        }
+        if (candidates.length > this._labels.length) {
+            assert();

WTF?! Even if assert() *was* an actual method, "oh, the input method returned too many suggestions, let's kill the window manager" is just wrong

@@ +202,3 @@
+    },
+
+    showAll: function() {

Pointless method, this.actor is public (same for hideAll)

@@ +217,3 @@
+    _init: function() {
+        this._orientation = ORIENTATION_VERTICAL;
+        this._currentOrientation = this._orientation;

For the life of it, I'm not able to figure out what the difference between "orientation" and "currentOrientation" is supposed to be - can we please get rid of one

@@ +230,3 @@
+    },
+
+    _initSt: function() {

Again, too many 'st's around - let's just get rid of those (there's a load of others as well) (not sure why the split between _init() and _initSt() would make sense in the first place, but feel free to just rename to initUI or something instead)

@@ +240,3 @@
+        this.actor._delegate = this;
+        this.actor.style_class = 'popup-menu-boxpointer';
+        this.actor.add_style_class_name('popup-menu');

I seriously dislike all the "make it look like a menu, but don't use one" stuff - just use a PopupMenu (and get rid of all the duplicated show-hide-separator code and what not). This means that CandidateArea should probably extend BaseMenuItem.

@@ +280,3 @@
+    },
+
+    _packAllStWidgets: function() {

Why is that a method on its own?

@@ +387,3 @@
+        for (let i = 0; this._lookupTable.get_label(i) != null; i++) {
+            let label = this._lookupTable.get_label(i);
+            newLabels.push([label.get_text(), label.get_attributes()]);

See comment for _refreshCandidates() below

@@ +420,3 @@
+            let candidate = candidates[i];
+            newCandidates.push([candidate.get_text(),
+                                candidate.get_attributes()]);

What? So _getCandidatesInCurrentPage() gets us an array of "candidate" objects, which we than iterate to get an array of arrays (with the first element being one property of the candidate object and the second one another property)? Can we please just pass around the proper objects?

@@ +471,3 @@
+    },
+
+    cursorUpLookupTable: function() {

Yay, "press <up> to move <left>" is back! </sarcasm>

Anyway, that appears to be completely ibus' fault, so nothing we can do I guess :(

@@ +529,3 @@
+    },
+
+    setOrientation: function(orientation) {

Looks unused.

@@ +534,3 @@
+    },
+
+    getCurrentOrientation: function() {

Unused.

@@ +564,3 @@
+    showAll: function() {
+        if (!this._isVisible) {
+            this.actor.opacity = 255;

I don't see why showAll()/hideAll() cannot be replaced by this.actor.show()/this.actor.hide()

@@ +579,3 @@
+
+    move: function(x, y) {
+        this.actor.set_position(x, y);

this.actor is public, so this method is pointless.

::: js/ui/status/keyboard.js
@@ +108,3 @@
+    },
+
+    _ibusInitCandidatePanel: function() {

_initIbusCandidatePanel()/_initIbusPanelService() etc. makes more sense to me (or "IM" instead of "Ibus" because it reads nicer - it'll also make it easier to port to fcitx (/me hides))

@@ +221,3 @@
+
+    focusIn: function(panel, path) {
+    },

...
Comment 95 Rui Matos 2012-06-01 16:06:57 UTC
Created attachment 215441 [details] [review]
status/keyboard: Port to the new input sources settings

--
I think I addressed all your comments here. The patch was also updated
for a change in the settings schema.
Comment 96 Rui Matos 2012-06-01 20:28:33 UTC
Comment on attachment 215441 [details] [review]
status/keyboard: Port to the new input sources settings

Florian acked this on IRC with a fix to not show the indicator when
there's 2 or more sources configured but only 1 could be visible.

Attachment 215441 [details] pushed as ec0730f - status/keyboard: Port to the new input sources settings
Comment 97 Takao Fujiwara 2012-06-08 09:47:35 UTC
Created attachment 215924 [details] [review]
Patch for IME menu items

I think the reviewed patches lose the IME menu items.
This patch is applied over attachment #215208 [details].
I'd also like to port the IME switcher dialog (switcher.js) from my git but I'm not sure how the keyboard indicator could get the key press events If the IME switching is handled by gnome-settings-daemon.
Comment 98 Matthias Clasen 2012-06-08 11:35:30 UTC
ime switcher dialog, what is that ? not sure we want that, now that we have a unified approach to input sources.
Comment 99 Murray Cumming 2012-06-08 11:55:32 UTC
(In reply to comment #76)
> (In reply to comment #10), Owen wrote:
> > There's no way we can provide a good experience supporting IBus and also other
> > input methods.
> 
> Does this mean that you propose removing (or strongly discouraging the
> installation of) the non-IBus GtkIMContext immodules:
> http://git.gnome.org/browse/gtk+/tree/modules/input ?

Do you have an answer for this now, please? Things seem to be more decided now based on the recent desktop-devel-list thread, but it would be good to know some hard facts.
Comment 100 Matthias Clasen 2012-06-09 01:30:11 UTC
(In reply to comment #99)

> > Does this mean that you propose removing (or strongly discouraging the
> > installation of) the non-IBus GtkIMContext immodules:
> > http://git.gnome.org/browse/gtk+/tree/modules/input ?
> 
> Do you have an answer for this now, please? Things seem to be more decided now
> based on the recent desktop-devel-list thread, but it would be good to know
> some hard facts.

What answer do you expect ?

Yes, we don't believe that individual immodules like the ones you point at make sense in addition to a well-integrated input method framework like ibus. So yes, we strongly discourage installing them - which is why they are separately packaged and not in the default install in Fedora.

That being said, the immodule facility in GTK+ is still around and is useful to have - e.g. it allows GTK+ applications to integrate in a different im framework when running under a different desktop like, say, kde.
Comment 101 Takao Fujiwara 2012-06-11 02:02:50 UTC
(In reply to comment #98)
> ime switcher dialog, what is that ? not sure we want that, now that we have a
> unified approach to input sources.

I have already integrated it in both ibus and ibus-gnome3 package in Fedora 17.
Now ibus Control+space switches engine by pressing space key till releasing Ctrl key likes Alt+tab metacity window.
E.g. In case that ibus engine list is, us keyboard, anthy, and jp keyboard and the current engine is us keyboard:
If one Ctrl+space is typed, ibus swaps us keyboard and anthy and the list is anthy, us keyboard, jp keyboard. 
If Ctrl+space+space is typed ibus swaps us keyboard and jp keyboard and the list is jp keyboard, anthy, us keyboard.
This would need the UI to know the current selected ibus engine.
Comment 102 Bastien Nocera 2012-06-11 09:58:25 UTC
(In reply to comment #101)
> (In reply to comment #98)
> > ime switcher dialog, what is that ? not sure we want that, now that we have a
> > unified approach to input sources.
> 
> I have already integrated it in both ibus and ibus-gnome3 package in Fedora 17.
> Now ibus Control+space switches engine by pressing space key till releasing
> Ctrl key likes Alt+tab metacity window.

This would need to be implemented in gnome-settings-daemon if anywhere, as that's where the layout switching is done.
Comment 103 Takao Fujiwara 2012-06-11 10:22:05 UTC
(In reply to comment #102)
> (In reply to comment #101)
> > (In reply to comment #98)
> > > ime switcher dialog, what is that ? not sure we want that, now that we have a
> > > unified approach to input sources.
> > 
> > I have already integrated it in both ibus and ibus-gnome3 package in Fedora 17.
> > Now ibus Control+space switches engine by pressing space key till releasing
> > Ctrl key likes Alt+tab metacity window.
> 
> This would need to be implemented in gnome-settings-daemon if anywhere, as
> that's where the layout switching is done.

I'm not sure if gnome-settings-daemon can provides the shell UI. I'm not sure if it's no problem that the GTK GUI is shown on gnome-shell.
Comment 104 Bastien Nocera 2012-06-11 15:51:45 UTC
(In reply to comment #103)
> (In reply to comment #102)
> > (In reply to comment #101)
> > > (In reply to comment #98)
> > > > ime switcher dialog, what is that ? not sure we want that, now that we have a
> > > > unified approach to input sources.
> > > 
> > > I have already integrated it in both ibus and ibus-gnome3 package in Fedora 17.
> > > Now ibus Control+space switches engine by pressing space key till releasing
> > > Ctrl key likes Alt+tab metacity window.
> > 
> > This would need to be implemented in gnome-settings-daemon if anywhere, as
> > that's where the layout switching is done.
> 
> I'm not sure if gnome-settings-daemon can provides the shell UI. I'm not sure
> if it's no problem that the GTK GUI is shown on gnome-shell.

Maybe I don't understand the requirements of this OSD. Why does it need to use GTK+?
Comment 105 Takao Fujiwara 2012-06-12 01:35:12 UTC
(In reply to comment #104)
> (In reply to comment #103) 
> Maybe I don't understand the requirements of this OSD. Why does it need to use
> GTK+?

We're talking about GUI - IME switcher dialog.
Comment 106 Matthias Clasen 2012-06-12 02:06:41 UTC
I'm not convinced we need a 'switcher dialog' or an OSD for this. But Bastien is right, if we need it, the right place for it is in gnome-settings-daemon, reusing our existing osd infrastructure there
Comment 107 Takao Fujiwara 2012-06-12 02:23:18 UTC
(In reply to comment #106)
> I'm not convinced we need a 'switcher dialog' or an OSD for this. But Bastien
> is right, if we need it, the right place for it is in gnome-settings-daemon,
> reusing our existing osd infrastructure there

If we don't have the switcher dialog, a toggle keybinding is required newly.
Currently switch-input-source and switch-input-source-backward are implemented in org.gnome.settings-daemon.plugins.media-keys.gschema.xml.

In ibus 1.4 (for Fedora 16), we provides the toggle keybinding and prev/next keybinding (likes switch-input-source/switch-input-source-backward).
In ibus 1.5 (for Fedora 17), we migrated the toggle and prev/next keybinginds to the modifiers key + accelerator key likes metacity's Alt+Tab application switchers. The switcher needs the GUI to determine the next engine.
If we don't provide the switcher dialog, the previous implementation, a toggle keybinding, is required to switch one keyboard layout and one input method likes IME on and off. It means users use both the toggle keybinding and prev/next keybindings for primary input method and other keyboards.
Comment 108 Matthias Clasen 2012-06-13 20:25:31 UTC
I think we really want to frontend to be in charge of how switching input methods works with keybindings. Having those methods completely change from one ibus version to the next is just not supportable.
Comment 109 Takao Fujiwara 2012-06-14 03:23:59 UTC
We migrated the toggle and prev/next shortcut keys to the new Control+space to satisfy the user's requests in the current ibus 1.5.
If we use the previous ibus implementation, the toggle shortcut key is required in the current gnome-settings-daemon newly.

Mainly we use the toggle shortcut key to swap a XKB layout engine and an input method engine and sometimes use the prev/next shortcut keys to switch other XKB layouts or input methods.
Since the previous shortcut key and next one are different, it's not useful for more than 3 engines of XKB layouts and input methods if the toggle shortcut key is not provided.


Here is one of the background of the new Control+space.
A user uses multiple input methods and us keyboard. He choose the several input methods on ibus menu by mouse and use Control+space to switch back to us keyboard.
The new Control+space can satisfy that user's request without using mouse.
Comment 110 Matthias Clasen 2012-06-14 10:51:08 UTC
I can see how an Alt-Tab like cycle-input-sources key combination could be neat.
I still think this should be in the control of the front end.
Comment 111 Rui Matos 2012-06-18 16:55:14 UTC
Created attachment 216691 [details] [review]
status/keyboard: Add support for IBus input sources

--
Addressed Florian's points and rebased.
Comment 112 Rui Matos 2012-06-18 17:04:38 UTC
Created attachment 216692 [details] [review]
ibusCandidatePopup: A candidate popup for IBus input methods

--
I ended up re-writing this thing and stripping it down to a minimum
that works. I'd like some comments on it.

Some things that don't work:
- separator doesn't show up. I'll have to fix that in a separate patch
  for popupMenu.js;

- mouse pointer input on this popup. It wasn't working on the previous
  patch either. I think it's doable with
  set_stage_input_mode/region. I'll attach a patch for that next;

- theming is probably all wrong.
Comment 113 Florian Müllner 2012-06-20 17:53:42 UTC
Review of attachment 216691 [details] [review]:

The commit message is still poor, otherwise looks good (the comments below are really nothing but nitpicks, the code is fine)

::: js/ui/status/keyboard.js
@@ +159,3 @@
+                    this._xkbInfo.get_layout_info(id);
+            } else if (type == INPUT_SOURCE_TYPE_IBUS) {
+                if (this._ibus) {

Could save one level of indentation by using
  (type == ... && this._ibus)

@@ +231,3 @@
+            }
+        } else {
+            return;

Is that useful? The (!xkbLayout) case below would catch this anyway, right?
Comment 114 Florian Müllner 2012-06-20 17:54:11 UTC
Review of attachment 216692 [details] [review]:

(In reply to comment #112)
> I ended up re-writing this thing and stripping it down to a minimum
> that works. I'd like some comments on it.

Thanks, this is *much* better. I have a couple of nits and suggestions, but nothing substantial.


> Some things that don't work:
> - separator doesn't show up. I'll have to fix that in a separate patch
>   for popupMenu.js;

Mmh, not immediately obvious to me why it doesn't. No objections on my part to fix that later.


> - mouse pointer input on this popup. It wasn't working on the previous
>   patch either. I think it's doable with
>   set_stage_input_mode/region. I'll attach a patch for that next;

Yeah, I mentioned that briefly in the review. I don't think this is a deal breaker, e.g. fixing it after landing the main patch is perfectly fine with me.

::: data/theme/gnome-shell.css
@@ +2049,3 @@
 }
+
+/* Candidate Popup */

Nit: I have a hunch that mentioning IBus in the comment will save me grepping in the future :-)

::: js/ui/ibusCandidatePopup.js
@@ +22,3 @@
+
+        this._tableLayout = new Clutter.TableLayout();
+        this._table.set_layout_manager(this._tableLayout);

LayoutManagers are the future, but for now I'd suggest StTable to allow proper styling on the container

@@ +31,3 @@
+        }
+
+        this._orientation = null;

Nit: "orientation" is an enum value, so null seems inappropriate - -1 maybe?

@@ +59,3 @@
+
+        let i;
+        for (i = 0; i < candidates.length && i < MAX_CANDIDATES_PER_PAGE; ++i) {

I wonder if

for (let i = 0; i < MAX_CANDIDATES_PER_PAGE; i++) {
    let visible = i < candidates.length;
    this._indexLabels[i].visible = visible;
    this._candidateLabels[i].visible = visible;

    if (!visible)
        continue;

    // update label texts
}

is clearer than the two loops.

@@ +84,3 @@
+        Main.uiGroup.add_actor(this._cursor);
+
+        this._popup = new PopupMenu.PopupMenu(this._cursor, 0, St.Side.TOP);

I think it makes more sense for the candidate popup to be a popup menu rather than to have one

@@ +103,3 @@
+    },
+
+    connectSignals: function(panelService) {

Does this really need to be public? It looks like you could initialize the panel service before the panel and just pass it to the constructor ...

@@ +199,3 @@
+    },
+
+    _hideShow: function() {

Awkward name - how about _updateVisibility()?

@@ +211,3 @@
+});
+
+function _setTextAttributes(clutterText, ibusAttrList) {

This doesn't look too useful outside the class, so I'd move this there
Comment 115 Rui Matos 2012-06-23 00:27:44 UTC
Created attachment 217054 [details] [review]
status/keyboard: Add support for IBus input sources

We connect to the IBus daemon asyncronously and use it to query info
about input sources of the type 'ibus'. In case the daemon is or
becomes unreachable we just skip showing input sources of this type.

--
I should have done this from the beggining really but, here it
goes.

This patch makes our interaction with the ibus daemon asyncronous to
reduce the potential for compositor lockups and also tries to be
robust against the daemon going away from under us at any time and
then tries to re-connect immediately or when the input sources list
changes.
Comment 116 Rui Matos 2012-06-23 00:31:38 UTC
Created attachment 217055 [details] [review]
ibusCandidatePopup: A candidate popup for IBus input methods

--

I think I addressed all your points except making
CandidatePopup.connectSignals() private since I need to be able to
pass in a new panelService object if/when we have to re-connect to the
ibus daemon.
Comment 117 Florian Müllner 2012-06-26 00:03:57 UTC
Review of attachment 217054 [details] [review]:

::: js/ui/status/keyboard.js
@@ +61,3 @@
+        this._state = IBusState.INITIALIZING;
+        this._ibus.connect('connected', Lang.bind(this, this._onConnected));
+        this._timeoutId = Mainloop.timeout_add_seconds(IBUS_CONNECTION_TIMEOUT,

This is pretty ugly, but looking at the ibus code it appears that the only indication of failure is indeed not receiving the 'connected' signal. Sigh.

@@ +94,3 @@
+    },
+
+    _initialize: function() {

Is it really worth putting this in a separate function? (mainly because having both _init() and _initialize() is a bit odd)

@@ +121,3 @@
+
+        if (this._state == IBusState.OFF)
+            this._connect();

Mmh, this is a bit unexpected here - is there really much of a point to initiate a connection in a getter? I don't see you retrying in case null is returned, so this is more or less for the case where you (for whatever reason) happen to call this function again?

@@ +240,3 @@
+                    info.exists = true;
+                    info.displayName = engineDesc.get_longname();
+                    info.shortName = engineDesc.get_symbol();

I always get an empty shortName here, I reckon this is ibus' fault?
Comment 118 Florian Müllner 2012-06-26 00:04:24 UTC
Review of attachment 217055 [details] [review]:

::: js/ui/ibusCandidatePopup.js
@@ +106,3 @@
+    },
+
+    connectSignals: function(panelService) {

I still don't like this - can we make this setPanelService() instead and take a reference (this._panelService = panelService)?
Comment 119 Rui Matos 2012-06-27 00:25:04 UTC
Created attachment 217336 [details] [review]
status/keyboard: Add support for IBus input sources

--
(In reply to comment #117)
> ::: js/ui/status/keyboard.js
> @@ +61,3 @@
> +        this._state = IBusState.INITIALIZING;
> +        this._ibus.connect('connected', Lang.bind(this, this._onConnected));
> +        this._timeoutId =
> Mainloop.timeout_add_seconds(IBUS_CONNECTION_TIMEOUT,
>
> This is pretty ugly, but looking at the ibus code it appears that the only
> indication of failure is indeed not receiving the 'connected' signal. Sigh.

I agree. We can get rid of it later when the ibus-daemon init issue is solved.

> @@ +94,3 @@
> +    },
> +
> +    _initialize: function() {
>
> Is it really worth putting this in a separate function? (mainly because having
> both _init() and _initialize() is a bit odd)

Sure, fixed.

> @@ +121,3 @@
> +
> +        if (this._state == IBusState.OFF)
> +            this._connect();
>
> Mmh, this is a bit unexpected here - is there really much of a point to
> initiate a connection in a getter? I don't see you retrying in case null is
> returned, so this is more or less for the case where you (for whatever reason)
> happen to call this function again?

Right, this is related to the ibus-daemon issue above and was a really
silly attempt to smooth that hard edge. Removed it now.

> @@ +240,3 @@
> +                    info.exists = true;
> +                    info.displayName = engineDesc.get_longname();
> +                    info.shortName = engineDesc.get_symbol();
>
> I always get an empty shortName here, I reckon this is ibus' fault?

Yes, see http://code.google.com/p/ibus/issues/detail?id=1473 .
Comment 120 Rui Matos 2012-06-27 00:26:36 UTC
Created attachment 217338 [details] [review]
ibusCandidatePopup: A candidate popup for IBus input methods

--
(In reply to comment #118)
> +    connectSignals: function(panelService) {
>
> I still don't like this - can we make this setPanelService() instead and take a
> reference (this._panelService = panelService)?

Sure.
Comment 121 Florian Müllner 2012-06-27 06:06:25 UTC
Review of attachment 217336 [details] [review]:

Yup
Comment 122 Florian Müllner 2012-06-27 06:08:44 UTC
Review of attachment 217338 [details] [review]:

LGTM
Comment 123 Rui Matos 2012-06-28 13:59:46 UTC
Created attachment 217523 [details] [review]
status/keyboard: Add support for IBus input sources

--
I've re-worked the IBus connection code since IBus is gaining a
session bus name which makes things easier and race free for us.

Changes to the previous version are solely inside the IBusManager
class.
Comment 124 Rui Matos 2012-06-28 14:00:42 UTC
Created attachment 217524 [details] [review]
ibusCandidatePopup: A candidate popup for IBus input methods

--
Rebased. Idem.
Comment 125 Florian Müllner 2012-06-28 21:00:57 UTC
Review of attachment 217523 [details] [review]:

Looks good. I'm still a bit worried about failing with older versions of IBus ("failing" in the sense of "crashing the window manager", not "im not working as expected"), but maybe it's good enough to add ibus to the moduleset for now (though there's this nagging, unfounded fear of an upcoming "I tried teh new ibus integration and the shell crashed OMG IBUS IS SUCH A PIECE OF CRAP GNOME HOW CAN YOU DO THIS!!11!"-thread on d-d-l ...)

::: js/ui/status/keyboard.js
@@ +86,3 @@
+    getEngineDesc: function(id) {
+        if (!IBus)
+            return null;

More concise as

    if (!IBus || !this._ready)
        return null;

    return this._engines[id];

but up to you
Comment 126 Florian Müllner 2012-06-28 21:01:21 UTC
Review of attachment 217524 [details] [review]:

Some nits, OK to commit with those fixed

::: js/ui/status/keyboard.js
@@ +92,3 @@
+        }
+
+        this._updateReadyness();

grammar gnome alert: readiness ;-)

@@ +112,3 @@
+        let ready = (Object.keys(this._engines).length > 0 &&
+                     this._panelService != null);
+        if (ready) {

This reads funny - once this._ready is true, it is never reset to false regardless of whether the condition is met or not. I know this._clear() handles this and is called appropriately, still the behavior is misleading regarding a method name of _updateReadiness(). Something like _maybeSetReady() would match the current behavior better IMHO, but the least confusing thing would probably be to just do

    function _updateReadiness: function() {
        this._ready = (Object.keys(this._engines).length > 0 &&
                       this._panelService != null);
        if (this._ready && this._readyCallback)
            this._readyCallback();
    }

(just to be clear: setting this._ready explicitly in this._clear() makes sense and I think it is perfectly fine to never actually use _updateReadiness() to reset this._ready - this is merely to avoid flow-breaking surprises when reading the code)
Comment 127 Rui Matos 2012-07-09 15:22:03 UTC
Created attachment 218345 [details] [review]
status/keyboard: Add support for IBus input sources

--
Almost forgot about this :-)

(In reply to comment #125)
> Looks good. I'm still a bit worried about failing with older versions of IBus
> ("failing" in the sense of "crashing the window manager", not "im not working
> as expected"), but maybe it's good enough to add ibus to the moduleset for now
> (though there's this nagging, unfounded fear of an upcoming "I tried teh new
> ibus integration and the shell crashed OMG IBUS IS SUCH A PIECE OF CRAP GNOME
> HOW CAN YOU DO THIS!!11!"-thread on d-d-l ...)

Agree, I've now added an ugly looking check that should prevent
that. I intend to replace it with a version check as soon as IBus 1.5
is released.

Also simplified the getEngineDesc() logic as you suggested, thanks!
Comment 128 Rui Matos 2012-07-09 15:22:57 UTC
Created attachment 218346 [details] [review]
ibusCandidatePopup: A candidate popup for IBus input methods

--
All comments addressed, thanks.
Comment 129 Florian Müllner 2012-07-09 15:33:37 UTC
Review of attachment 218345 [details] [review]:

Cool, thanks!
Comment 130 Florian Müllner 2012-07-09 15:33:50 UTC
Review of attachment 218346 [details] [review]:

LGTM
Comment 131 Rui Matos 2012-07-15 23:09:32 UTC
Marking as fixed. Further features shall be tracked in different bug reports.

Attachment 218345 [details] pushed as 14d3235 - status/keyboard: Add support for IBus input sources
Attachment 218346 [details] pushed as 04074f8 - ibusCandidatePopup: A candidate popup for IBus input methods