Merge the devel branch

Re-implement ibus panel in gtk3 and vala
Handle hotkey in ibus panel
Remove preload_engines from ibus-daemon, and move it to ibus panel
Implement a new IME popup switching UI
Integrate xkb anb IMEs
Deprecate the ibus python library 
Port setup ui from ibus python library to gobject-introspect
Port setup ui from gtk2 to gtk3
Refine some build scripts and rpm build script

BUG=None
TEST=Manually

[Peng, 2012-01-09 17:57:25]

I am using the new ibus on my Linux desktop for a while. Most major features are
finished. So I would like to merge them to upstream. Please test and review
them. Thanks.

The code is in my devel branch.
https://github.com/phuang/ibus/compare/master...devel .

[fujiwara, 2012-01-10 11:11:09]

On 2012/01/09 17:57:25, Peng wrote:
> I am using the new ibus on my Linux desktop for a while. Most major features
are
> finished. So I would like to merge them to upstream. Please test and review
> them. Thanks.
> 
> The code is in my devel branch.
> https://github.com/phuang/ibus/compare/master...devel .

I tested it today and I think there are two problems.
One is that ibus-ui-gtk3 always handle the keyboard events however I think users
could set GTK_IM_MODULE, QT_IM_MODULE and XMODIFIERS not to eat Control+Space by
ibus.
It might be good to copy the logic of gtkimmodule to ibus-ui-gtk3.

Another is the key confusion in Control + Space.
I'm thinking to use ibus-xkb and libgnomekbd to set xkb layouts:
https://github.com/fujiwarat/ibus/tree/gtk3-vala
When I set 'us' and 'jp' layouts with ctrl:swapcaps option, the handling of
Control+Space does not work correctly in ibus-ui-gtk3.
Test Case:
1. Set 'us' and 'jp':
% /usr/libexec/ibus-xkb -l us,jp \
  -o compose:rctrl,ctrl:swapcaps,grp:alt_shift_toggle
% /usr/libexec/ibus-xkb --get
layout: us,jp
variant: 
option: compose:rctrl,ctrl:swapcaps,grp:alt_shift_toggle
2. Run ibus-ui-gtk3
3. Switch layout 'us' to 'jp' by ibus or gnome-settings-daemon GUI.

Then the Control and CapsLock are confused.
I guess the grab_keycode needs to be re-binded after the layouts are changed.

[Peng, 2012-01-11 02:15:12]

On 2012/01/10 11:11:09, fujiwara wrote:
> On 2012/01/09 17:57:25, Peng wrote:
> > I am using the new ibus on my Linux desktop for a while. Most major features
> are
> > finished. So I would like to merge them to upstream. Please test and review
> > them. Thanks.
> > 
> > The code is in my devel branch.
> > https://github.com/phuang/ibus/compare/master...devel .
> 
> I tested it today and I think there are two problems.
> One is that ibus-ui-gtk3 always handle the keyboard events however I think
users
> could set GTK_IM_MODULE, QT_IM_MODULE and XMODIFIERS not to eat Control+Space
by
> ibus.

With the current design, the input method is a global settings. All applications
are using the input method engine. So we have to handle the hotkey globally.
Even if some applications do not need input method, user can still press the
hotkey to switch engine IME. It is like OSX ( maybe and Windows). If user do not
want it, they have to disable ibus at all.


> It might be good to copy the logic of gtkimmodule to ibus-ui-gtk3.
> 
> Another is the key confusion in Control + Space.
> I'm thinking to use ibus-xkb and libgnomekbd to set xkb layouts:
> https://github.com/fujiwarat/ibus/tree/gtk3-vala
> When I set 'us' and 'jp' layouts with ctrl:swapcaps option, the handling of
> Control+Space does not work correctly in ibus-ui-gtk3.
> Test Case:
> 1. Set 'us' and 'jp':
> % /usr/libexec/ibus-xkb -l us,jp \
>   -o compose:rctrl,ctrl:swapcaps,grp:alt_shift_toggle
> % /usr/libexec/ibus-xkb --get
> layout: us,jp
> variant: 
> option: compose:rctrl,ctrl:swapcaps,grp:alt_shift_toggle

Do you know how to set that xkb layout with setxkbmap? I do not have ibus-xkb in
my system.

BTW, in the new design, users have to use ibus to mange xkb layouts. If they set
the xkb layout manually, probably it could cause problems. 


> 2. Run ibus-ui-gtk3
> 3. Switch layout 'us' to 'jp' by ibus or gnome-settings-daemon GUI.
> 
> Then the Control and CapsLock are confused.
> I guess the grab_keycode needs to be re-binded after the layouts are changed.

[Peng, 2012-01-11 02:25:08]

BTW, In fedora, you could use below steps to install and test it.

cd $SRC
make rpm
sudo rpm -Uvh --force rpm/*/*.rpm
ibus-daemon -rvh


On 2012/01/11 02:15:12, Peng wrote:
> On 2012/01/10 11:11:09, fujiwara wrote:
> > On 2012/01/09 17:57:25, Peng wrote:
> > > I am using the new ibus on my Linux desktop for a while. Most major
features
> > are
> > > finished. So I would like to merge them to upstream. Please test and
review
> > > them. Thanks.
> > > 
> > > The code is in my devel branch.
> > > https://github.com/phuang/ibus/compare/master...devel .
> > 
> > I tested it today and I think there are two problems.
> > One is that ibus-ui-gtk3 always handle the keyboard events however I think
> users
> > could set GTK_IM_MODULE, QT_IM_MODULE and XMODIFIERS not to eat
Control+Space
> by
> > ibus.
> 
> With the current design, the input method is a global settings. All
applications
> are using the input method engine. So we have to handle the hotkey globally.
> Even if some applications do not need input method, user can still press the
> hotkey to switch engine IME. It is like OSX ( maybe and Windows). If user do
not
> want it, they have to disable ibus at all.
> 
> 
> > It might be good to copy the logic of gtkimmodule to ibus-ui-gtk3.
> > 
> > Another is the key confusion in Control + Space.
> > I'm thinking to use ibus-xkb and libgnomekbd to set xkb layouts:
> > https://github.com/fujiwarat/ibus/tree/gtk3-vala
> > When I set 'us' and 'jp' layouts with ctrl:swapcaps option, the handling of
> > Control+Space does not work correctly in ibus-ui-gtk3.
> > Test Case:
> > 1. Set 'us' and 'jp':
> > % /usr/libexec/ibus-xkb -l us,jp \
> >   -o compose:rctrl,ctrl:swapcaps,grp:alt_shift_toggle
> > % /usr/libexec/ibus-xkb --get
> > layout: us,jp
> > variant: 
> > option: compose:rctrl,ctrl:swapcaps,grp:alt_shift_toggle
> 
> Do you know how to set that xkb layout with setxkbmap? I do not have ibus-xkb
in
> my system.
> 
> BTW, in the new design, users have to use ibus to mange xkb layouts. If they
set
> the xkb layout manually, probably it could cause problems. 
> 
> 
> > 2. Run ibus-ui-gtk3
> > 3. Switch layout 'us' to 'jp' by ibus or gnome-settings-daemon GUI.
> > 
> > Then the Control and CapsLock are confused.
> > I guess the grab_keycode needs to be re-binded after the layouts are
changed.

[fujiwara, 2012-01-11 03:11:43]

On 2012/01/11 02:15:12, Peng wrote:
> On 2012/01/10 11:11:09, fujiwara wrote:
> > On 2012/01/09 17:57:25, Peng wrote:
> > > I am using the new ibus on my Linux desktop for a while. Most major
features
> > are
> > > finished. So I would like to merge them to upstream. Please test and
review
> > > them. Thanks.
> > > 
> > > The code is in my devel branch.
> > > https://github.com/phuang/ibus/compare/master...devel .
> > 
> > I tested it today and I think there are two problems.
> > One is that ibus-ui-gtk3 always handle the keyboard events however I think
> users
> > could set GTK_IM_MODULE, QT_IM_MODULE and XMODIFIERS not to eat
Control+Space
> by
> > ibus.
> 
> With the current design, the input method is a global settings. All
applications
> are using the input method engine. So we have to handle the hotkey globally.
> Even if some applications do not need input method, user can still press the
> hotkey to switch engine IME. It is like OSX ( maybe and Windows). If user do
not
> want it, they have to disable ibus at all.
> 
> 
> > It might be good to copy the logic of gtkimmodule to ibus-ui-gtk3.
> > 
> > Another is the key confusion in Control + Space.
> > I'm thinking to use ibus-xkb and libgnomekbd to set xkb layouts:
> > https://github.com/fujiwarat/ibus/tree/gtk3-vala
> > When I set 'us' and 'jp' layouts with ctrl:swapcaps option, the handling of
> > Control+Space does not work correctly in ibus-ui-gtk3.
> > Test Case:
> > 1. Set 'us' and 'jp':
> > % /usr/libexec/ibus-xkb -l us,jp \
> >   -o compose:rctrl,ctrl:swapcaps,grp:alt_shift_toggle
> > % /usr/libexec/ibus-xkb --get
> > layout: us,jp
> > variant: 
> > option: compose:rctrl,ctrl:swapcaps,grp:alt_shift_toggle
> 
> Do you know how to set that xkb layout with setxkbmap? I do not have ibus-xkb
in
> my system.

You could see ibus-xkb in my branch. Actually ibus-xkb is not used to set xkb
layouts.
libgnomekbd & libxklavier is used to set xkb layouts and ibus-xkb is used to get
the current layouts only.
If ibus-xkb or setxkbmap is used to set xkb layouts, the problem won't be
happened but currently probably I'm not thinking to use ibus-xkb on GNOME since
it would not work with libxklavier fully.

> 
> BTW, in the new design, users have to use ibus to mange xkb layouts. If they
set
> the xkb layout manually, probably it could cause problems. 

I think it's not the GNOME upstream's design.
I'm thinking the panel icon:
 - exists whether ibus-daemon runs or not.
 - cannot disable gnome-settings-daemon(libgnomekbd) from ibus.
 - works with gnome-control-center and ibus-setup doesn't touch keymaps on
GNOME.
 - handles events of libgnomekbd to reload keymaps.
 - use libgnomekbd API to set keymaps.

Currently Fedora ibus uses ibus-xkb to set keymaps but it has several problems
under libxklavier:
 - ibus cannot disable gnome-settings-daemon(libgnomekbd).
 - gnome-settings-daemon(libgnomekbd) outputs errors and often causes SEGV.
 - ibus doesn't notice when the system keymaps are loaded by
gnome-settings-daemon unless libgnomekbd is used.

so I'd like to use ibus-xkb to get the current layouts only.
I'm thinking to change the behavior when libxklavier is enabled.
But if this problem is not fixed, probably I cannot use libgnomekbd.
Probably I think GNOME will use libgnomebkd(libxklavier).

> 
> 
> > 2. Run ibus-ui-gtk3
> > 3. Switch layout 'us' to 'jp' by ibus or gnome-settings-daemon GUI.
> > 
> > Then the Control and CapsLock are confused.
> > I guess the grab_keycode needs to be re-binded after the layouts are
changed.

[Peng, 2012-01-11 16:40:45]

http://codereview.appspot.com/5517048/diff/12028/ui/gtk3/keybindingmanager.vala
File ui/gtk3/keybindingmanager.vala (right):

http://codereview.appspot.com/5517048/diff/12028/ui/gtk3/keybindingmanager.va...
ui/gtk3/keybindingmanager.vala:35: Gdk.ModifierType.MODIFIER_MASK & ~(
Fixed the hotkey related issue by add Gdk.ModifierType.MODIFIER_MASK here

[fujiwara, 2012-01-12 01:55:04]

http://codereview.appspot.com/5517048/diff/12028/ui/gtk3/panel.vala
File ui/gtk3/panel.vala (right):

http://codereview.appspot.com/5517048/diff/12028/ui/gtk3/panel.vala#newcode34
ui/gtk3/panel.vala:34: private Gtk.StatusIcon m_status_icon;
Please assign m_status_icon = null;
I got a SEGV during the login:
Thread 1 (Thread 0x7f2205b47940 (LWP 18565)):
#0  0x0000003b6f236285 in __GI_raise (sig=6) at
../nptl/sysdeps/unix/sysv/linux/raise.c:64
        resultvar = 0
        pid = <optimized out>
        selftid = 18565
#1  0x0000003b6f237b9b in __GI_abort () at abort.c:91
        save_stage = 2
        act = {__sigaction_handler = {sa_handler = 0, sa_sigaction = 0}, sa_mask
= {__val = {255301388018, 39962848, 255301407089, 4294967296, 32, 39962848,
39962848, 40060208, 40060208, 40056128, 40060208, 98, 1, 0, 255301302163,
39962848}}, sa_flags = 1898348202, sa_restorer = 0x80}
        sigs = {__val = {32, 0 <repeats 15 times>}}
#2  0x0000003b712683cd in g_assertion_message (domain=<optimized out>,
file=<optimized out>, line=<optimized out>, func=0x427070 "S", message=0x2634300
"assertion failed: (_tmp9_ != NULL)") at gtestutils.c:1425
        lstr =
"419\000\377\177\000\000\340\234C'\377\177\000\000\000\367Zo\005\000\000\000GiB\000\000\000\000"
        s = 0x2633540 ""
#3  0x0000003b712688f2 in g_assertion_message_expr (domain=0x4268ea "p9_ !=
NULL", file=0x42690c "gtk", line=419, func=0x427070 "S", expr=<optimized out>)
at gtestutils.c:1436
        s = <optimized out>
#4  0x000000000041424a in panel_construct (object_type=40001392, bus=0x2598180
[IBusBus]) at panel.c:423
        self = 0x262e030 [Panel]
        _tmp0_ = 0x2598180 [IBusBus]
        _tmp1_ = 1
        _tmp2_ = 0x2598180 [IBusBus]
        _tmp3_ = 0x261a800 [GDBusConnection]
        _tmp4_ = 0x2598180 [IBusBus]
        _tmp5_ = 0x2598180 [IBusBus]
        _tmp6_ = 0x2598180 [IBusBus]
        _tmp7_ = 0x0
        _tmp8_ = 0x0
        _tmp9_ = 0x0
        _tmp10_ = 0x2625f70
        _tmp11_ = 0x10
        _tmp12_ = 0x2625f70
        _tmp13_ = 0x3b7124b6b8
        _tmp14_ = 0x261a0c0
        _tmp15_ = 0x3b712606f2
        _tmp16_ = 0x0
        _tmp17_ = 0x426d80
        _tmp18_ = 0x24c7590
        _tmp19_ = 0x50
        _tmp20_ = 0x0
        _tmp21_ = 0x3b6f281f6a
        _tmp22_ = 0x0
        _tmp23_ = 0x50
        _tmp24_ = 0x3b6f5af700
        _tmp25_ = 0x0
        _tmp26_ = 0x2626030
        _tmp27_ = 0x5fd0
        _tmp28_ = 0x3b6f5af700
        _tmp29_ = 0x0
        _tmp30_ = 0x50
        _tmp31_ = 0x0
        _tmp32_ = 0x6e0000005b
        _tmp33_ = 0x7c00000077
        _tmp34_ = 0x6e0000005b
        __PRETTY_FUNCTION__ = "panel_construct"
#5  0x0000000000414724 in panel_init_engines_order (self=0x262e030 [Panel]) at
panel.c:486
        _tmp0_ = 0x2598180 [IBusBus]
        _tmp1_ = 0x7fff27439f30
        _tmp2_ = 0x414724
        __PRETTY_FUNCTION__ = "panel_init_engines_order"

[Peng, 2012-01-12 16:05:27]

http://codereview.appspot.com/5517048/diff/12028/ui/gtk3/panel.vala
File ui/gtk3/panel.vala (right):

http://codereview.appspot.com/5517048/diff/12028/ui/gtk3/panel.vala#newcode34
ui/gtk3/panel.vala:34: private Gtk.StatusIcon m_status_icon;
On 2012/01/12 01:55:04, fujiwara wrote:
> Please assign m_status_icon = null;
> I got a SEGV during the login:
> Thread 1 (Thread 0x7f2205b47940 (LWP 18565)):
> #0  0x0000003b6f236285 in __GI_raise (sig=6) at
> ../nptl/sysdeps/unix/sysv/linux/raise.c:64
>         resultvar = 0
>         pid = <optimized out>
>         selftid = 18565
> #1  0x0000003b6f237b9b in __GI_abort () at abort.c:91
>         save_stage = 2
>         act = {__sigaction_handler = {sa_handler = 0, sa_sigaction = 0},
sa_mask
> = {__val = {255301388018, 39962848, 255301407089, 4294967296, 32, 39962848,
> 39962848, 40060208, 40060208, 40056128, 40060208, 98, 1, 0, 255301302163,
> 39962848}}, sa_flags = 1898348202, sa_restorer = 0x80}
>         sigs = {__val = {32, 0 <repeats 15 times>}}
> #2  0x0000003b712683cd in g_assertion_message (domain=<optimized out>,
> file=<optimized out>, line=<optimized out>, func=0x427070 "S",
message=0x2634300
> "assertion failed: (_tmp9_ != NULL)") at gtestutils.c:1425
>         lstr =
>
"419\000\377\177\000\000\340\234C'\377\177\000\000\000\367Zo\005\000\000\000GiB\000\000\000\000"
>         s = 0x2633540 ""
> #3  0x0000003b712688f2 in g_assertion_message_expr (domain=0x4268ea "p9_ !=
> NULL", file=0x42690c "gtk", line=419, func=0x427070 "S", expr=<optimized out>)
> at gtestutils.c:1436
>         s = <optimized out>
> #4  0x000000000041424a in panel_construct (object_type=40001392, bus=0x2598180
> [IBusBus]) at panel.c:423
>         self = 0x262e030 [Panel]
>         _tmp0_ = 0x2598180 [IBusBus]
>         _tmp1_ = 1
>         _tmp2_ = 0x2598180 [IBusBus]
>         _tmp3_ = 0x261a800 [GDBusConnection]
>         _tmp4_ = 0x2598180 [IBusBus]
>         _tmp5_ = 0x2598180 [IBusBus]
>         _tmp6_ = 0x2598180 [IBusBus]
>         _tmp7_ = 0x0
>         _tmp8_ = 0x0
>         _tmp9_ = 0x0
>         _tmp10_ = 0x2625f70
>         _tmp11_ = 0x10
>         _tmp12_ = 0x2625f70
>         _tmp13_ = 0x3b7124b6b8
>         _tmp14_ = 0x261a0c0
>         _tmp15_ = 0x3b712606f2
>         _tmp16_ = 0x0
>         _tmp17_ = 0x426d80
>         _tmp18_ = 0x24c7590
>         _tmp19_ = 0x50
>         _tmp20_ = 0x0
>         _tmp21_ = 0x3b6f281f6a
>         _tmp22_ = 0x0
>         _tmp23_ = 0x50
>         _tmp24_ = 0x3b6f5af700
>         _tmp25_ = 0x0
>         _tmp26_ = 0x2626030
>         _tmp27_ = 0x5fd0
>         _tmp28_ = 0x3b6f5af700
>         _tmp29_ = 0x0
>         _tmp30_ = 0x50
>         _tmp31_ = 0x0
>         _tmp32_ = 0x6e0000005b
>         _tmp33_ = 0x7c00000077
>         _tmp34_ = 0x6e0000005b
>         __PRETTY_FUNCTION__ = "panel_construct"
> #5  0x0000000000414724 in panel_init_engines_order (self=0x262e030 [Panel]) at
> panel.c:486
>         _tmp0_ = 0x2598180 [IBusBus]
>         _tmp1_ = 0x7fff27439f30
>         _tmp2_ = 0x414724
>         __PRETTY_FUNCTION__ = "panel_init_engines_order"

Fixed

http://codereview.appspot.com/5517048/diff/18001/ui/gtk3/switcher.vala
File ui/gtk3/switcher.vala (right):

http://codereview.appspot.com/5517048/diff/18001/ui/gtk3/switcher.vala#newcod...
ui/gtk3/switcher.vala:224: Gdk.EventKey *pe = &e;
Fixed the hotkey issue issue by remove 

 225         uint modifier = KeybindingManager.keyval_to_modifier(pe->keyval);	
 226         if (m_primary_modifier != modifier)	
 227             return true;

[fujiwara, 2012-01-13 01:34:52]

On 2012/01/12 16:05:27, Peng wrote:
> http://codereview.appspot.com/5517048/diff/12028/ui/gtk3/panel.vala
> File ui/gtk3/panel.vala (right):
> 
> http://codereview.appspot.com/5517048/diff/12028/ui/gtk3/panel.vala#newcode34
> ui/gtk3/panel.vala:34: private Gtk.StatusIcon m_status_icon;
> On 2012/01/12 01:55:04, fujiwara wrote:
> > Please assign m_status_icon = null;
> > I got a SEGV during the login:
> > Thread 1 (Thread 0x7f2205b47940 (LWP 18565)):
> > #0  0x0000003b6f236285 in __GI_raise (sig=6) at
> > ../nptl/sysdeps/unix/sysv/linux/raise.c:64
> >         resultvar = 0
> >         pid = <optimized out>
> >         selftid = 18565
> > #1  0x0000003b6f237b9b in __GI_abort () at abort.c:91
> >         save_stage = 2
> >         act = {__sigaction_handler = {sa_handler = 0, sa_sigaction = 0},
> sa_mask
> > = {__val = {255301388018, 39962848, 255301407089, 4294967296, 32, 39962848,
> > 39962848, 40060208, 40060208, 40056128, 40060208, 98, 1, 0, 255301302163,
> > 39962848}}, sa_flags = 1898348202, sa_restorer = 0x80}
> >         sigs = {__val = {32, 0 <repeats 15 times>}}
> > #2  0x0000003b712683cd in g_assertion_message (domain=<optimized out>,
> > file=<optimized out>, line=<optimized out>, func=0x427070 "S",
> message=0x2634300
> > "assertion failed: (_tmp9_ != NULL)") at gtestutils.c:1425
> >         lstr =
> >
>
"419\000\377\177\000\000\340\234C'\377\177\000\000\000\367Zo\005\000\000\000GiB\000\000\000\000"
> >         s = 0x2633540 ""
> > #3  0x0000003b712688f2 in g_assertion_message_expr (domain=0x4268ea "p9_ !=
> > NULL", file=0x42690c "gtk", line=419, func=0x427070 "S", expr=<optimized
out>)
> > at gtestutils.c:1436
> >         s = <optimized out>
> > #4  0x000000000041424a in panel_construct (object_type=40001392,
bus=0x2598180
> > [IBusBus]) at panel.c:423
> >         self = 0x262e030 [Panel]
> >         _tmp0_ = 0x2598180 [IBusBus]
> >         _tmp1_ = 1
> >         _tmp2_ = 0x2598180 [IBusBus]
> >         _tmp3_ = 0x261a800 [GDBusConnection]
> >         _tmp4_ = 0x2598180 [IBusBus]
> >         _tmp5_ = 0x2598180 [IBusBus]
> >         _tmp6_ = 0x2598180 [IBusBus]
> >         _tmp7_ = 0x0
> >         _tmp8_ = 0x0
> >         _tmp9_ = 0x0
> >         _tmp10_ = 0x2625f70
> >         _tmp11_ = 0x10
> >         _tmp12_ = 0x2625f70
> >         _tmp13_ = 0x3b7124b6b8
> >         _tmp14_ = 0x261a0c0
> >         _tmp15_ = 0x3b712606f2
> >         _tmp16_ = 0x0
> >         _tmp17_ = 0x426d80
> >         _tmp18_ = 0x24c7590
> >         _tmp19_ = 0x50
> >         _tmp20_ = 0x0
> >         _tmp21_ = 0x3b6f281f6a
> >         _tmp22_ = 0x0
> >         _tmp23_ = 0x50
> >         _tmp24_ = 0x3b6f5af700
> >         _tmp25_ = 0x0
> >         _tmp26_ = 0x2626030
> >         _tmp27_ = 0x5fd0
> >         _tmp28_ = 0x3b6f5af700
> >         _tmp29_ = 0x0
> >         _tmp30_ = 0x50
> >         _tmp31_ = 0x0
> >         _tmp32_ = 0x6e0000005b
> >         _tmp33_ = 0x7c00000077
> >         _tmp34_ = 0x6e0000005b
> >         __PRETTY_FUNCTION__ = "panel_construct"
> > #5  0x0000000000414724 in panel_init_engines_order (self=0x262e030 [Panel])
at
> > panel.c:486
> >         _tmp0_ = 0x2598180 [IBusBus]
> >         _tmp1_ = 0x7fff27439f30
> >         _tmp2_ = 0x414724
> >         __PRETTY_FUNCTION__ = "panel_init_engines_order"
> 
> Fixed
> 
> http://codereview.appspot.com/5517048/diff/18001/ui/gtk3/switcher.vala
> File ui/gtk3/switcher.vala (right):
> 
>
http://codereview.appspot.com/5517048/diff/18001/ui/gtk3/switcher.vala#newcod...
> ui/gtk3/switcher.vala:224: Gdk.EventKey *pe = &e;
> Fixed the hotkey issue issue by remove 
> 
>  225         uint modifier =
KeybindingManager.keyval_to_modifier(pe->keyval);	
>  226         if (m_primary_modifier != modifier)	
>  227             return true;

I confirmed the fix resolves my problem. thanks.
I will have the furthermore tests.

[fujiwara, 2012-01-13 10:12:06]

http://codereview.appspot.com/5517048/diff/18001/ui/gtk3/application.vala
File ui/gtk3/application.vala (right):

http://codereview.appspot.com/5517048/diff/18001/ui/gtk3/application.vala#new...
ui/gtk3/application.vala:63: m_bus.request_name("org.freedesktop.IBus.Panel",
2);
I think the argument is 1 or 3 but not 2.
Also need to hide the pane icon when "NameLost" is received.

[Peng, 2012-01-13 16:59:35]

http://codereview.appspot.com/5517048/diff/18001/ui/gtk3/application.vala
File ui/gtk3/application.vala (right):

http://codereview.appspot.com/5517048/diff/18001/ui/gtk3/application.vala#new...
ui/gtk3/application.vala:63: m_bus.request_name("org.freedesktop.IBus.Panel",
2);
On 2012/01/13 10:12:06, fujiwara wrote:
> I think the argument is 1 or 3 but not 2.
> Also need to hide the pane icon when "NameLost" is received.
Fixed

[fujiwara, 2012-01-16 10:19:09]

I'm thinking how ibus can handle the keybindings with gnome-shell.
Probably I think it's better to apply a new patch for
meta_keybindings_set_custom_handler() in mutter since it handles gnome-shell
keybindings.
However it might be good to move ui/gtk3/keybindingmanager.vala to libibus.so in
ibus at the moment so that ibus-gjs uses the Gdk keybindings without another
patch.
Currently keybindingmanager.vala uses the C code grab_keycode() and
ungrab_keycode() and they cannot be implemented in gjs.

On 2012/01/09 17:57:25, Peng wrote:

[Peng, 2012-01-16 15:22:15]

On 2012/01/16 10:19:09, fujiwara wrote:
> I'm thinking how ibus can handle the keybindings with gnome-shell.
> Probably I think it's better to apply a new patch for
> meta_keybindings_set_custom_handler() in mutter since it handles gnome-shell

I think it is not good to add X related code in libibus which does not depends
on any X or other graphics systems.

And I think gnome-shell is a X window manager and it will grab X server includes
key events. So Maybe it is good to let gnome-shell provides a features to allow
plugins to set custom keybindings. right?


> keybindings.
> However it might be good to move ui/gtk3/keybindingmanager.vala to libibus.so
in
> ibus at the moment so that ibus-gjs uses the Gdk keybindings without another
> patch.
> Currently keybindingmanager.vala uses the C code grab_keycode() and
> ungrab_keycode() and they cannot be implemented in gjs.
> 
> On 2012/01/09 17:57:25, Peng wrote:

[Peng, 2012-01-16 15:41:20]

On 2012/01/16 15:22:15, Peng wrote:
> On 2012/01/16 10:19:09, fujiwara wrote:
> > I'm thinking how ibus can handle the keybindings with gnome-shell.
> > Probably I think it's better to apply a new patch for
> > meta_keybindings_set_custom_handler() in mutter since it handles gnome-shell
> 
> I think it is not good to add X related code in libibus which does not depends
> on any X or other graphics systems.
> 
> And I think gnome-shell is a X window manager and it will grab X server
includes
> key events. So Maybe it is good to let gnome-shell provides a features to
allow
> plugins to set custom keybindings. right?
> 
> 
> > keybindings.
> > However it might be good to move ui/gtk3/keybindingmanager.vala to
libibus.so
> in
> > ibus at the moment so that ibus-gjs uses the Gdk keybindings without another
> > patch.
> > Currently keybindingmanager.vala uses the C code grab_keycode() and
> > ungrab_keycode() and they cannot be implemented in gjs.
> > 
> > On 2012/01/09 17:57:25, Peng wrote:

https://bugzilla.gnome.org/show_bug.cgi?id=663428

Seems this feature has been implemented in upstream git repo of mutter. Please
try it.

[Peng, 2012-01-19 22:14:02]

On 2012/01/16 15:41:20, Peng wrote:
> On 2012/01/16 15:22:15, Peng wrote:
> > On 2012/01/16 10:19:09, fujiwara wrote:
> > > I'm thinking how ibus can handle the keybindings with gnome-shell.
> > > Probably I think it's better to apply a new patch for
> > > meta_keybindings_set_custom_handler() in mutter since it handles
gnome-shell
> > 
> > I think it is not good to add X related code in libibus which does not
depends
> > on any X or other graphics systems.
> > 
> > And I think gnome-shell is a X window manager and it will grab X server
> includes
> > key events. So Maybe it is good to let gnome-shell provides a features to
> allow
> > plugins to set custom keybindings. right?
> > 
> > 
> > > keybindings.
> > > However it might be good to move ui/gtk3/keybindingmanager.vala to
> libibus.so
> > in
> > > ibus at the moment so that ibus-gjs uses the Gdk keybindings without
another
> > > patch.
> > > Currently keybindingmanager.vala uses the C code grab_keycode() and
> > > ungrab_keycode() and they cannot be implemented in gjs.
> > > 
> > > On 2012/01/09 17:57:25, Peng wrote:
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=663428
> 
> Seems this feature has been implemented in upstream git repo of mutter. Please
> try it.

Hi Fujiwara-san, Did you have to try upstream mutter? Do you have any other
feedback? Thanks.

[fujiwara, 2012-01-21 05:22:38]

On 2012/01/19 22:14:02, Peng wrote:
> On 2012/01/16 15:41:20, Peng wrote:
> > On 2012/01/16 15:22:15, Peng wrote:
> > > On 2012/01/16 10:19:09, fujiwara wrote:
> > > > I'm thinking how ibus can handle the keybindings with gnome-shell.
> > > > Probably I think it's better to apply a new patch for
> > > > meta_keybindings_set_custom_handler() in mutter since it handles
> gnome-shell
> > > 
> > > I think it is not good to add X related code in libibus which does not
> depends
> > > on any X or other graphics systems.
> > > 
> > > And I think gnome-shell is a X window manager and it will grab X server
> > includes
> > > key events. So Maybe it is good to let gnome-shell provides a features to
> > allow
> > > plugins to set custom keybindings. right?
> > > 
> > > 
> > > > keybindings.
> > > > However it might be good to move ui/gtk3/keybindingmanager.vala to
> > libibus.so
> > > in
> > > > ibus at the moment so that ibus-gjs uses the Gdk keybindings without
> another
> > > > patch.
> > > > Currently keybindingmanager.vala uses the C code grab_keycode() and
> > > > ungrab_keycode() and they cannot be implemented in gjs.
> > > > 
> > > > On 2012/01/09 17:57:25, Peng wrote:
> > 
> > https://bugzilla.gnome.org/show_bug.cgi?id=663428
> > 
> > Seems this feature has been implemented in upstream git repo of mutter.
Please
> > try it.
> 
> Hi Fujiwara-san, Did you have to try upstream mutter? Do you have any other
> feedback? Thanks.

Sorry for the late response. I forgot to run glib-compile-schemas to fix my
problem.
Now I'm adding meta_display_add_keybinding() and it seems to be ok. 
https://github.com/fujiwarat/ibus-gjs/blob/gtk3-vala/js/ui/status/ibus/ibusPa...
However probably I think ibus-gjs needs to have the similar implementation with
Alt + Tab in shell.
Probably I'd like to upstream ibus-xkb on this devel so that I don't use
setxkbmap and get the current keymaps from session.
https://github.com/fujiwarat/ibus/blob/gtk3-vala/ui/gtk3/panel.vala#L166
The ibus-xkb supports:
 - the session XKB options.
 - no ascii layouts likes ru
 - xmodmap
 - default[options]
Also we need to support 'default' in IBusEngienDesc.layout for input methods not
to change the current layout.

I have some feedback later.

[fujiwara, 2012-01-21 05:35:47]

http://codereview.appspot.com/5517048/diff/23002/src/ibusbus.h
File src/ibusbus.h (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibusbus.h#newcode717
src/ibusbus.h:717: * @returns: (transfer container) (element-type
IBusEngineDesc): A %NULL-terminated array of engines.
Please use (array zero-terminated=1) instead of (transfer container):
https://github.com/fujiwarat/ibus/commit/df9e6524a7738ed7d535352fd0754e487aa0...

http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala
File ui/gtk3/panel.vala (right):

http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala#newcode44
ui/gtk3/panel.vala:44: private const string ACCELERATOR_SWITCH_IME_BACKWARD =
"<Control><Shift>space";
meta_display_add_keybinding() supports to handle reverse hotkeys with Shift. So
we may need ACCELERATOR_SWITCH_IME_FOREWARD only but not need
ACCELERATOR_SWITCH_IME_BACKWARD.
Also meta_display_add_keybinding() needs to access gsettings key so I'm thinking
to add /desktop/ibus/general/hotkey/trigger-accel .

http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala#newcode122
ui/gtk3/panel.vala:122: string cmdline = "setxkbmap
%s".printf(engine.get_layout());
If the layout == 'default', I'd like not to call setxkbmap.

[komatsu1, 2012-01-23 02:45:12]

http://codereview.appspot.com/5517048/diff/23002/bus/test-client.c
File bus/test-client.c (right):

http://codereview.appspot.com/5517048/diff/23002/bus/test-client.c#newcode115
bus/test-client.c:115: // ibus_input_context_enable (client->ibuscontext);
Please delete this line, or add a comment describing why this is commented out.

http://codereview.appspot.com/5517048/diff/23002/src/ibusenginesimple.c
File src/ibusenginesimple.c (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibusenginesimple.c#newco...
src/ibusenginesimple.c:53: /* From the values below, the value 23 means the
number of different first keysyms
24?

http://codereview.appspot.com/5517048/diff/23002/src/ibusenginesimple.c#newco...
src/ibusenginesimple.c:86: static void     ibus_engine_simple_reset       
(IBusEngine         *engine);
Would you add a comment why this type is not IBusEngineSimple?

http://codereview.appspot.com/5517048/diff/23002/src/ibusutil.c
File src/ibusutil.c (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibusutil.c#newcode37
src/ibusutil.c:37: #define N_(t) t
Would you add a comment describing your intention?

http://codereview.appspot.com/5517048/diff/23002/src/ibusutil.c#newcode102
src/ibusutil.c:102: // filename = g_build_filename (ISOCODES_PREFIX,
Please double check this line.

[nona1, 2012-01-23 03:01:35]

http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py
File ui/gtk2/panel.py (left):

http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py#oldcode220
ui/gtk2/panel.py:220: def focus_in(self, ic):
Is gtk2 panel also support hot key binding?
If not, how about remove gtk2 related codes?

http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala
File ui/gtk3/panel.vala (right):

http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala#newcode122
ui/gtk3/panel.vala:122: string cmdline = "setxkbmap
%s".printf(engine.get_layout());
I also think 'default' is good, because there are many Japanese engine users
with English keyboard.

However I think just avoid calling setxkbmap is incomplete.

If you use 'French' and 'Japanese' engine.
And if Japanese engine use 'default' key map.

1. Change engine to Japanese [keymap: Japanese keyboard as default]
2. Change engine to French [keymap: French keyboard]
3. Change engine to Japanese [keymap: French]

I think the user expects Japanese keyboard in the 3. step.

However I don't have good idea to solve above problem.
Making it configurable is the good way but it needs a bit cost...

On 2012/01/21 05:35:47, fujiwara wrote:
> If the layout == 'default', I'd like not to call setxkbmap.

[Yusuke Sato, 2012-01-23 03:09:07]

initial review.

> I think it is not good to add X related code in libibus which does not depends
> on any X or other graphics systems.

+1 for the Peng's decision.

http://codereview.appspot.com/5517048/diff/23002/bus/dbusimpl.c
File bus/dbusimpl.c (right):

http://codereview.appspot.com/5517048/diff/23002/bus/dbusimpl.c#newcode334
bus/dbusimpl.c:334: g_slist_free_full (service->owners,
(GDestroyNotify)bus_connection_owner_free);
nit: one space after 'Notify)'.

http://codereview.appspot.com/5517048/diff/23002/bus/dbusimpl.c#newcode630
bus/dbusimpl.c:630: g_list_free_full (dbus->start_service_calls,
(GDestroyNotify)bus_method_call_free);
ditto

http://codereview.appspot.com/5517048/diff/23002/bus/dbusimpl.c#newcode1832
bus/dbusimpl.c:1832: g_list_free_full (dbus->dispatch_queue,
(GDestroyNotify)bus_dispatch_data_free);
ditto

http://codereview.appspot.com/5517048/diff/23002/bus/global.h
File bus/global.h (right):

http://codereview.appspot.com/5517048/diff/23002/bus/global.h#newcode5
bus/global.h:5: * Copyright (c) 2010 Google, Inc.
not sure but 2010-2012 or 2012, maybe?

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.c
File bus/ibusimpl.c (right):

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.c#newcode60
bus/ibusimpl.c:60: gboolean enable_by_default;
looks like you can remove the member now.

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.c#newcode169
bus/ibusimpl.c:169: "    <method name='GetEnginesByNames'>\n"
please write a unit test for the newly added IPC to make sure the XML does not
have typo and the code for handling GVariant does not have memory
leaks/corruptions.

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.c#newcode349
bus/ibusimpl.c:349: ibus->enable_by_default = TRUE;
remove

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.c#newcode477
bus/ibusimpl.c:477: engine_name = "xkb:us::eng";
nit: what about moving the literal to global.h?

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.c#newcode799
bus/ibusimpl.c:799: if (ibus->enable_by_default) {
remove the if.

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.h
File bus/ibusimpl.h (right):

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.h#newcode80
bus/ibusimpl.h:80: gboolean         bus_ibus_impl_filter_keyboard_shortcuts
remove the prototype.

http://codereview.appspot.com/5517048/diff/23002/bus/inputcontext.c
File bus/inputcontext.c (right):

http://codereview.appspot.com/5517048/diff/23002/bus/inputcontext.c#newcode145
bus/inputcontext.c:145: /*
is the comment (line 145-163) still necessary?

http://codereview.appspot.com/5517048/diff/23002/bus/inputcontext.c#newcode1084
bus/inputcontext.c:1084: #if 0
why the block is commented out?

http://codereview.appspot.com/5517048/diff/23002/bus/inputcontext.c#newcode1969
bus/inputcontext.c:1969: /* FIXME Do we need to emit "enabled" signal? */
nit: remove the comment.

http://codereview.appspot.com/5517048/diff/23002/bus/matchrule.c
File bus/matchrule.c (right):

http://codereview.appspot.com/5517048/diff/23002/bus/matchrule.c#newcode272
bus/matchrule.c:272: // g_debug("failed at: \"%s\"", p);
remove?

http://codereview.appspot.com/5517048/diff/23002/bus/test-client.c
File bus/test-client.c (right):

http://codereview.appspot.com/5517048/diff/23002/bus/test-client.c#newcode115
bus/test-client.c:115: // ibus_input_context_enable (client->ibuscontext);
uncomment?

http://codereview.appspot.com/5517048/diff/23002/conf/gconf/Makefile.am
File conf/gconf/Makefile.am (right):

http://codereview.appspot.com/5517048/diff/23002/conf/gconf/Makefile.am#newcode1
conf/gconf/Makefile.am:1: # vim:set noet ts=4:
I'm not an expert of Linux desktop, but can't we just drop gconf support from
the new version of ibus?

http://codereview.appspot.com/5517048/diff/23002/configure.ac
File configure.ac (right):

http://codereview.appspot.com/5517048/diff/23002/configure.ac#newcode429
configure.ac:429: # --enable-surrounding-text option.
How about enabling the feature by default? i.e. remove --enable-surrounding-text
and add --disable-surrounding-text.

ibus-mozc recently started using the feature and it's important for the engine
that the feature is supported by IBus on various Linux distributions.

http://codereview.appspot.com/5517048/diff/23002/src/ibusbus.c
File src/ibusbus.c (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibusbus.c#newcode1482
src/ibusbus.c:1482: ibus_bus_get_engines_by_names (IBusBus             *bus,
please write a unit test for the new IPC.

http://codereview.appspot.com/5517048/diff/23002/src/ibusenginesimple.h
File src/ibusenginesimple.h (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibusenginesimple.h#newcode1
src/ibusenginesimple.h:1: /* -*- mode: C; c-basic-offset: 4; indent-tabs-mode:
nil; -*- */
Can you let me know why we have to do the dead key composition in ibus-daemon
rather then in an application?

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeynames.c
File src/ibuskeynames.c (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeynames.c#newcode135
src/ibuskeynames.c:135: g_return_val_if_fail (keyval != IBUS_KEY_VoidSymbol,
NULL);
nit: tab to space

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeynames.c#newcode192
src/ibuskeynames.c:192: if (*keyval != IBUS_KEY_VoidSymbol)
ditto

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeysyms-compat.h
File src/ibuskeysyms-compat.h (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeysyms-compat.h#new...
src/ibuskeysyms-compat.h:43: #ifndef __IBUS_KEYSYMS_COMPACT_H__
s/compact/compat/

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeysyms-compat.h#new...
src/ibuskeysyms-compat.h:44: #define __IBUS_KEYSYMS_COMPACT_H__
the same as above.

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeysyms-compat.h#new...
src/ibuskeysyms-compat.h:2099: #endif /* __IBUS_KEYSYMS_COMPACT_H__ */
same

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeysyms.h
File src/ibuskeysyms.h (left):

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeysyms.h#oldcode1
src/ibuskeysyms.h:1: /* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil;
-*- */
keep the line?

http://codereview.appspot.com/5517048/diff/23002/src/ibusxml.c
File src/ibusxml.c (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibusxml.c#newcode37
src/ibusxml.c:37: g_list_free_full (node->sub_nodes,
(GDestroyNotify)ibus_xml_free);
one space after 'Notify)'.

[fujiwara, 2012-01-23 08:48:17]

On 2012/01/23 03:01:35, nona1 wrote:
> http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py
> File ui/gtk2/panel.py (left):
> 
> http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py#oldcode220
> ui/gtk2/panel.py:220: def focus_in(self, ic):
> Is gtk2 panel also support hot key binding?
> If not, how about remove gtk2 related codes?
> 
> http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala
> File ui/gtk3/panel.vala (right):
> 
> http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala#newcode122
> ui/gtk3/panel.vala:122: string cmdline = "setxkbmap
> %s".printf(engine.get_layout());
> I also think 'default' is good, because there are many Japanese engine users
> with English keyboard.
> 
> However I think just avoid calling setxkbmap is incomplete.
> 
> If you use 'French' and 'Japanese' engine.
> And if Japanese engine use 'default' key map.
> 
> 1. Change engine to Japanese [keymap: Japanese keyboard as default]
> 2. Change engine to French [keymap: French keyboard]
> 3. Change engine to Japanese [keymap: French]
> 
> I think the user expects Japanese keyboard in the 3. step.

I think the user expects French keyboard in the 3 by default.
This doesn't mean to just avoid the specific keymaps.
Also the behavior could be changed by each engine's property if the engine
thrown 'default' or the specific keymap.

> 
> However I don't have good idea to solve above problem.
> Making it configurable is the good way but it needs a bit cost...
> 
> On 2012/01/21 05:35:47, fujiwara wrote:
> > If the layout == 'default', I'd like not to call setxkbmap.

[Peng, 2012-01-23 19:33:47]

http://codereview.appspot.com/5517048/diff/23002/bus/dbusimpl.c
File bus/dbusimpl.c (right):

http://codereview.appspot.com/5517048/diff/23002/bus/dbusimpl.c#newcode334
bus/dbusimpl.c:334: g_slist_free_full (service->owners,
(GDestroyNotify)bus_connection_owner_free);
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> nit: one space after 'Notify)'.

Done.

http://codereview.appspot.com/5517048/diff/23002/bus/dbusimpl.c#newcode630
bus/dbusimpl.c:630: g_list_free_full (dbus->start_service_calls,
(GDestroyNotify)bus_method_call_free);
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> ditto

Done.

http://codereview.appspot.com/5517048/diff/23002/bus/dbusimpl.c#newcode1832
bus/dbusimpl.c:1832: g_list_free_full (dbus->dispatch_queue,
(GDestroyNotify)bus_dispatch_data_free);
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> ditto

Done.

http://codereview.appspot.com/5517048/diff/23002/bus/global.h
File bus/global.h (right):

http://codereview.appspot.com/5517048/diff/23002/bus/global.h#newcode5
bus/global.h:5: * Copyright (c) 2010 Google, Inc.
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> not sure but 2010-2012 or 2012, maybe?

Done.

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.c
File bus/ibusimpl.c (right):

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.c#newcode60
bus/ibusimpl.c:60: gboolean enable_by_default;
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> looks like you can remove the member now.

Done.

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.c#newcode349
bus/ibusimpl.c:349: ibus->enable_by_default = TRUE;
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> remove

Done.

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.c#newcode477
bus/ibusimpl.c:477: engine_name = "xkb:us::eng";
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> nit: what about moving the literal to global.h?

Done.

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.c#newcode799
bus/ibusimpl.c:799: if (ibus->enable_by_default) {
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> remove the if.

Done.

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.h
File bus/ibusimpl.h (right):

http://codereview.appspot.com/5517048/diff/23002/bus/ibusimpl.h#newcode80
bus/ibusimpl.h:80: gboolean         bus_ibus_impl_filter_keyboard_shortcuts
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> remove the prototype.

Done.

http://codereview.appspot.com/5517048/diff/23002/bus/inputcontext.c
File bus/inputcontext.c (right):

http://codereview.appspot.com/5517048/diff/23002/bus/inputcontext.c#newcode145
bus/inputcontext.c:145: /*
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> is the comment (line 145-163) still necessary?

Done.

http://codereview.appspot.com/5517048/diff/23002/bus/inputcontext.c#newcode1084
bus/inputcontext.c:1084: #if 0
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> why the block is commented out?
It is for fixing a focus related issue.

This code will request to create engine instance asynchronously for the input
context, when this IC is going to get focus. But ibusimpl will pass the global
engine to this IC after this function. And then when async request is done, it
will overwrite the global engine.

I removed those code. Done

http://codereview.appspot.com/5517048/diff/23002/bus/inputcontext.c#newcode1969
bus/inputcontext.c:1969: /* FIXME Do we need to emit "enabled" signal? */
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> nit: remove the comment.

Done.

http://codereview.appspot.com/5517048/diff/23002/bus/matchrule.c
File bus/matchrule.c (right):

http://codereview.appspot.com/5517048/diff/23002/bus/matchrule.c#newcode272
bus/matchrule.c:272: // g_debug("failed at: \"%s\"", p);
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> remove?

Done.

http://codereview.appspot.com/5517048/diff/23002/bus/test-client.c
File bus/test-client.c (right):

http://codereview.appspot.com/5517048/diff/23002/bus/test-client.c#newcode115
bus/test-client.c:115: // ibus_input_context_enable (client->ibuscontext);
On 2012/01/23 02:45:12, komatsu1 wrote:
> Please delete this line, or add a comment describing why this is commented
out.
ibus_input_context_enable() has been removed.

http://codereview.appspot.com/5517048/diff/23002/conf/gconf/Makefile.am
File conf/gconf/Makefile.am (right):

http://codereview.appspot.com/5517048/diff/23002/conf/gconf/Makefile.am#newcode1
conf/gconf/Makefile.am:1: # vim:set noet ts=4:
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> I'm not an expert of Linux desktop, but can't we just drop gconf support from
> the new version of ibus?

I am not sure. I think it is better to keep it. I don't know if all Linux
distributions do not use gconf in latest release.

http://codereview.appspot.com/5517048/diff/23002/configure.ac
File configure.ac (right):

http://codereview.appspot.com/5517048/diff/23002/configure.ac#newcode429
configure.ac:429: # --enable-surrounding-text option.
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> How about enabling the feature by default? i.e. remove
--enable-surrounding-text
> and add --disable-surrounding-text.
> 
> ibus-mozc recently started using the feature and it's important for the engine
> that the feature is supported by IBus on various Linux distributions.
> 

Done.

http://codereview.appspot.com/5517048/diff/23002/src/ibusbus.c
File src/ibusbus.c (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibusbus.c#newcode1482
src/ibusbus.c:1482: ibus_bus_get_engines_by_names (IBusBus             *bus,
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> please write a unit test for the new IPC.

Done.

http://codereview.appspot.com/5517048/diff/23002/src/ibusbus.h
File src/ibusbus.h (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibusbus.h#newcode717
src/ibusbus.h:717: * @returns: (transfer container) (element-type
IBusEngineDesc): A %NULL-terminated array of engines.
On 2012/01/21 05:35:47, fujiwara wrote:
> Please use (array zero-terminated=1) instead of (transfer container):
>
https://github.com/fujiwarat/ibus/commit/df9e6524a7738ed7d535352fd0754e487aa0...

I changed them to :

* @names: (array zero-terminated=1): A %NULL-terminated array of names.
* @returns: (array zero-terminated=1) (transfer full): A %NULL-terminated array
of engines.

Is it right?

http://codereview.appspot.com/5517048/diff/23002/src/ibusenginesimple.c
File src/ibusenginesimple.c (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibusenginesimple.c#newco...
src/ibusenginesimple.c:53: /* From the values below, the value 23 means the
number of different first keysyms
On 2012/01/23 02:45:12, komatsu1 wrote:
> 24?

Done.

http://codereview.appspot.com/5517048/diff/23002/src/ibusenginesimple.c#newco...
src/ibusenginesimple.c:86: static void     ibus_engine_simple_reset       
(IBusEngine         *engine);
On 2012/01/23 02:45:12, komatsu1 wrote:
> Would you add a comment why this type is not IBusEngineSimple?
This function is for overriding parent's virtual function. It should keep same
signatures of parent's virtual function. Otherwise, it should be cast to same
signature when assign the functions pointer to parent's vtable.

http://codereview.appspot.com/5517048/diff/23002/src/ibusenginesimple.h
File src/ibusenginesimple.h (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibusenginesimple.h#newcode1
src/ibusenginesimple.h:1: /* -*- mode: C; c-basic-offset: 4; indent-tabs-mode:
nil; -*- */
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> Can you let me know why we have to do the dead key composition in ibus-daemon
> rather then in an application?

1. It is not in ibus-daemon. It will be an engine (as ibus-pinyin and etc).
2. It is because xim issue. XIM apps do not load a module into their process (
do not like gtkimmodule or qtimmodule), so we can not do it in XIM apps.

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeynames.c
File src/ibuskeynames.c (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeynames.c#newcode135
src/ibuskeynames.c:135: g_return_val_if_fail (keyval != IBUS_KEY_VoidSymbol,
NULL);
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> nit: tab to space

Done.

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeynames.c#newcode192
src/ibuskeynames.c:192: if (*keyval != IBUS_KEY_VoidSymbol)
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> ditto

Done.

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeysyms-compat.h
File src/ibuskeysyms-compat.h (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeysyms-compat.h#new...
src/ibuskeysyms-compat.h:43: #ifndef __IBUS_KEYSYMS_COMPACT_H__
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> s/compact/compat/

Done.

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeysyms-compat.h#new...
src/ibuskeysyms-compat.h:44: #define __IBUS_KEYSYMS_COMPACT_H__
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> the same as above.

Done.

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeysyms-compat.h#new...
src/ibuskeysyms-compat.h:2099: #endif /* __IBUS_KEYSYMS_COMPACT_H__ */
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> same

Done.

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeysyms.h
File src/ibuskeysyms.h (left):

http://codereview.appspot.com/5517048/diff/23002/src/ibuskeysyms.h#oldcode1
src/ibuskeysyms.h:1: /* -*- mode: C; c-basic-offset: 4; indent-tabs-mode: nil;
-*- */
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> keep the line?

Done.

http://codereview.appspot.com/5517048/diff/23002/src/ibusutil.c
File src/ibusutil.c (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibusutil.c#newcode37
src/ibusutil.c:37: #define N_(t) t
On 2012/01/23 02:45:12, komatsu1 wrote:
> Would you add a comment describing your intention?

_() N_() are used for gettext. some tool of gettext will get all strings in _()
and N_() for generating i18n po files.

The different is _() will call gettext() to translate origin string to different
language by LANG settings.
and N_() is just a tag for gettext tools, but do not call gettext().

Done

http://codereview.appspot.com/5517048/diff/23002/src/ibusutil.c#newcode102
src/ibusutil.c:102: // filename = g_build_filename (ISOCODES_PREFIX,
On 2012/01/23 02:45:12, komatsu1 wrote:
> Please double check this line.

Done.

http://codereview.appspot.com/5517048/diff/23002/src/ibusxml.c
File src/ibusxml.c (right):

http://codereview.appspot.com/5517048/diff/23002/src/ibusxml.c#newcode37
src/ibusxml.c:37: g_list_free_full (node->sub_nodes,
(GDestroyNotify)ibus_xml_free);
On 2012/01/23 03:09:07, Yusuke Sato wrote:
> one space after 'Notify)'.

Done.

http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py
File ui/gtk2/panel.py (left):

http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py#oldcode220
ui/gtk2/panel.py:220: def focus_in(self, ic):
On 2012/01/23 03:01:35, nona1 wrote:
> Is gtk2 panel also support hot key binding?
> If not, how about remove gtk2 related codes?

Gtk2 panel can not work with latest ibus. I removed it from configure.ac and
Makefile.am. So those code will be not released in tarball.But they are needed
as reference for developing the gtk3 panel. So I think it is better to keep them
for a while in git repo. And I will remove them in future.

Done

http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala
File ui/gtk3/panel.vala (right):

http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala#newcode44
ui/gtk3/panel.vala:44: private const string ACCELERATOR_SWITCH_IME_BACKWARD =
"<Control><Shift>space";
On 2012/01/21 05:35:47, fujiwara wrote:
> meta_display_add_keybinding() supports to handle reverse hotkeys with Shift.
So
> we may need ACCELERATOR_SWITCH_IME_FOREWARD only but not need
> ACCELERATOR_SWITCH_IME_BACKWARD.
> Also meta_display_add_keybinding() needs to access gsettings key so I'm
thinking
> to add /desktop/ibus/general/hotkey/trigger-accel .

That's great!

http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala#newcode122
ui/gtk3/panel.vala:122: string cmdline = "setxkbmap
%s".printf(engine.get_layout());
On 2012/01/21 05:35:47, fujiwara wrote:
> If the layout == 'default', I'd like not to call setxkbmap.

If an IME's layout is 'default', which xkb layout should be used?

[fujiwara, 2012-01-24 01:37:31]

On 2012/01/23 19:33:47, Peng wrote:
> http://codereview.appspot.com/5517048/diff/23002/src/ibusbus.h
> File src/ibusbus.h (right):
> 
> http://codereview.appspot.com/5517048/diff/23002/src/ibusbus.h#newcode717
> src/ibusbus.h:717: * @returns: (transfer container) (element-type
> IBusEngineDesc): A %NULL-terminated array of engines.
> On 2012/01/21 05:35:47, fujiwara wrote:
> > Please use (array zero-terminated=1) instead of (transfer container):
> >
>
https://github.com/fujiwarat/ibus/commit/df9e6524a7738ed7d535352fd0754e487aa0...
> 
> I changed them to :
> 
> * @names: (array zero-terminated=1): A %NULL-terminated array of names.
> * @returns: (array zero-terminated=1) (transfer full): A %NULL-terminated
array
> of engines.
> 
> Is it right?

My understanding is the mixing full and none might be trouble.
Probably I think:
 * @returns: (array zero-terminated=1) (transfer none): A %NULL-terminated array

But actually I don't understand the definition.

> http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py
> File ui/gtk2/panel.py (left):
> 
> http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py#oldcode220
> ui/gtk2/panel.py:220: def focus_in(self, ic):
> On 2012/01/23 03:01:35, nona1 wrote:
> > Is gtk2 panel also support hot key binding?
> > If not, how about remove gtk2 related codes?
> 
> Gtk2 panel can not work with latest ibus. I removed it from configure.ac and
> Makefile.am. So those code will be not released in tarball.But they are needed
> as reference for developing the gtk3 panel. So I think it is better to keep
them
> for a while in git repo. And I will remove them in future.

I hope the gtk2 options are back at present.
$datadir/ibus/ui/gtk2 is not needed but $libdir/python/site-packages/ibus is
still needed.

> http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala#newcode122
> ui/gtk3/panel.vala:122: string cmdline = "setxkbmap
> %s".printf(engine.get_layout());
> On 2012/01/21 05:35:47, fujiwara wrote:
> > If the layout == 'default', I'd like not to call setxkbmap.
> 
> If an IME's layout is 'default', which xkb layout should be used?

It's the current keymap since ibus doesn't change the keymap.
If the previous engine was xkb engine, it would be the previous keymap. If the
previous engine was input method, it might be the session layout.
I would think almost engines use the default layout by default except for XKB
engines and changing xkb keymaps by Control + Space would be a cost.
Currently I'm asking to use ibus_keymap_lookup_keysym directly for ibus-m17n.

[fujiwara, 2012-01-24 05:44:43]

http://codereview.appspot.com/5517048/diff/34004/ui/gtk3/panel.vala
File ui/gtk3/panel.vala (right):

http://codereview.appspot.com/5517048/diff/34004/ui/gtk3/panel.vala#newcode326
ui/gtk3/panel.vala:326: var desc = engine.get_description();
I think get_longname() is better than get_description() because get_description
returns too long multiple lines.

[Peng, 2012-01-24 18:14:27]

http://codereview.appspot.com/5517048/diff/34004/ui/gtk3/panel.vala
File ui/gtk3/panel.vala (right):

http://codereview.appspot.com/5517048/diff/34004/ui/gtk3/panel.vala#newcode326
ui/gtk3/panel.vala:326: var desc = engine.get_description();
On 2012/01/24 05:44:43, fujiwara wrote:
> I think get_longname() is better than get_description() because
get_description
> returns too long multiple lines.

Done.

[Peng, 2012-01-24 22:31:05]

http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py
File ui/gtk2/panel.py (left):

http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py#oldcode220
ui/gtk2/panel.py:220: def focus_in(self, ic):
On 2012/01/23 19:33:47, Peng wrote:
> On 2012/01/23 03:01:35, nona1 wrote:
> > Is gtk2 panel also support hot key binding?
> > If not, how about remove gtk2 related codes?
> 
> Gtk2 panel can not work with latest ibus. I removed it from configure.ac and
> Makefile.am. So those code will be not released in tarball.But they are needed
> as reference for developing the gtk3 panel. So I think it is better to keep
them
> for a while in git repo. And I will remove them in future.
> 
> Done
> 
> 

I just removed gtk2 panel from release tarball. The ibus python library is still
in release tarball. But it is better to port all python ibus engine to g-i. I
plan to stop maintain ibus python library.

http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala
File ui/gtk3/panel.vala (right):

http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala#newcode122
ui/gtk3/panel.vala:122: string cmdline = "setxkbmap
%s".printf(engine.get_layout());
On 2012/01/23 19:33:47, Peng wrote:
> On 2012/01/21 05:35:47, fujiwara wrote:
> > If the layout == 'default', I'd like not to call setxkbmap.
> 
> If an IME's layout is 'default', which xkb layout should be used?
> 
> 

> Fujiwara:
> It's the current keymap since ibus doesn't change the keymap.
> If the previous engine was xkb engine, it would be the previous keymap. If the
> previous engine was input method, it might be the session layout.
> I would think almost engines use the default layout by default except for XKB
> engines and changing xkb keymaps by Control + Space would be a cost.
> Currently I'm asking to use ibus_keymap_lookup_keysym directly for ibus-m17n.

I think it is little complex. It is better to do more investigate to find out
how to resolve this issue in a better way. So I would like to make it as a
future task.

http://codereview.appspot.com/5517048/diff/33033/src/ibusbus.h
File src/ibusbus.h (right):

http://codereview.appspot.com/5517048/diff/33033/src/ibusbus.h#newcode717
src/ibusbus.h:717: * @returns: (array zero-terminated=1) (transfer full): A
%NULL-terminated array of engines.
http://live.gnome.org/GObjectIntrospection/Annotations#Transfer_Modes

Base on the document, I think transfer full is correct. Because caller will onw
the array and elements.

[fujiwara, 2012-01-25 05:29:44]

On 2012/01/24 22:31:05, Peng wrote:
> http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py
> File ui/gtk2/panel.py (left):
> 
> http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py#oldcode220
> ui/gtk2/panel.py:220: def focus_in(self, ic):
> On 2012/01/23 19:33:47, Peng wrote:
> > On 2012/01/23 03:01:35, nona1 wrote:
> > > Is gtk2 panel also support hot key binding?
> > > If not, how about remove gtk2 related codes?
> > 
> > Gtk2 panel can not work with latest ibus. I removed it from configure.ac and
> > Makefile.am. So those code will be not released in tarball.But they are
needed
> > as reference for developing the gtk3 panel. So I think it is better to keep
> them
> > for a while in git repo. And I will remove them in future.
> > 
> > Done
> > 
> > 
> 
> I just removed gtk2 panel from release tarball. The ibus python library is
still
> in release tarball. But it is better to port all python ibus engine to g-i. I
> plan to stop maintain ibus python library.
> 
> http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala
> File ui/gtk3/panel.vala (right):
> 
> http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala#newcode122
> ui/gtk3/panel.vala:122: string cmdline = "setxkbmap
> %s".printf(engine.get_layout());
> On 2012/01/23 19:33:47, Peng wrote:
> > On 2012/01/21 05:35:47, fujiwara wrote:
> > > If the layout == 'default', I'd like not to call setxkbmap.
> > 
> > If an IME's layout is 'default', which xkb layout should be used?
> > 
> > 
> 
> > Fujiwara:
> > It's the current keymap since ibus doesn't change the keymap.
> > If the previous engine was xkb engine, it would be the previous keymap. If
the
> > previous engine was input method, it might be the session layout.
> > I would think almost engines use the default layout by default except for
XKB
> > engines and changing xkb keymaps by Control + Space would be a cost.
> > Currently I'm asking to use ibus_keymap_lookup_keysym directly for
ibus-m17n.
> 
> I think it is little complex. It is better to do more investigate to find out
> how to resolve this issue in a better way. So I would like to make it as a
> future task.

I think the 'default' is similar with use_system_keyboard_layout option and more
enhanced.
Using the default mode is why we are moving input-method on/off to switching
engines in Fedora.
ibus-m17n would be a separated topic.

> 
> http://codereview.appspot.com/5517048/diff/33033/src/ibusbus.h
> File src/ibusbus.h (right):
> 
> http://codereview.appspot.com/5517048/diff/33033/src/ibusbus.h#newcode717
> src/ibusbus.h:717: * @returns: (array zero-terminated=1) (transfer full): A
> %NULL-terminated array of engines.
> http://live.gnome.org/GObjectIntrospection/Annotations#Transfer_Modes
> 
> Base on the document, I think transfer full is correct. Because caller will
onw
> the array and elements.

If I use full, gnome-shell always gets SEGV.
I think mixing full and none would be wrong as we got the bug report previously.

[Peng, 2012-01-25 15:26:35]

On 2012/01/25 05:29:44, fujiwara wrote:
> On 2012/01/24 22:31:05, Peng wrote:
> > http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py
> > File ui/gtk2/panel.py (left):
> > 
> > http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py#oldcode220
> > ui/gtk2/panel.py:220: def focus_in(self, ic):
> > On 2012/01/23 19:33:47, Peng wrote:
> > > On 2012/01/23 03:01:35, nona1 wrote:
> > > > Is gtk2 panel also support hot key binding?
> > > > If not, how about remove gtk2 related codes?
> > > 
> > > Gtk2 panel can not work with latest ibus. I removed it from configure.ac
and
> > > Makefile.am. So those code will be not released in tarball.But they are
> needed
> > > as reference for developing the gtk3 panel. So I think it is better to
keep
> > them
> > > for a while in git repo. And I will remove them in future.
> > > 
> > > Done
> > > 
> > > 
> > 
> > I just removed gtk2 panel from release tarball. The ibus python library is
> still
> > in release tarball. But it is better to port all python ibus engine to g-i.
I
> > plan to stop maintain ibus python library.
> > 
> > http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala
> > File ui/gtk3/panel.vala (right):
> > 
> >
http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala#newcode122
> > ui/gtk3/panel.vala:122: string cmdline = "setxkbmap
> > %s".printf(engine.get_layout());
> > On 2012/01/23 19:33:47, Peng wrote:
> > > On 2012/01/21 05:35:47, fujiwara wrote:
> > > > If the layout == 'default', I'd like not to call setxkbmap.
> > > 
> > > If an IME's layout is 'default', which xkb layout should be used?
> > > 
> > > 
> > 
> > > Fujiwara:
> > > It's the current keymap since ibus doesn't change the keymap.
> > > If the previous engine was xkb engine, it would be the previous keymap. If
> the
> > > previous engine was input method, it might be the session layout.
> > > I would think almost engines use the default layout by default except for
> XKB
> > > engines and changing xkb keymaps by Control + Space would be a cost.
> > > Currently I'm asking to use ibus_keymap_lookup_keysym directly for
> ibus-m17n.
> > 
> > I think it is little complex. It is better to do more investigate to find
out
> > how to resolve this issue in a better way. So I would like to make it as a
> > future task.
> 
> I think the 'default' is similar with use_system_keyboard_layout option and
more
> enhanced.
> Using the default mode is why we are moving input-method on/off to switching
> engines in Fedora.
> ibus-m17n would be a separated topic.

I see. I think it could be implemented in gnome-shell's ibus panel, right? At
least, I think it is not necessary to implement it in ibus gtk3 panel right now.

> 
> > 
> > http://codereview.appspot.com/5517048/diff/33033/src/ibusbus.h
> > File src/ibusbus.h (right):
> > 
> > http://codereview.appspot.com/5517048/diff/33033/src/ibusbus.h#newcode717
> > src/ibusbus.h:717: * @returns: (array zero-terminated=1) (transfer full): A
> > %NULL-terminated array of engines.
> > http://live.gnome.org/GObjectIntrospection/Annotations#Transfer_Modes
> > 
> > Base on the document, I think transfer full is correct. Because caller will
> onw
> > the array and elements.
> 
> If I use full, gnome-shell always gets SEGV.
> I think mixing full and none would be wrong as we got the bug report
previously.

Could you tell me detail of the SEGV? And please test it with latest code in my
branch. I think it should not happen.

BTW, what do you mean mixing full and none? In here, the ownership is passed to
caller. Use none will cause memory leak.

I test below code. It works fine.

 3 const IBus = imports.gi.IBus;
  4 
  5 IBus.init();
  6 bus = new IBus.Bus();
  7 print("connected = " + bus.is_connected());
  8 names = ["pinyin", "anthy"];
  9 engines = bus.get_engines_by_names(names);
 10 
 11 for(e in engines) {
 12   print("name = " + engines[e].get_longname());
 13 }
 14 
 15 engines = null;

[fujiwara, 2012-01-26 01:54:30]

On 2012/01/25 15:26:35, Peng wrote:
> On 2012/01/25 05:29:44, fujiwara wrote:
> > On 2012/01/24 22:31:05, Peng wrote:
> > > http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py
> > > File ui/gtk2/panel.py (left):
> > > 
> > >
http://codereview.appspot.com/5517048/diff/23002/ui/gtk2/panel.py#oldcode220
> > > ui/gtk2/panel.py:220: def focus_in(self, ic):
> > > On 2012/01/23 19:33:47, Peng wrote:
> > > > On 2012/01/23 03:01:35, nona1 wrote:
> > > > > Is gtk2 panel also support hot key binding?
> > > > > If not, how about remove gtk2 related codes?
> > > > 
> > > > Gtk2 panel can not work with latest ibus. I removed it from configure.ac
> and
> > > > Makefile.am. So those code will be not released in tarball.But they are
> > needed
> > > > as reference for developing the gtk3 panel. So I think it is better to
> keep
> > > them
> > > > for a while in git repo. And I will remove them in future.
> > > > 
> > > > Done
> > > > 
> > > > 
> > > 
> > > I just removed gtk2 panel from release tarball. The ibus python library is
> > still
> > > in release tarball. But it is better to port all python ibus engine to
g-i.
> I
> > > plan to stop maintain ibus python library.
> > > 
> > > http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala
> > > File ui/gtk3/panel.vala (right):
> > > 
> > >
> http://codereview.appspot.com/5517048/diff/23002/ui/gtk3/panel.vala#newcode122
> > > ui/gtk3/panel.vala:122: string cmdline = "setxkbmap
> > > %s".printf(engine.get_layout());
> > > On 2012/01/23 19:33:47, Peng wrote:
> > > > On 2012/01/21 05:35:47, fujiwara wrote:
> > > > > If the layout == 'default', I'd like not to call setxkbmap.
> > > > 
> > > > If an IME's layout is 'default', which xkb layout should be used?
> > > > 
> > > > 
> > > 
> > > > Fujiwara:
> > > > It's the current keymap since ibus doesn't change the keymap.
> > > > If the previous engine was xkb engine, it would be the previous keymap.
If
> > the
> > > > previous engine was input method, it might be the session layout.
> > > > I would think almost engines use the default layout by default except
for
> > XKB
> > > > engines and changing xkb keymaps by Control + Space would be a cost.
> > > > Currently I'm asking to use ibus_keymap_lookup_keysym directly for
> > ibus-m17n.
> > > 
> > > I think it is little complex. It is better to do more investigate to find
> out
> > > how to resolve this issue in a better way. So I would like to make it as a
> > > future task.
> > 
> > I think the 'default' is similar with use_system_keyboard_layout option and
> more
> > enhanced.
> > Using the default mode is why we are moving input-method on/off to switching
> > engines in Fedora.
> > ibus-m17n would be a separated topic.
> 
> I see. I think it could be implemented in gnome-shell's ibus panel, right? At
> least, I think it is not necessary to implement it in ibus gtk3 panel right
now.

Hmm.., I'm thinking both needs the implementation as our bridge-hotkey feature
but probably I think the discussion can be moved to ibus-xkb integration over
this patch later.

> 
> > 
> > > 
> > > http://codereview.appspot.com/5517048/diff/33033/src/ibusbus.h
> > > File src/ibusbus.h (right):
> > > 
> > > http://codereview.appspot.com/5517048/diff/33033/src/ibusbus.h#newcode717
> > > src/ibusbus.h:717: * @returns: (array zero-terminated=1) (transfer full):
A
> > > %NULL-terminated array of engines.
> > > http://live.gnome.org/GObjectIntrospection/Annotations#Transfer_Modes
> > > 
> > > Base on the document, I think transfer full is correct. Because caller
will
> > onw
> > > the array and elements.
> > 
> > If I use full, gnome-shell always gets SEGV.
> > I think mixing full and none would be wrong as we got the bug report
> previously.
> 
> Could you tell me detail of the SEGV? And please test it with latest code in
my
> branch. I think it should not happen.
> 
> BTW, what do you mean mixing full and none? In here, the ownership is passed
to
> caller. Use none will cause memory leak.

OK, thanks for your confirmation. I tested your branch and confirmed it works
fine.
Probably the changing 'gchar**' to 'const gchar* const *' was lost in my test
yesterday.

> 
> I test below code. It works fine.
> 
>  3 const IBus = imports.gi.IBus;
>   4 
>   5 IBus.init();
>   6 bus = new IBus.Bus();
>   7 print("connected = " + bus.is_connected());
>   8 names = ["pinyin", "anthy"];
>   9 engines = bus.get_engines_by_names(names);
>  10 
>  11 for(e in engines) {
>  12   print("name = " + engines[e].get_longname());
>  13 }
>  14 
>  15 engines = null;

[fujiwara, 2012-01-27 06:00:53]

Probably I think Quit button is useful for users.
ibus-gjs removes that button but it's due to the ibus & xkb integration.
The ibus gtk button handles ibus UI only so it would be nice to provide the way
to quit the application.
Also to exit ibus-daemon with ibus-gjs, I'm thinking it might be good to provide
an option in ibus-daemon to quit itself.
e.g. ibus-daemon -q

Probably I think it's good to release the current master (1.4.1?) before the new
ibus integration (1.5.0?). 

There is a still discussion if the new Control + space is useful.
Some Java applications (e.g. eclipse) also bind Control + Space.
Now the new ibus binds Control + Space even if no input methods but xkb engines
only. It would be a duplicated key bindings between ibus and others.
My understanding is that the solution is to move ibus settings to
gnome-control-center settings. I'm not sure about other desktops.

On 2012/01/09 17:57:25, Peng wrote:

[Peng, 2012-01-27 15:32:28]

On 2012/01/27 06:00:53, fujiwara wrote:
> Probably I think Quit button is useful for users.
> ibus-gjs removes that button but it's due to the ibus & xkb integration.
> The ibus gtk button handles ibus UI only so it would be nice to provide the
way
> to quit the application.
> Also to exit ibus-daemon with ibus-gjs, I'm thinking it might be good to
provide
> an option in ibus-daemon to quit itself.
> e.g. ibus-daemon -q

I added the quit item in menu, and in the branch, I created a new command line
tool `ibus`. It could be used for exit or restart ibus daemon. Please it.


> 
> Probably I think it's good to release the current master (1.4.1?) before the
new
> ibus integration (1.5.0?). 

Sure.

> 
> There is a still discussion if the new Control + space is useful.
> Some Java applications (e.g. eclipse) also bind Control + Space.
> Now the new ibus binds Control + Space even if no input methods but xkb
engines
> only. It would be a duplicated key bindings between ibus and others.
> My understanding is that the solution is to move ibus settings to
> gnome-control-center settings. I'm not sure about other desktops.

So probably, we could add a way to disable the ime keybinding. So if users don't
like it, they could disable the ime keybinding, and then use the menu for
switching IMEs. what do you think?

> 
> On 2012/01/09 17:57:25, Peng wrote:

[fujiwara, 2012-01-31 01:27:56]

On 2012/01/27 15:32:28, Peng wrote:
> On 2012/01/27 06:00:53, fujiwara wrote:
> > Probably I think Quit button is useful for users.
> > ibus-gjs removes that button but it's due to the ibus & xkb integration.
> > The ibus gtk button handles ibus UI only so it would be nice to provide the
> way
> > to quit the application.
> > Also to exit ibus-daemon with ibus-gjs, I'm thinking it might be good to
> provide
> > an option in ibus-daemon to quit itself.
> > e.g. ibus-daemon -q
> 
> I added the quit item in menu, and in the branch, I created a new command line
> tool `ibus`. It could be used for exit or restart ibus daemon. Please it.
> 
> 
> > 
> > Probably I think it's good to release the current master (1.4.1?) before the
> new
> > ibus integration (1.5.0?). 
> 
> Sure.
> 
> > 
> > There is a still discussion if the new Control + space is useful.
> > Some Java applications (e.g. eclipse) also bind Control + Space.
> > Now the new ibus binds Control + Space even if no input methods but xkb
> engines
> > only. It would be a duplicated key bindings between ibus and others.
> > My understanding is that the solution is to move ibus settings to
> > gnome-control-center settings. I'm not sure about other desktops.
> 
> So probably, we could add a way to disable the ime keybinding. So if users
don't
> like it, they could disable the ime keybinding, and then use the menu for
> switching IMEs. what do you think?

Maybe I think it's a good idea but you indicates another issue.
You're talking about which Control+Space and old Control+Space people like to
use. However I'm talking about which application owns the keybinding of
Control+Space by default. I said I have used Control+Space for input methods for
more than ten years but Java people also said Control+Space for Java.
XKB keyboard option doesn't have the keybinding of Control+Space. MacOS is
Command + Space.
However probably I'd like to use Control+Space for ibus by default. I'm not sure
about other desktops except for GNOME.

> 
> > 
> > On 2012/01/09 17:57:25, Peng wrote:

[fujiwara, 2012-01-31 01:34:25]

On 2012/01/31 01:27:56, fujiwara wrote:
> On 2012/01/27 15:32:28, Peng wrote:
> > On 2012/01/27 06:00:53, fujiwara wrote:
> > > Probably I think Quit button is useful for users.
> > > ibus-gjs removes that button but it's due to the ibus & xkb integration.
> > > The ibus gtk button handles ibus UI only so it would be nice to provide
the
> > way
> > > to quit the application.
> > > Also to exit ibus-daemon with ibus-gjs, I'm thinking it might be good to
> > provide
> > > an option in ibus-daemon to quit itself.
> > > e.g. ibus-daemon -q
> > 
> > I added the quit item in menu, and in the branch, I created a new command
line
> > tool `ibus`. It could be used for exit or restart ibus daemon. Please it.
> > 
> > 
> > > 
> > > Probably I think it's good to release the current master (1.4.1?) before
the
> > new
> > > ibus integration (1.5.0?). 
> > 
> > Sure.
> > 
> > > 
> > > There is a still discussion if the new Control + space is useful.
> > > Some Java applications (e.g. eclipse) also bind Control + Space.
> > > Now the new ibus binds Control + Space even if no input methods but xkb
> > engines
> > > only. It would be a duplicated key bindings between ibus and others.
> > > My understanding is that the solution is to move ibus settings to
> > > gnome-control-center settings. I'm not sure about other desktops.
> > 
> > So probably, we could add a way to disable the ime keybinding. So if users
> don't
> > like it, they could disable the ime keybinding, and then use the menu for
> > switching IMEs. what do you think?
> 
> Maybe I think it's a good idea but you indicates another issue.
> You're talking about which Control+Space and old Control+Space people like to
> use. However I'm talking about which application owns the keybinding of
> Control+Space by default. I said I have used Control+Space for input methods
for
> more than ten years but Java people also said Control+Space for Java.
> XKB keyboard option doesn't have the keybinding of Control+Space. MacOS is
> Command + Space.
> However probably I'd like to use Control+Space for ibus by default. I'm not
sure
> about other desktops except for GNOME.

s/which Control+Space and old Control+Space/which new Control+Space and old
Control+Space/

I mean we will enhance gnome-control-center for the ibus default keybidning but
I may not touch other desktops.

> 
> > 
> > > 
> > > On 2012/01/09 17:57:25, Peng wrote:

[fujiwara, 2012-01-31 09:22:07]

I prefer "ibus-keyboard" icon to "ibus-engine" icon for ibus XKB engines under
the new Control + Space.

On 2012/01/09 17:57:25, Peng wrote:

[Peng, 2012-01-31 15:47:58]

On 2012/01/31 01:27:56, fujiwara wrote:
> On 2012/01/27 15:32:28, Peng wrote:
> > On 2012/01/27 06:00:53, fujiwara wrote:
> > > Probably I think Quit button is useful for users.
> > > ibus-gjs removes that button but it's due to the ibus & xkb integration.
> > > The ibus gtk button handles ibus UI only so it would be nice to provide
the
> > way
> > > to quit the application.
> > > Also to exit ibus-daemon with ibus-gjs, I'm thinking it might be good to
> > provide
> > > an option in ibus-daemon to quit itself.
> > > e.g. ibus-daemon -q
> > 
> > I added the quit item in menu, and in the branch, I created a new command
line
> > tool `ibus`. It could be used for exit or restart ibus daemon. Please it.
> > 
> > 
> > > 
> > > Probably I think it's good to release the current master (1.4.1?) before
the
> > new
> > > ibus integration (1.5.0?). 
> > 
> > Sure.
> > 
> > > 
> > > There is a still discussion if the new Control + space is useful.
> > > Some Java applications (e.g. eclipse) also bind Control + Space.
> > > Now the new ibus binds Control + Space even if no input methods but xkb
> > engines
> > > only. It would be a duplicated key bindings between ibus and others.
> > > My understanding is that the solution is to move ibus settings to
> > > gnome-control-center settings. I'm not sure about other desktops.
> > 
> > So probably, we could add a way to disable the ime keybinding. So if users
> don't
> > like it, they could disable the ime keybinding, and then use the menu for
> > switching IMEs. what do you think?
> 
> Maybe I think it's a good idea but you indicates another issue.
> You're talking about which Control+Space and old Control+Space people like to
> use. However I'm talking about which application owns the keybinding of
> Control+Space by default. I said I have used Control+Space for input methods
for
> more than ten years but Java people also said Control+Space for Java.
> XKB keyboard option doesn't have the keybinding of Control+Space. MacOS is
> Command + Space.
> However probably I'd like to use Control+Space for ibus by default. I'm not
sure
> about other desktops except for GNOME.

I guess most IME users use C+Space for IMEs, even if they are using Java. So it
is not a problem for them.

For users only use XKB, they can disable C+Space for IME. Only use the menu for
switching xkb.

So I think a configurable option should resolve this issue.

[Peng, 2012-01-31 15:49:01]

On 2012/01/31 09:22:07, fujiwara wrote:
> I prefer "ibus-keyboard" icon to "ibus-engine" icon for ibus XKB engines under
> the new Control + Space.

I changed the icon to ibus-leyboard for all xkb layouts.

[fujiwara, 2012-02-01 04:11:59]

http://codereview.appspot.com/5517048/diff/39001/setup/main.py
File setup/main.py (right):

http://codereview.appspot.com/5517048/diff/39001/setup/main.py#newcode257
setup/main.py:257: name = str(engine.name)
s/engine.name/engine.props.name/

http://codereview.appspot.com/5517048/diff/39001/setup/main.py#newcode282
setup/main.py:282: #     obj.set_sensitive(False)
I think those lines could be uncommented.

http://codereview.appspot.com/5517048/diff/39001/setup/main.py#newcode305
setup/main.py:305: name = engine.name
s/engine.name/engine.props.name/

[fujiwara, 2012-02-01 08:18:35]

I confirmed 'ibus exit' and 'ibus restart' command.

http://codereview.appspot.com/5517048/diff/39001/tools/main.vala
File tools/main.vala (right):

http://codereview.appspot.com/5517048/diff/39001/tools/main.vala#newcode149
tools/main.vala:149: string[] new_argv = argv[1:argv.length];
Maybe I think it's good to add help argument:

+        if (argv[1] == "--help" || argv[1] == "help" || argv[1] == "-h") {
+            foreach (var command in commands) {
+                print("  %s\n", command.name);
+            }
+            return 0;
+        }

[fujiwara, 2012-02-01 09:10:25]

On 2012/01/31 15:47:58, Peng wrote:
> On 2012/01/31 01:27:56, fujiwara wrote:
> > On 2012/01/27 15:32:28, Peng wrote:
> > > On 2012/01/27 06:00:53, fujiwara wrote:
> > > > Probably I think Quit button is useful for users.
> > > > ibus-gjs removes that button but it's due to the ibus & xkb integration.
> > > > The ibus gtk button handles ibus UI only so it would be nice to provide
> the
> > > way
> > > > to quit the application.
> > > > Also to exit ibus-daemon with ibus-gjs, I'm thinking it might be good to
> > > provide
> > > > an option in ibus-daemon to quit itself.
> > > > e.g. ibus-daemon -q
> > > 
> > > I added the quit item in menu, and in the branch, I created a new command
> line
> > > tool `ibus`. It could be used for exit or restart ibus daemon. Please it.
> > > 
> > > 
> > > > 
> > > > Probably I think it's good to release the current master (1.4.1?) before
> the
> > > new
> > > > ibus integration (1.5.0?). 
> > > 
> > > Sure.
> > > 
> > > > 
> > > > There is a still discussion if the new Control + space is useful.
> > > > Some Java applications (e.g. eclipse) also bind Control + Space.
> > > > Now the new ibus binds Control + Space even if no input methods but xkb
> > > engines
> > > > only. It would be a duplicated key bindings between ibus and others.
> > > > My understanding is that the solution is to move ibus settings to
> > > > gnome-control-center settings. I'm not sure about other desktops.
> > > 
> > > So probably, we could add a way to disable the ime keybinding. So if users
> > don't
> > > like it, they could disable the ime keybinding, and then use the menu for
> > > switching IMEs. what do you think?
> > 
> > Maybe I think it's a good idea but you indicates another issue.
> > You're talking about which Control+Space and old Control+Space people like
to
> > use. However I'm talking about which application owns the keybinding of
> > Control+Space by default. I said I have used Control+Space for input methods
> for
> > more than ten years but Java people also said Control+Space for Java.
> > XKB keyboard option doesn't have the keybinding of Control+Space. MacOS is
> > Command + Space.
> > However probably I'd like to use Control+Space for ibus by default. I'm not
> sure
> > about other desktops except for GNOME.
> 
> I guess most IME users use C+Space for IMEs, even if they are using Java. So
it
> is not a problem for them.
> 
> For users only use XKB, they can disable C+Space for IME. Only use the menu
for
> switching xkb.
> 
> So I think a configurable option should resolve this issue.

OK, I will discuss this issue on bugzilla.

lgtm with comments.

[fujiwara, 2012-02-02 02:05:31]

On 2012/02/01 09:10:25, fujiwara wrote:
> On 2012/01/31 15:47:58, Peng wrote:
> > On 2012/01/31 01:27:56, fujiwara wrote:
> > > On 2012/01/27 15:32:28, Peng wrote:
> > > > On 2012/01/27 06:00:53, fujiwara wrote:
> > > > > Probably I think Quit button is useful for users.
> > > > > ibus-gjs removes that button but it's due to the ibus & xkb
integration.
> > > > > The ibus gtk button handles ibus UI only so it would be nice to
provide
> > the
> > > > way
> > > > > to quit the application.
> > > > > Also to exit ibus-daemon with ibus-gjs, I'm thinking it might be good
to
> > > > provide
> > > > > an option in ibus-daemon to quit itself.
> > > > > e.g. ibus-daemon -q
> > > > 
> > > > I added the quit item in menu, and in the branch, I created a new
command
> > line
> > > > tool `ibus`. It could be used for exit or restart ibus daemon. Please
it.
> > > > 
> > > > 
> > > > > 
> > > > > Probably I think it's good to release the current master (1.4.1?)
before
> > the
> > > > new
> > > > > ibus integration (1.5.0?). 
> > > > 
> > > > Sure.
> > > > 
> > > > > 
> > > > > There is a still discussion if the new Control + space is useful.
> > > > > Some Java applications (e.g. eclipse) also bind Control + Space.
> > > > > Now the new ibus binds Control + Space even if no input methods but
xkb
> > > > engines
> > > > > only. It would be a duplicated key bindings between ibus and others.
> > > > > My understanding is that the solution is to move ibus settings to
> > > > > gnome-control-center settings. I'm not sure about other desktops.
> > > > 
> > > > So probably, we could add a way to disable the ime keybinding. So if
users
> > > don't
> > > > like it, they could disable the ime keybinding, and then use the menu
for
> > > > switching IMEs. what do you think?
> > > 
> > > Maybe I think it's a good idea but you indicates another issue.
> > > You're talking about which Control+Space and old Control+Space people like
> to
> > > use. However I'm talking about which application owns the keybinding of
> > > Control+Space by default. I said I have used Control+Space for input
methods
> > for
> > > more than ten years but Java people also said Control+Space for Java.
> > > XKB keyboard option doesn't have the keybinding of Control+Space. MacOS is
> > > Command + Space.
> > > However probably I'd like to use Control+Space for ibus by default. I'm
not
> > sure
> > > about other desktops except for GNOME.
> > 
> > I guess most IME users use C+Space for IMEs, even if they are using Java. So
> it
> > is not a problem for them.
> > 
> > For users only use XKB, they can disable C+Space for IME. Only use the menu
> for
> > switching xkb.
> > 
> > So I think a configurable option should resolve this issue.
> 
> OK, I will discuss this issue on bugzilla.
> 
> lgtm with comments.

I think the two fixes are needed for src/ibusenginesimple.c:
https://github.com/ibus/ibus-xkb/commit/5a122df7d15842cfaafb349d7859a7b76d41f928
https://github.com/ibus/ibus-xkb/commit/64a8da3cd13783eb2c6c9dd952e02be63af4a832

[fujiwara, 2012-02-04 01:58:06]

On 2012/02/02 02:05:31, fujiwara wrote:
> On 2012/02/01 09:10:25, fujiwara wrote:
> > On 2012/01/31 15:47:58, Peng wrote:
> > > On 2012/01/31 01:27:56, fujiwara wrote:
> > > > On 2012/01/27 15:32:28, Peng wrote:
> > > > > On 2012/01/27 06:00:53, fujiwara wrote:
> > > > > > Probably I think Quit button is useful for users.
> > > > > > ibus-gjs removes that button but it's due to the ibus & xkb
> integration.
> > > > > > The ibus gtk button handles ibus UI only so it would be nice to
> provide
> > > the
> > > > > way
> > > > > > to quit the application.
> > > > > > Also to exit ibus-daemon with ibus-gjs, I'm thinking it might be
good
> to
> > > > > provide
> > > > > > an option in ibus-daemon to quit itself.
> > > > > > e.g. ibus-daemon -q
> > > > > 
> > > > > I added the quit item in menu, and in the branch, I created a new
> command
> > > line
> > > > > tool `ibus`. It could be used for exit or restart ibus daemon. Please
> it.
> > > > > 
> > > > > 
> > > > > > 
> > > > > > Probably I think it's good to release the current master (1.4.1?)
> before
> > > the
> > > > > new
> > > > > > ibus integration (1.5.0?). 
> > > > > 
> > > > > Sure.
> > > > > 
> > > > > > 
> > > > > > There is a still discussion if the new Control + space is useful.
> > > > > > Some Java applications (e.g. eclipse) also bind Control + Space.
> > > > > > Now the new ibus binds Control + Space even if no input methods but
> xkb
> > > > > engines
> > > > > > only. It would be a duplicated key bindings between ibus and others.
> > > > > > My understanding is that the solution is to move ibus settings to
> > > > > > gnome-control-center settings. I'm not sure about other desktops.
> > > > > 
> > > > > So probably, we could add a way to disable the ime keybinding. So if
> users
> > > > don't
> > > > > like it, they could disable the ime keybinding, and then use the menu
> for
> > > > > switching IMEs. what do you think?
> > > > 
> > > > Maybe I think it's a good idea but you indicates another issue.
> > > > You're talking about which Control+Space and old Control+Space people
like
> > to
> > > > use. However I'm talking about which application owns the keybinding of
> > > > Control+Space by default. I said I have used Control+Space for input
> methods
> > > for
> > > > more than ten years but Java people also said Control+Space for Java.
> > > > XKB keyboard option doesn't have the keybinding of Control+Space. MacOS
is
> > > > Command + Space.
> > > > However probably I'd like to use Control+Space for ibus by default. I'm
> not
> > > sure
> > > > about other desktops except for GNOME.
> > > 
> > > I guess most IME users use C+Space for IMEs, even if they are using Java.
So
> > it
> > > is not a problem for them.
> > > 
> > > For users only use XKB, they can disable C+Space for IME. Only use the
menu
> > for
> > > switching xkb.
> > > 
> > > So I think a configurable option should resolve this issue.
> > 
> > OK, I will discuss this issue on bugzilla.
> > 
> > lgtm with comments.
> 
> I think the two fixes are needed for src/ibusenginesimple.c:
>
https://github.com/ibus/ibus-xkb/commit/5a122df7d15842cfaafb349d7859a7b76d41f928
>
https://github.com/ibus/ibus-xkb/commit/64a8da3cd13783eb2c6c9dd952e02be63af4a832

It seems 'make distcheck' has several errors.
I integrated the devel directory & new ibus-gjs in Fedora rawhide today:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/ibus-1.4.99.20120203.tar.gz

The following patch is needed for the latest vala & gtk3:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/ibus-HEAD.patch

[fujiwara, 2012-02-06 11:38:17]

http://codereview.appspot.com/5517048/diff/39001/setup/enginecombobox.py
File setup/enginecombobox.py (right):

http://codereview.appspot.com/5517048/diff/39001/setup/enginecombobox.py#newc...
setup/enginecombobox.py:78: current_lang =
IBus.get_language_name(locale.getlocale()[0])
locale.getlocale()[0] == None on C locale

[shawn.p.huang_gmail.com, 2012-02-07 19:50:14]

On Fri, Feb 3, 2012 at 8:58 PM,  <takao.fujiwara1@gmail.com> wrote:
> On 2012/02/02 02:05:31, fujiwara wrote:
>>
>> On 2012/02/01 09:10:25, fujiwara wrote:
>> > On 2012/01/31 15:47:58, Peng wrote:
>> > > On 2012/01/31 01:27:56, fujiwara wrote:
>> > > > On 2012/01/27 15:32:28, Peng wrote:
>> > > > > On 2012/01/27 06:00:53, fujiwara wrote:
>> > > > > > Probably I think Quit button is useful for users.
>> > > > > > ibus-gjs removes that button but it's due to the ibus & xkb
>> integration.
>> > > > > > The ibus gtk button handles ibus UI only so it would be nice
>
> to
>>
>> provide
>> > > the
>> > > > > way
>> > > > > > to quit the application.
>> > > > > > Also to exit ibus-daemon with ibus-gjs, I'm thinking it
>
> might be good
>>
>> to
>> > > > > provide
>> > > > > > an option in ibus-daemon to quit itself.
>> > > > > > e.g. ibus-daemon -q
>> > > > >
>> > > > > I added the quit item in menu, and in the branch, I created a
>
> new
>>
>> command
>> > > line
>> > > > > tool `ibus`. It could be used for exit or restart ibus daemon.
>
> Please
>>
>> it.
>> > > > >
>> > > > >
>> > > > > >
>> > > > > > Probably I think it's good to release the current master
>
> (1.4.1?)
>>
>> before
>> > > the
>> > > > > new
>> > > > > > ibus integration (1.5.0?).
>> > > > >
>> > > > > Sure.
>> > > > >
>> > > > > >
>> > > > > > There is a still discussion if the new Control + space is
>
> useful.
>>
>> > > > > > Some Java applications (e.g. eclipse) also bind Control +
>
> Space.
>>
>> > > > > > Now the new ibus binds Control + Space even if no input
>
> methods but
>>
>> xkb
>> > > > > engines
>> > > > > > only. It would be a duplicated key bindings between ibus and
>
> others.
>>
>> > > > > > My understanding is that the solution is to move ibus
>
> settings to
>>
>> > > > > > gnome-control-center settings. I'm not sure about other
>
> desktops.
>>
>> > > > >
>> > > > > So probably, we could add a way to disable the ime keybinding.
>
> So if
>>
>> users
>> > > > don't
>> > > > > like it, they could disable the ime keybinding, and then use
>
> the menu
>>
>> for
>> > > > > switching IMEs. what do you think?
>> > > >
>> > > > Maybe I think it's a good idea but you indicates another issue.
>> > > > You're talking about which Control+Space and old Control+Space
>
> people like
>>
>> > to
>> > > > use. However I'm talking about which application owns the
>
> keybinding of
>>
>> > > > Control+Space by default. I said I have used Control+Space for
>
> input
>>
>> methods
>> > > for
>> > > > more than ten years but Java people also said Control+Space for
>
> Java.
>>
>> > > > XKB keyboard option doesn't have the keybinding of
>
> Control+Space. MacOS is
>>
>> > > > Command + Space.
>> > > > However probably I'd like to use Control+Space for ibus by
>
> default. I'm
>>
>> not
>> > > sure
>> > > > about other desktops except for GNOME.
>> > >
>> > > I guess most IME users use C+Space for IMEs, even if they are
>
> using Java. So
>>
>> > it
>> > > is not a problem for them.
>> > >
>> > > For users only use XKB, they can disable C+Space for IME. Only use
>
> the menu
>>
>> > for
>> > > switching xkb.
>> > >
>> > > So I think a configurable option should resolve this issue.
>> >
>> > OK, I will discuss this issue on bugzilla.
>> >
>> > lgtm with comments.
>
>
>> I think the two fixes are needed for src/ibusenginesimple.c:
>
>
>
https://github.com/ibus/ibus-xkb/commit/5a122df7d15842cfaafb349d7859a7b76d41f928
>
>
https://github.com/ibus/ibus-xkb/commit/64a8da3cd13783eb2c6c9dd952e02be63af4a832
>
> It seems 'make distcheck' has several errors.
> I integrated the devel directory & new ibus-gjs in Fedora rawhide today:
> http://fujiwara.fedorapeople.org/ibus/gnome-shell/ibus-1.4.99.20120203.tar.gz
>
> The following patch is needed for the latest vala & gtk3:
> http://fujiwara.fedorapeople.org/ibus/gnome-shell/ibus-HEAD.patch

This looks like a gdk or gir issue. Could you do more investigation on
it? What's the change in the latest gtk?


>
> http://codereview.appspot.com/5517048/

[fujiwara, 2012-02-08 03:38:05]

On 2012/02/07 19:50:14, shawn.p.huang_gmail.com wrote:
> On Fri, Feb 3, 2012 at 8:58 PM,  <mailto:takao.fujiwara1@gmail.com> wrote:
> > On 2012/02/02 02:05:31, fujiwara wrote:
> >>
> >> On 2012/02/01 09:10:25, fujiwara wrote:
> >> > On 2012/01/31 15:47:58, Peng wrote:
> >> > > On 2012/01/31 01:27:56, fujiwara wrote:
> >> > > > On 2012/01/27 15:32:28, Peng wrote:
> >> > > > > On 2012/01/27 06:00:53, fujiwara wrote:
> >> > > > > > Probably I think Quit button is useful for users.
> >> > > > > > ibus-gjs removes that button but it's due to the ibus & xkb
> >> integration.
> >> > > > > > The ibus gtk button handles ibus UI only so it would be nice
> >
> > to
> >>
> >> provide
> >> > > the
> >> > > > > way
> >> > > > > > to quit the application.
> >> > > > > > Also to exit ibus-daemon with ibus-gjs, I'm thinking it
> >
> > might be good
> >>
> >> to
> >> > > > > provide
> >> > > > > > an option in ibus-daemon to quit itself.
> >> > > > > > e.g. ibus-daemon -q
> >> > > > >
> >> > > > > I added the quit item in menu, and in the branch, I created a
> >
> > new
> >>
> >> command
> >> > > line
> >> > > > > tool `ibus`. It could be used for exit or restart ibus daemon.
> >
> > Please
> >>
> >> it.
> >> > > > >
> >> > > > >
> >> > > > > >
> >> > > > > > Probably I think it's good to release the current master
> >
> > (1.4.1?)
> >>
> >> before
> >> > > the
> >> > > > > new
> >> > > > > > ibus integration (1.5.0?).
> >> > > > >
> >> > > > > Sure.
> >> > > > >
> >> > > > > >
> >> > > > > > There is a still discussion if the new Control + space is
> >
> > useful.
> >>
> >> > > > > > Some Java applications (e.g. eclipse) also bind Control +
> >
> > Space.
> >>
> >> > > > > > Now the new ibus binds Control + Space even if no input
> >
> > methods but
> >>
> >> xkb
> >> > > > > engines
> >> > > > > > only. It would be a duplicated key bindings between ibus and
> >
> > others.
> >>
> >> > > > > > My understanding is that the solution is to move ibus
> >
> > settings to
> >>
> >> > > > > > gnome-control-center settings. I'm not sure about other
> >
> > desktops.
> >>
> >> > > > >
> >> > > > > So probably, we could add a way to disable the ime keybinding.
> >
> > So if
> >>
> >> users
> >> > > > don't
> >> > > > > like it, they could disable the ime keybinding, and then use
> >
> > the menu
> >>
> >> for
> >> > > > > switching IMEs. what do you think?
> >> > > >
> >> > > > Maybe I think it's a good idea but you indicates another issue.
> >> > > > You're talking about which Control+Space and old Control+Space
> >
> > people like
> >>
> >> > to
> >> > > > use. However I'm talking about which application owns the
> >
> > keybinding of
> >>
> >> > > > Control+Space by default. I said I have used Control+Space for
> >
> > input
> >>
> >> methods
> >> > > for
> >> > > > more than ten years but Java people also said Control+Space for
> >
> > Java.
> >>
> >> > > > XKB keyboard option doesn't have the keybinding of
> >
> > Control+Space. MacOS is
> >>
> >> > > > Command + Space.
> >> > > > However probably I'd like to use Control+Space for ibus by
> >
> > default. I'm
> >>
> >> not
> >> > > sure
> >> > > > about other desktops except for GNOME.
> >> > >
> >> > > I guess most IME users use C+Space for IMEs, even if they are
> >
> > using Java. So
> >>
> >> > it
> >> > > is not a problem for them.
> >> > >
> >> > > For users only use XKB, they can disable C+Space for IME. Only use
> >
> > the menu
> >>
> >> > for
> >> > > switching xkb.
> >> > >
> >> > > So I think a configurable option should resolve this issue.
> >> >
> >> > OK, I will discuss this issue on bugzilla.
> >> >
> >> > lgtm with comments.
> >
> >
> >> I think the two fixes are needed for src/ibusenginesimple.c:
> >
> >
> >
>
https://github.com/ibus/ibus-xkb/commit/5a122df7d15842cfaafb349d7859a7b76d41f928
> >
> >
>
https://github.com/ibus/ibus-xkb/commit/64a8da3cd13783eb2c6c9dd952e02be63af4a832
> >
> > It seems 'make distcheck' has several errors.
> > I integrated the devel directory & new ibus-gjs in Fedora rawhide today:
> >
http://fujiwara.fedorapeople.org/ibus/gnome-shell/ibus-1.4.99.20120203.tar.gz
> >
> > The following patch is needed for the latest vala & gtk3:
> > http://fujiwara.fedorapeople.org/ibus/gnome-shell/ibus-HEAD.patch
> 
> This looks like a gdk or gir issue. Could you do more investigation on
> it? What's the change in the latest gtk?

This problem is happened in vala-0.15.1 or later.
OK, I updated the patch with another approach:
http://fujiwara.fedorapeople.org/ibus/gnome-shell/ibus-HEAD.patch2

Probably I think we need to specify the structure members in the inherited
class.

> 
> 
> >
> > http://codereview.appspot.com/5517048/

[fujiwara, 2012-02-13 06:10:26]

http://codereview.appspot.com/5517048/diff/39001/configure.ac
File configure.ac (right):

http://codereview.appspot.com/5517048/diff/39001/configure.ac#newcode86
configure.ac:86: LT_VERSION_INFO="lt_current:lt_revision:lt_age"
People asked to change the major number in SONAME.

[Peng, 2012-02-21 19:42:17]

On 2012/02/13 06:10:26, fujiwara wrote:
> http://codereview.appspot.com/5517048/diff/39001/configure.ac
> File configure.ac (right):
> 
> http://codereview.appspot.com/5517048/diff/39001/configure.ac#newcode86
> configure.ac:86: LT_VERSION_INFO="lt_current:lt_revision:lt_age"
> People asked to change the major number in SONAME.

If the branch does not have design or critical issues, I suggest merging devel
branch to upstream first, and then we can begin fixing minor issues one by one.
Right now, I have to keep the devel branch sync with upstream HEAD branch. And
the devel branch becomes bigger and bigger, maintaining it becomes harder
continually. :(

[fujiwara, 2012-02-22 04:17:25]

On 2012/02/21 19:42:17, Peng wrote:
> On 2012/02/13 06:10:26, fujiwara wrote:
> > http://codereview.appspot.com/5517048/diff/39001/configure.ac
> > File configure.ac (right):
> > 
> > http://codereview.appspot.com/5517048/diff/39001/configure.ac#newcode86
> > configure.ac:86: LT_VERSION_INFO="lt_current:lt_revision:lt_age"
> > People asked to change the major number in SONAME.
> 
> If the branch does not have design or critical issues, I suggest merging devel
> branch to upstream first, and then we can begin fixing minor issues one by
one.
> Right now, I have to keep the devel branch sync with upstream HEAD branch. And
> the devel branch becomes bigger and bigger, maintaining it becomes harder
> continually. :(

lgtm

BTW, I'd ask ibus-1.4.2 later, maybe branch.

[Peng, 2012-02-22 04:18:30]

On 2012/02/22 04:17:25, fujiwara wrote:
> On 2012/02/21 19:42:17, Peng wrote:
> > On 2012/02/13 06:10:26, fujiwara wrote:
> > > http://codereview.appspot.com/5517048/diff/39001/configure.ac
> > > File configure.ac (right):
> > > 
> > > http://codereview.appspot.com/5517048/diff/39001/configure.ac#newcode86
> > > configure.ac:86: LT_VERSION_INFO="lt_current:lt_revision:lt_age"
> > > People asked to change the major number in SONAME.
> > 
> > If the branch does not have design or critical issues, I suggest merging
devel
> > branch to upstream first, and then we can begin fixing minor issues one by
> one.
> > Right now, I have to keep the devel branch sync with upstream HEAD branch.
And
> > the devel branch becomes bigger and bigger, maintaining it becomes harder
> > continually. :(
> 
> lgtm
> 
> BTW, I'd ask ibus-1.4.2 later, maybe branch.

Hi Yusuke, Could please review this CL again? Thanks.

[Yusuke Sato, 2012-02-25 15:55:43]

LGTM.

From the Chromium OS POV, the new version should not affect the OS that much. If
I understand correctly, what I have to do is to stop sending the preload_engines
config to the daemon, and that's it. At the same time, it's very nice to see the
new ibus-daemon is greatly simplified: no hotkey handling in the daemon,
simplified engine activation with out the "preload" concept. Thanks for the
work.

http://codereview.appspot.com/5517048/diff/60001/src/ibusbus.c
File src/ibusbus.c (right):

http://codereview.appspot.com/5517048/diff/60001/src/ibusbus.c#newcode1385
src/ibusbus.c:1385: g_object_ref_sink (serializable);
Should we backport this fix to 1.4? If so, please do that (and patch Chromium OS
version of IBus as well :)

http://codereview.appspot.com/5517048/diff/60001/src/ibusbus.c#newcode1441
src/ibusbus.c:1441: g_object_ref_sink (serializable);
ditto

http://codereview.appspot.com/5517048/diff/60001/src/ibusbus.h
File src/ibusbus.h (right):

http://codereview.appspot.com/5517048/diff/60001/src/ibusbus.h#newcode719
src/ibusbus.h:719: * Get engines by given names synchronously.
what happens if one or some of the names are invalid? looks like such names are
just ignored, correct?  please document the behavior.

http://codereview.appspot.com/5517048/diff/60001/src/ibusproplist.c
File src/ibusproplist.c (right):

http://codereview.appspot.com/5517048/diff/60001/src/ibusproplist.c#newcode111
src/ibusproplist.c:111: g_object_ref_sink (prop);
the same. backport?

http://codereview.appspot.com/5517048/diff/60001/src/tests/ibus-bus.c
File src/tests/ibus-bus.c (right):

http://codereview.appspot.com/5517048/diff/60001/src/tests/ibus-bus.c#newcode537
src/tests/ibus-bus.c:537: const gchar *names[] = {
what happens if one (or some) of the names are unknown? please test the case as
well.

[Peng, 2012-02-26 21:52:14]

https://codereview.appspot.com/5517048/diff/60001/src/ibusbus.c
File src/ibusbus.c (right):

https://codereview.appspot.com/5517048/diff/60001/src/ibusbus.c#newcode1385
src/ibusbus.c:1385: g_object_ref_sink (serializable);
On 2012/02/25 15:55:43, Yusuke Sato wrote:
> Should we backport this fix to 1.4? If so, please do that (and patch Chromium
OS
> version of IBus as well :)
I think we should back port it.

https://codereview.appspot.com/5517048/diff/60001/src/ibusbus.c#newcode1441
src/ibusbus.c:1441: g_object_ref_sink (serializable);
On 2012/02/25 15:55:43, Yusuke Sato wrote:
> ditto

same

https://codereview.appspot.com/5517048/diff/60001/src/ibusbus.h
File src/ibusbus.h (right):

https://codereview.appspot.com/5517048/diff/60001/src/ibusbus.h#newcode719
src/ibusbus.h:719: * Get engines by given names synchronously.
On 2012/02/25 15:55:43, Yusuke Sato wrote:
> what happens if one or some of the names are invalid? looks like such names
are
> just ignored, correct?  please document the behavior.

I will fixed it in another CL.

https://codereview.appspot.com/5517048/diff/60001/src/ibusproplist.c
File src/ibusproplist.c (right):

https://codereview.appspot.com/5517048/diff/60001/src/ibusproplist.c#newcode111
src/ibusproplist.c:111: g_object_ref_sink (prop);
On 2012/02/25 15:55:43, Yusuke Sato wrote:
> the same. backport?

Yes

https://codereview.appspot.com/5517048/diff/60001/src/tests/ibus-bus.c
File src/tests/ibus-bus.c (right):

https://codereview.appspot.com/5517048/diff/60001/src/tests/ibus-bus.c#newcod...
src/tests/ibus-bus.c:537: const gchar *names[] = {
On 2012/02/25 15:55:43, Yusuke Sato wrote:
> what happens if one (or some) of the names are unknown? please test the case
as
> well.
I will fix this in another CL.

[Peng, 2012-02-27 15:28:06]

On 2012/02/08 03:38:05, fujiwara wrote:
> On 2012/02/07 19:50:14, http://shawn.p.huang_gmail.com wrote:
> > On Fri, Feb 3, 2012 at 8:58 PM,  <mailto:takao.fujiwara1@gmail.com> wrote:
> > > On 2012/02/02 02:05:31, fujiwara wrote:
> > >>
> > >> On 2012/02/01 09:10:25, fujiwara wrote:
> > >> > On 2012/01/31 15:47:58, Peng wrote:
> > >> > > On 2012/01/31 01:27:56, fujiwara wrote:
> > >> > > > On 2012/01/27 15:32:28, Peng wrote:
> > >> > > > > On 2012/01/27 06:00:53, fujiwara wrote:
> > >> > > > > > Probably I think Quit button is useful for users.
> > >> > > > > > ibus-gjs removes that button but it's due to the ibus & xkb
> > >> integration.
> > >> > > > > > The ibus gtk button handles ibus UI only so it would be nice
> > >
> > > to
> > >>
> > >> provide
> > >> > > the
> > >> > > > > way
> > >> > > > > > to quit the application.
> > >> > > > > > Also to exit ibus-daemon with ibus-gjs, I'm thinking it
> > >
> > > might be good
> > >>
> > >> to
> > >> > > > > provide
> > >> > > > > > an option in ibus-daemon to quit itself.
> > >> > > > > > e.g. ibus-daemon -q
> > >> > > > >
> > >> > > > > I added the quit item in menu, and in the branch, I created a
> > >
> > > new
> > >>
> > >> command
> > >> > > line
> > >> > > > > tool `ibus`. It could be used for exit or restart ibus daemon.
> > >
> > > Please
> > >>
> > >> it.
> > >> > > > >
> > >> > > > >
> > >> > > > > >
> > >> > > > > > Probably I think it's good to release the current master
> > >
> > > (1.4.1?)
> > >>
> > >> before
> > >> > > the
> > >> > > > > new
> > >> > > > > > ibus integration (1.5.0?).
> > >> > > > >
> > >> > > > > Sure.
> > >> > > > >
> > >> > > > > >
> > >> > > > > > There is a still discussion if the new Control + space is
> > >
> > > useful.
> > >>
> > >> > > > > > Some Java applications (e.g. eclipse) also bind Control +
> > >
> > > Space.
> > >>
> > >> > > > > > Now the new ibus binds Control + Space even if no input
> > >
> > > methods but
> > >>
> > >> xkb
> > >> > > > > engines
> > >> > > > > > only. It would be a duplicated key bindings between ibus and
> > >
> > > others.
> > >>
> > >> > > > > > My understanding is that the solution is to move ibus
> > >
> > > settings to
> > >>
> > >> > > > > > gnome-control-center settings. I'm not sure about other
> > >
> > > desktops.
> > >>
> > >> > > > >
> > >> > > > > So probably, we could add a way to disable the ime keybinding.
> > >
> > > So if
> > >>
> > >> users
> > >> > > > don't
> > >> > > > > like it, they could disable the ime keybinding, and then use
> > >
> > > the menu
> > >>
> > >> for
> > >> > > > > switching IMEs. what do you think?
> > >> > > >
> > >> > > > Maybe I think it's a good idea but you indicates another issue.
> > >> > > > You're talking about which Control+Space and old Control+Space
> > >
> > > people like
> > >>
> > >> > to
> > >> > > > use. However I'm talking about which application owns the
> > >
> > > keybinding of
> > >>
> > >> > > > Control+Space by default. I said I have used Control+Space for
> > >
> > > input
> > >>
> > >> methods
> > >> > > for
> > >> > > > more than ten years but Java people also said Control+Space for
> > >
> > > Java.
> > >>
> > >> > > > XKB keyboard option doesn't have the keybinding of
> > >
> > > Control+Space. MacOS is
> > >>
> > >> > > > Command + Space.
> > >> > > > However probably I'd like to use Control+Space for ibus by
> > >
> > > default. I'm
> > >>
> > >> not
> > >> > > sure
> > >> > > > about other desktops except for GNOME.
> > >> > >
> > >> > > I guess most IME users use C+Space for IMEs, even if they are
> > >
> > > using Java. So
> > >>
> > >> > it
> > >> > > is not a problem for them.
> > >> > >
> > >> > > For users only use XKB, they can disable C+Space for IME. Only use
> > >
> > > the menu
> > >>
> > >> > for
> > >> > > switching xkb.
> > >> > >
> > >> > > So I think a configurable option should resolve this issue.
> > >> >
> > >> > OK, I will discuss this issue on bugzilla.
> > >> >
> > >> > lgtm with comments.
> > >
> > >
> > >> I think the two fixes are needed for src/ibusenginesimple.c:
> > >
> > >
> > >
> >
>
https://github.com/ibus/ibus-xkb/commit/5a122df7d15842cfaafb349d7859a7b76d41f928
> > >
> > >
> >
>
https://github.com/ibus/ibus-xkb/commit/64a8da3cd13783eb2c6c9dd952e02be63af4a832
> > >
> > > It seems 'make distcheck' has several errors.
> > > I integrated the devel directory & new ibus-gjs in Fedora rawhide today:
> > >
> http://fujiwara.fedorapeople.org/ibus/gnome-shell/ibus-1.4.99.20120203.tar.gz
> > >
> > > The following patch is needed for the latest vala & gtk3:
> > > http://fujiwara.fedorapeople.org/ibus/gnome-shell/ibus-HEAD.patch
> > 
> > This looks like a gdk or gir issue. Could you do more investigation on
> > it? What's the change in the latest gtk?
> 
> This problem is happened in vala-0.15.1 or later.
> OK, I updated the patch with another approach:
> http://fujiwara.fedorapeople.org/ibus/gnome-shell/ibus-HEAD.patch2
> 

Could you please create a CL for it? Thanks.

> Probably I think we need to specify the structure members in the inherited
> class.
> 
> > 
> > 
> > >
> > > http://codereview.appspot.com/5517048/

[Peng, 2012-02-27 15:51:09]

http://codereview.appspot.com/5517048/diff/39001/configure.ac
File configure.ac (right):

http://codereview.appspot.com/5517048/diff/39001/configure.ac#newcode86
configure.ac:86: LT_VERSION_INFO="lt_current:lt_revision:lt_age"
On 2012/02/13 06:10:27, fujiwara wrote:
> People asked to change the major number in SONAME.
Do you know how to change it? I don't know understand libtool version system
very well.