Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
664893004:
If keysym is ASCII control or space keys, directly map to keycode. (Closed)
DescriptionIf keysym is ASCII control or space keys, directly map to keycode instead of go through the fallback maps.
A real case is de(neo) layout, AltGr+V maps to Enter key, so it should map to VKEY_ENTER instead of VKEY_V.
BUG=420544
TEST=Verified on linux_chromeos.
Committed: https://crrev.com/3f1b319d5fb0fe247046d858af70e12878524f0e
Cr-Commit-Position: refs/heads/master@{#302047}
Patch Set 1 #Patch Set 2 : #
Total comments: 2
Patch Set 3 : revised per comments. #
Total comments: 2
Patch Set 4 : modified per comments. #
Total comments: 6
Patch Set 5 : add the missing XK_Escape #Patch Set 6 : nits #
Total comments: 4
Patch Set 7 : make a list for TTY function and space keys #Messages
Total messages: 27 (4 generated)
shuchen@chromium.org changed reviewers: + wez@chromium.org, yukishiino@chromium.org
Wez/Yuki, can you please review this cl? Thanks
Thanks for the quick fix. https://codereview.chromium.org/664893004/diff/20001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/664893004/diff/20001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:496: keysym == XK_BackSpace || keysym == XK_space) { Do we want to support other control characters here? For example, vertical tab, form feed, escape, etc. We may want to treat all control characters in ASCII (0x01 to 0x1F) + space (0x20) here. You may want to add such a helper function, say IsAsciiControlKey(keysym). Though, note that space is not a control character.
https://codereview.chromium.org/664893004/diff/20001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/664893004/diff/20001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:496: keysym == XK_BackSpace || keysym == XK_space) { On 2014/10/23 06:33:44, Yuki wrote: > Do we want to support other control characters here? > For example, vertical tab, form feed, escape, etc. > > We may want to treat all control characters in ASCII (0x01 to 0x1F) + space > (0x20) here. You may want to add such a helper function, say > IsAsciiControlKey(keysym). Though, note that space is not a control character. Done. Thanks for the suggestion.
lgtm. https://codereview.chromium.org/664893004/diff/40001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/664893004/diff/40001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:463: if ((keysym > 0xff00 && keysym < 0xff1f) || keysym == XK_Delete) keysym <= 0xff1f ? I think we'd like to include 0xff1f, too. We may want to comment that 0xff01 to 0xff1f represent ASCII control in keysym world (it's not 0x01 to 0x1f). It's not so trivial. For example, XK_space is 0x20. Not everyone knows Xkeysym details. nit: return (0xff01 <= keysym && keysym <= 0xff1f) || keysym == XK_Delete; seems more natural to me, but it's up to you.
https://codereview.chromium.org/664893004/diff/40001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/664893004/diff/40001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:463: if ((keysym > 0xff00 && keysym < 0xff1f) || keysym == XK_Delete) On 2014/10/23 07:13:56, Yuki wrote: > keysym <= 0xff1f ? > I think we'd like to include 0xff1f, too. > > We may want to comment that 0xff01 to 0xff1f represent ASCII control in keysym > world (it's not 0x01 to 0x1f). It's not so trivial. For example, XK_space is > 0x20. Not everyone knows Xkeysym details. > > nit: > return (0xff01 <= keysym && keysym <= 0xff1f) || keysym == XK_Delete; > seems more natural to me, but it's up to you. Done.
https://codereview.chromium.org/664893004/diff/60001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/664893004/diff/60001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:465: return (keysym > 0xff00 && keysym <= 0xff1f) || keysym == XK_Delete; Why is XK_Delete in here? It's not a Ctrl+<ascii> keysym. https://codereview.chromium.org/664893004/diff/60001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:502: keysym == XK_space) { could/should space go in IsAsciiControlKey?
https://codereview.chromium.org/664893004/diff/60001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/664893004/diff/60001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:465: return (keysym > 0xff00 && keysym <= 0xff1f) || keysym == XK_Delete; On 2014/10/24 01:17:44, Wez wrote: > Why is XK_Delete in here? It's not a Ctrl+<ascii> keysym. Please refer to: http://en.wikipedia.org/wiki/Control_character#In_ASCII https://codereview.chromium.org/664893004/diff/60001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:502: keysym == XK_space) { On 2014/10/24 01:17:44, Wez wrote: > could/should space go in IsAsciiControlKey? Ditto.
https://codereview.chromium.org/664893004/diff/60001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/664893004/diff/60001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:465: return (keysym > 0xff00 && keysym <= 0xff1f) || keysym == XK_Delete; On 2014/10/24 01:26:58, Shu Chen wrote: > On 2014/10/24 01:17:44, Wez wrote: > > Why is XK_Delete in here? It's not a Ctrl+<ascii> keysym. > > Please refer to: > http://en.wikipedia.org/wiki/Control_character#In_ASCII OK, understood; please make sure the CL description explains clearly _why_ we need to do this. https://codereview.chromium.org/664893004/diff/60001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:502: keysym == XK_space) { On 2014/10/24 01:26:58, Shu Chen wrote: > On 2014/10/24 01:17:44, Wez wrote: > > could/should space go in IsAsciiControlKey? > > Ditto. ? As currently laid out it's confusing that space is separate; is the reason we want it in this list the same basic reason as for delete, escape, etc? If so then why not put it in IsAsciiControlKey with a comment to explain why?
On 2014/10/24 01:43:14, Wez wrote: > https://codereview.chromium.org/664893004/diff/60001/ui/events/keycodes/keybo... > File ui/events/keycodes/keyboard_code_conversion_x.cc (right): > > https://codereview.chromium.org/664893004/diff/60001/ui/events/keycodes/keybo... > ui/events/keycodes/keyboard_code_conversion_x.cc:465: return (keysym > 0xff00 && > keysym <= 0xff1f) || keysym == XK_Delete; > On 2014/10/24 01:26:58, Shu Chen wrote: > > On 2014/10/24 01:17:44, Wez wrote: > > > Why is XK_Delete in here? It's not a Ctrl+<ascii> keysym. > > > > Please refer to: > > http://en.wikipedia.org/wiki/Control_character#In_ASCII > > OK, understood; please make sure the CL description explains clearly _why_ we > need to do this. Done. > > https://codereview.chromium.org/664893004/diff/60001/ui/events/keycodes/keybo... > ui/events/keycodes/keyboard_code_conversion_x.cc:502: keysym == XK_space) { > On 2014/10/24 01:26:58, Shu Chen wrote: > > On 2014/10/24 01:17:44, Wez wrote: > > > could/should space go in IsAsciiControlKey? > > > > Ditto. > > ? > > As currently laid out it's confusing that space is separate; is the reason we > want it in this list the same basic reason as for delete, escape, etc? If so > then why not put it in IsAsciiControlKey with a comment to explain why? Done. Thanks for your review.
https://codereview.chromium.org/664893004/diff/100001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/664893004/diff/100001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_code_conversion_x.cc:463: // instead of stripping the modifier states and go through the fallback maps. _Why_ should they do this, though? What would Ctrl+Space result in without this fix? What are the VKEYs we intend to generate instead as a result of this? KeyboardCodeFromXKeysym doesn't seem to have mappings for 0xff01-0xff1f, so won't we generate VK_UNKNOWN...?
https://codereview.chromium.org/664893004/diff/100001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/664893004/diff/100001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_code_conversion_x.cc:463: // instead of stripping the modifier states and go through the fallback maps. On 2014/10/24 22:29:18, Wez wrote: > _Why_ should they do this, though? > > What would Ctrl+Space result in without this fix? > > What are the VKEYs we intend to generate instead as a result of this? > KeyboardCodeFromXKeysym doesn't seem to have mappings for 0xff01-0xff1f, so > won't we generate VK_UNKNOWN...? The example you give in CL description is AltGr+V -> Enter - but Enter isn't a control nor space key, so won't match this case, nor is 'V'..?
https://codereview.chromium.org/664893004/diff/100001/ui/events/keycodes/keyb... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/664893004/diff/100001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_code_conversion_x.cc:463: // instead of stripping the modifier states and go through the fallback maps. On 2014/10/24 22:29:18, Wez wrote: > _Why_ should they do this, though? > > What would Ctrl+Space result in without this fix? > > What are the VKEYs we intend to generate instead as a result of this? > KeyboardCodeFromXKeysym doesn't seem to have mappings for 0xff01-0xff1f, so > won't we generate VK_UNKNOWN...? The range 0xff01-0xff1f contains XK_Tab, XK_Return, XK_Backspace, etc. so KeyboardCodeFromXKeysym can give out correct VKEYs. https://codereview.chromium.org/664893004/diff/100001/ui/events/keycodes/keyb... ui/events/keycodes/keyboard_code_conversion_x.cc:463: // instead of stripping the modifier states and go through the fallback maps. On 2014/10/24 22:31:22, Wez wrote: > On 2014/10/24 22:29:18, Wez wrote: > > _Why_ should they do this, though? > > > > What would Ctrl+Space result in without this fix? > > > > What are the VKEYs we intend to generate instead as a result of this? > > KeyboardCodeFromXKeysym doesn't seem to have mappings for 0xff01-0xff1f, so > > won't we generate VK_UNKNOWN...? > > The example you give in CL description is AltGr+V -> Enter - but Enter isn't a > control nor space key, so won't match this case, nor is 'V'..? Enter is an ascii control key: keysym=0xff0d (XK_Return).
Of course; thanks for the reminders! I think it'd make sense to use the start & end "control key" names in both the code & comment, e.g: "BackSpace (0xff08) to Escape (0xff1b) and Delete map to ASCII's control characters. They and Space should always be mapped based on KeySym, and never fall back to mapping by h/w keycode, since some layouts generate them by e.g. applying the Control modifier to some other key." Does that make sense? It may even be clearer to use an explicit switch listing the exact KeySyms we're exempting. Off CL, let's sync up on the overall approach being taken here; it feels like we should only ever be falling back to h/w mapping if we get an un-mappable Unicode KeySym for a key; then we wouldn't need this hack to exempt particular keys from fall-back.
On 2014/10/27 19:27:12, Wez wrote: > Of course; thanks for the reminders! > > I think it'd make sense to use the start & end "control key" names in both the > code & comment, e.g: > "BackSpace (0xff08) to Escape (0xff1b) and Delete map to ASCII's control > characters. They and Space should always be mapped based on KeySym, and never > fall back to mapping by h/w keycode, since some layouts generate them by e.g. > applying the Control modifier to some other key." > > Does that make sense? It may even be clearer to use an explicit switch listing > the exact KeySyms we're exempting. Done. I've now made it as a list. > > Off CL, let's sync up on the overall approach being taken here; it feels like we > should only ever be falling back to h/w mapping if we get an un-mappable Unicode > KeySym for a key; then we wouldn't need this hack to exempt particular keys from > fall-back. I think it is easier to make a blacklist than a whitelist for entering MAP0~MAP3. Do you have any ideas of how to create a whitelist of "un-mappable Unicode"?
On 2014/10/28 02:33:14, Shu Chen wrote: > On 2014/10/27 19:27:12, Wez wrote: > > Of course; thanks for the reminders! > > > > I think it'd make sense to use the start & end "control key" names in both the > > code & comment, e.g: > > "BackSpace (0xff08) to Escape (0xff1b) and Delete map to ASCII's control > > characters. They and Space should always be mapped based on KeySym, and never > > fall back to mapping by h/w keycode, since some layouts generate them by e.g. > > applying the Control modifier to some other key." > > > > Does that make sense? It may even be clearer to use an explicit switch listing > > the exact KeySyms we're exempting. > Done. I've now made it as a list. > > > > Off CL, let's sync up on the overall approach being taken here; it feels like > we > > should only ever be falling back to h/w mapping if we get an un-mappable > Unicode > > KeySym for a key; then we wouldn't need this hack to exempt particular keys > from > > fall-back. > I think it is easier to make a blacklist than a whitelist for entering > MAP0~MAP3. > Do you have any ideas of how to create a whitelist of "un-mappable Unicode"? With a second thought, I have a maybe-better idea: Current problem is that before entering map0-map3, it tries to stripe the modifier states and check for [a-z] keys. If we check [a-z] keys without striping the modifier states, we wouldn't have the bugs like AltGr+V->XKEnter. But that means the original behaviors would be changed. For example, for the case AltGr+A->XK_B, original behavior gets VKEY_A, but this solution would get VKEY_B. That should be ok since it is a rare case and AFAIK no XKB layout actually has that kind of mapping. Another downside for the idea is about performance. For the case of AltGr+V->XKEnter, it will go through map0-map3 and failed to find the mapped VKEY and then fallback to KeyboardCodeFromXKeysym() at line 584. But considering those are not common cases, I think this performance hit can be ignored. WDYT?
On 2014/10/28 02:44:39, Shu Chen wrote: > On 2014/10/28 02:33:14, Shu Chen wrote: > > On 2014/10/27 19:27:12, Wez wrote: > > > Of course; thanks for the reminders! > > > > > > I think it'd make sense to use the start & end "control key" names in both > the > > > code & comment, e.g: > > > "BackSpace (0xff08) to Escape (0xff1b) and Delete map to ASCII's control > > > characters. They and Space should always be mapped based on KeySym, and > never > > > fall back to mapping by h/w keycode, since some layouts generate them by > e.g. > > > applying the Control modifier to some other key." > > > > > > Does that make sense? It may even be clearer to use an explicit switch > listing > > > the exact KeySyms we're exempting. > > Done. I've now made it as a list. > > > > > > Off CL, let's sync up on the overall approach being taken here; it feels > like > > we > > > should only ever be falling back to h/w mapping if we get an un-mappable > > Unicode > > > KeySym for a key; then we wouldn't need this hack to exempt particular keys > > from > > > fall-back. > > I think it is easier to make a blacklist than a whitelist for entering > > MAP0~MAP3. > > Do you have any ideas of how to create a whitelist of "un-mappable Unicode"? > > With a second thought, I have a maybe-better idea: > > Current problem is that before entering map0-map3, it tries to stripe the > modifier states and check for [a-z] keys. > If we check [a-z] keys without striping the modifier states, we wouldn't have > the bugs like AltGr+V->XKEnter. > But that means the original behaviors would be changed. For example, for the > case AltGr+A->XK_B, original behavior gets VKEY_A, but this solution would get > VKEY_B. > That should be ok since it is a rare case and AFAIK no XKB layout actually has > that kind of mapping. > Another downside for the idea is about performance. For the case of > AltGr+V->XKEnter, it will go through map0-map3 and failed to find the mapped > VKEY and then fallback to KeyboardCodeFromXKeysym() at line 584. But considering > those are not common cases, I think this performance hit can be ignored. > > WDYT? Oh, I forgot that there would be potential breakages for cases (I made it up) like AltGr+9->XK_Enter.
OK, LGTM - let's discuss your ideas for simplifying this code separately, via email. Thanks :)
The CQ bit was checked by shuchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/664893004/120001
On 2014/10/30 00:43:18, Wez wrote: > OK, LGTM - let's discuss your ideas for simplifying this code separately, via > email. > > Thanks :) Thanks, will send mail for the discussion.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by shuchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/664893004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3f1b319d5fb0fe247046d858af70e12878524f0e Cr-Commit-Position: refs/heads/master@{#302047} |
