Refine IME switch keybinding related code.

Refine IME switch keybinding related code.


BUG=None
TEST=Manually

[fujiwara, 2012-06-07 03:41:44]

https://codereview.appspot.com/6295047/diff/1/ui/gtk3/panel.vala
File ui/gtk3/panel.vala (right):

https://codereview.appspot.com/6295047/diff/1/ui/gtk3/panel.vala#newcode84
ui/gtk3/panel.vala:84: private void bind_switcher_shortcut() {
unbind_switch_shortcut() and bind_switcher_shortcut()
       ^^^^^^                     ^^^^^^^^
have the different naming styles.

https://codereview.appspot.com/6295047/diff/1/ui/gtk3/panel.vala#newcode100
ui/gtk3/panel.vala:100: if ((m_switch_modifiers & Gdk.ModifierType.SHIFT_MASK)
!= 0)
I think if (m_switch_modifiers == 0 || m_switch_modifiers &
Gdk.ModifierType.SHIFT_MASK) != 0), it could return.
I think If m_switch_modifiers == 0, the switcher dialog is not needed.

https://codereview.appspot.com/6295047/diff/1/ui/gtk3/switcher.vala
File ui/gtk3/switcher.vala (right):

https://codereview.appspot.com/6295047/diff/1/ui/gtk3/switcher.vala#newcode322
ui/gtk3/switcher.vala:322: if (modifiers == m_modifiers)
I guess this means Ctrl+space is next_engine() and Alt+space is
previous_engine().

Probably I like:
                if (modifiers == m_modifiers) {
                    next_engine();
                } else if ((modifiers & Gdk.ModifierType.SHIFT_MASK) != 0) {
                    previous_engine();
                }

[Peng, 2012-06-07 12:38:06]

Update CL. Please review it again.

https://codereview.appspot.com/6295047/diff/1/ui/gtk3/panel.vala
File ui/gtk3/panel.vala (right):

https://codereview.appspot.com/6295047/diff/1/ui/gtk3/panel.vala#newcode84
ui/gtk3/panel.vala:84: private void bind_switcher_shortcut() {
On 2012/06/07 03:41:44, fujiwara wrote:
> unbind_switch_shortcut() and bind_switcher_shortcut()
>        ^^^^^^                     ^^^^^^^^
> have the different naming styles.

Done.

https://codereview.appspot.com/6295047/diff/1/ui/gtk3/panel.vala#newcode100
ui/gtk3/panel.vala:100: if ((m_switch_modifiers & Gdk.ModifierType.SHIFT_MASK)
!= 0)
On 2012/06/07 03:41:44, fujiwara wrote:
> I think if (m_switch_modifiers == 0 || m_switch_modifiers &
> Gdk.ModifierType.SHIFT_MASK) != 0), it could return.
> I think If m_switch_modifiers == 0, the switcher dialog is not needed.

Yes. In that case, the primary modifier is 0, the switcher dialog will not be
displayed. Please check 251 line in this file.

https://codereview.appspot.com/6295047/diff/1/ui/gtk3/switcher.vala
File ui/gtk3/switcher.vala (right):

https://codereview.appspot.com/6295047/diff/1/ui/gtk3/switcher.vala#newcode322
ui/gtk3/switcher.vala:322: if (modifiers == m_modifiers)
On 2012/06/07 03:41:44, fujiwara wrote:
> I guess this means Ctrl+space is next_engine() and Alt+space is
> previous_engine().
> 
> Probably I like:
>                 if (modifiers == m_modifiers) {
>                     next_engine();
>                 } else if ((modifiers & Gdk.ModifierType.SHIFT_MASK) != 0) {
>                     previous_engine();
>                 }
In line 216 and 317. It guarantees modifier has to equal to m_modifier or
m_modifier | Gdk.ModifierType.SHIFT_MASK.

So it is not necessary to check it again.

I can add some comment here to make it clear.

Done

[fujiwara, 2012-06-08 02:30:39]

On 2012/06/07 12:38:06, Peng wrote:
> Update CL. Please review it again.
> 
> https://codereview.appspot.com/6295047/diff/1/ui/gtk3/panel.vala
> File ui/gtk3/panel.vala (right):
> 
> https://codereview.appspot.com/6295047/diff/1/ui/gtk3/panel.vala#newcode84
> ui/gtk3/panel.vala:84: private void bind_switcher_shortcut() {
> On 2012/06/07 03:41:44, fujiwara wrote:
> > unbind_switch_shortcut() and bind_switcher_shortcut()
> >        ^^^^^^                     ^^^^^^^^
> > have the different naming styles.
> 
> Done.
> 
> https://codereview.appspot.com/6295047/diff/1/ui/gtk3/panel.vala#newcode100
> ui/gtk3/panel.vala:100: if ((m_switch_modifiers & Gdk.ModifierType.SHIFT_MASK)
> != 0)
> On 2012/06/07 03:41:44, fujiwara wrote:
> > I think if (m_switch_modifiers == 0 || m_switch_modifiers &
> > Gdk.ModifierType.SHIFT_MASK) != 0), it could return.
> > I think If m_switch_modifiers == 0, the switcher dialog is not needed.
> 
> Yes. In that case, the primary modifier is 0, the switcher dialog will not be
> displayed. Please check 251 line in this file.
> 
> https://codereview.appspot.com/6295047/diff/1/ui/gtk3/switcher.vala
> File ui/gtk3/switcher.vala (right):
> 
> https://codereview.appspot.com/6295047/diff/1/ui/gtk3/switcher.vala#newcode322
> ui/gtk3/switcher.vala:322: if (modifiers == m_modifiers)
> On 2012/06/07 03:41:44, fujiwara wrote:
> > I guess this means Ctrl+space is next_engine() and Alt+space is
> > previous_engine().
> > 
> > Probably I like:
> >                 if (modifiers == m_modifiers) {
> >                     next_engine();
> >                 } else if ((modifiers & Gdk.ModifierType.SHIFT_MASK) != 0) {
> >                     previous_engine();
> >                 }
> In line 216 and 317. It guarantees modifier has to equal to m_modifier or
> m_modifier | Gdk.ModifierType.SHIFT_MASK.
> 
> So it is not necessary to check it again.
> 
> I can add some comment here to make it clear.
> 
> Done

OK, I see.
lgtm.