Fix Alt+Shift hotkey issue for chrome os

Fix Alt+Shift hotkey issue for chrome os


BUG=chromium-os:6225
TEST=manual

[Yusuke Sato, 2010-12-25 13:38:48]

Hi, it was necessary to modify your patch a little to make Shift+Alt work on
Chromium OS.

I'll attach the modified version of the change to
http://code.google.com/p/chromium-os/issues/detail?id=6225 shortly.

http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c
File src/ibushotkey.c (right):

http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c#newcode404
src/ibushotkey.c:404: normalize_modifiers (keyval, modifiers));
This is not an issue in this CL, but shouldn't we use priv->mask here? I mean,

normalize_modifiers (keyval, modifiers & priv->mask));

http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c#newcode459
src/ibushotkey.c:459: .modifiers = normalize_modifiers (keyval, modifiers)
ditto.

normalize_modifiers (keyval, modifiers & priv->mask)

http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c#newcode545
src/ibushotkey.c:545: if (modifiers != (prev_modifiers | IBUS_RELEASE_MASK))
This line caused a problem. As I wrote at
http://code.google.com/p/chromium-os/issues/detail?id=6225#c18 , in the
alt+shift case, this condition will always be evaluated as false. I think you
meant to say


        /* modifiers should be same except the release bit */
        if (normalize_modifiers (keyval, modifiers & priv->mask) !=
            (normalize_modifiers (prev_keyval, prev_modifiers & priv->mask) |
IBUS_RELEASE_MASK))
            return 0;

http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c#newcode581
src/ibushotkey.c:581: .modifiers = modifiers & priv->mask,
I believe that this line should be changed as well:

.modifiers = normalize_modifiers (keyval, modifiers & priv->mask),

[Peng, 2010-12-25 18:08:32]

http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c
File src/ibushotkey.c (right):

http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c#newcode404
src/ibushotkey.c:404: normalize_modifiers (keyval, modifiers));
On 2010/12/25 13:38:48, Yusuke Sato wrote:
> This is not an issue in this CL, but shouldn't we use priv->mask here? I mean,
> 
> normalize_modifiers (keyval, modifiers & priv->mask));

Done.

http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c#newcode459
src/ibushotkey.c:459: .modifiers = normalize_modifiers (keyval, modifiers)
On 2010/12/25 13:38:48, Yusuke Sato wrote:
> ditto.
> 
> normalize_modifiers (keyval, modifiers & priv->mask)

Done.

http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c#newcode545
src/ibushotkey.c:545: if (modifiers != (prev_modifiers | IBUS_RELEASE_MASK))
On 2010/12/25 13:38:48, Yusuke Sato wrote:
> This line caused a problem. As I wrote at
> http://code.google.com/p/chromium-os/issues/detail?id=6225#c18 , in the
> alt+shift case, this condition will always be evaluated as false. I think you
> meant to say
> 
> 
>         /* modifiers should be same except the release bit */
>         if (normalize_modifiers (keyval, modifiers & priv->mask) !=
>             (normalize_modifiers (prev_keyval, prev_modifiers & priv->mask) |
> IBUS_RELEASE_MASK))
>             return 0;

Done.

http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c#newcode581
src/ibushotkey.c:581: .modifiers = modifiers & priv->mask,
On 2010/12/25 13:38:48, Yusuke Sato wrote:
> I believe that this line should be changed as well:
> 
> .modifiers = normalize_modifiers (keyval, modifiers & priv->mask),

Done.

[Yusuke Sato, 2010-12-25 23:58:46]

LGTM!

On 2010/12/25 18:08:32, Shawn.P.Huang wrote:
> http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c
> File src/ibushotkey.c (right):
> 
> http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c#newcode404
> src/ibushotkey.c:404: normalize_modifiers (keyval, modifiers));
> On 2010/12/25 13:38:48, Yusuke Sato wrote:
> > This is not an issue in this CL, but shouldn't we use priv->mask here? I
mean,
> > 
> > normalize_modifiers (keyval, modifiers & priv->mask));
> 
> Done.
> 
> http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c#newcode459
> src/ibushotkey.c:459: .modifiers = normalize_modifiers (keyval, modifiers)
> On 2010/12/25 13:38:48, Yusuke Sato wrote:
> > ditto.
> > 
> > normalize_modifiers (keyval, modifiers & priv->mask)
> 
> Done.
> 
> http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c#newcode545
> src/ibushotkey.c:545: if (modifiers != (prev_modifiers | IBUS_RELEASE_MASK))
> On 2010/12/25 13:38:48, Yusuke Sato wrote:
> > This line caused a problem. As I wrote at
> > http://code.google.com/p/chromium-os/issues/detail?id=6225#c18 , in the
> > alt+shift case, this condition will always be evaluated as false. I think
you
> > meant to say
> > 
> > 
> >         /* modifiers should be same except the release bit */
> >         if (normalize_modifiers (keyval, modifiers & priv->mask) !=
> >             (normalize_modifiers (prev_keyval, prev_modifiers & priv->mask)
|
> > IBUS_RELEASE_MASK))
> >             return 0;
> 
> Done.
> 
> http://codereview.appspot.com/3741043/diff/1/src/ibushotkey.c#newcode581
> src/ibushotkey.c:581: .modifiers = modifiers & priv->mask,
> On 2010/12/25 13:38:48, Yusuke Sato wrote:
> > I believe that this line should be changed as well:
> > 
> > .modifiers = normalize_modifiers (keyval, modifiers & priv->mask),
> 
> Done.