Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
430463005:
linux/cros: Supports XKeyEvent to Unicode conversion independent from user's locale. (Closed)
|
Created:
6 years, 4 months ago by Yuki Modified:
5 years, 4 months ago CC:
chromium-reviews, tdresser+watch_chromium.org, kpschoedel Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Descriptionlinux/cros: Supports XKeyEvent to Unicode conversion independent from user's locale.
XLookupString depends on user's locale settings (LC_CTYPE). The returned string is encoded in LC_CTYPE, and if the string cannot be respresented in LC_CTYPE, it becomes an empty string. i.e. users cannot input characters.
This CL introduces our own map from KeySym (virtual key code in X) to Unicode character. This map works independent from user's locale.
BUG=395019
TEST=Done manually.
R=sadrul@chromium.org, wez@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289272
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Updated BUILD.gn #Patch Set 4 : Addressed review comments. #
Total comments: 28
Patch Set 5 : Addressed review comments. #
Total comments: 2
Patch Set 6 : Addressed review comments. #Patch Set 7 : Synced. #
Total comments: 6
Patch Set 8 : Addressed sadrul's review comments. #
Total comments: 2
Patch Set 9 : Addressed sadrul's review comments. #
Total comments: 1
Messages
Total messages: 22 (1 generated)
Could you review this CL? I think that, with this fix, we can get rid of a lot of locale-dependent issues on linux/aura.
On 2014/08/02 11:56:57, Yuki wrote: > Could you review this CL? > > I think that, with this fix, we can get rid of a lot of locale-dependent issues > on linux/aura. Sorry, haven't got to this yet. :( Will take a look tomorrow. Where did we get the table contents from?
On 2014/08/05 00:51:41, Wez wrote: > On 2014/08/02 11:56:57, Yuki wrote: > > Could you review this CL? > > > > I think that, with this fix, we can get rid of a lot of locale-dependent > issues > > on linux/aura. > > Sorry, haven't got to this yet. :( Will take a look tomorrow. > > Where did we get the table contents from? I took a look the KeySym-to-Unicode table that you told me, but its content is a little poor compared to this CL. I'll check the contents more carefully tomorrow, but I'd like to go with my table probably a little modified. At a glance, my table lacks Hangul characters, but supports Armenian, Georgian, and Braille at least which are not supported the table you told. My table was generated from /usr/include/X11/keysymdef.h, which has lines like this: #define XK_a 0x0061 /* U+0061 LATIN SMALL LETTER A */ From this line, we can say XK_a should be mapped to U+0061. Only control characters and numeric pad are exceptions. In keysymdef.h, Hangul characters lack the comments like above (U+xxxx), and it seems Korean users don't assign these XK_Hangul_xxx to hardware keys. They seem using IMEs to input Hangul. So I didn't add Hangul characters in the first CL, but probably I should add them. I'll take a closer look on both of tables and update the table in this CL. Could you review other part of this CL?
OK, thanks for taking the time to check that. I think using the old-style KeySym-> Unicode KeySym mapping approach, and then extracting the Unicode value would be a reasonable modification of your CL, since it keeps the two aspects separate. WDYT? On 5 August 2014 07:55, <yukishiino@chromium.org> wrote: > On 2014/08/05 00:51:41, Wez wrote: > >> On 2014/08/02 11:56:57, Yuki wrote: >> > Could you review this CL? >> > >> > I think that, with this fix, we can get rid of a lot of locale-dependent >> issues >> > on linux/aura. >> > > Sorry, haven't got to this yet. :( Will take a look tomorrow. >> > > Where did we get the table contents from? >> > > I took a look the KeySym-to-Unicode table that you told me, but its > content is a > little poor compared to this CL. I'll check the contents more carefully > tomorrow, but I'd like to go with my table probably a little modified. At > a > glance, my table lacks Hangul characters, but supports Armenian, Georgian, > and > Braille at least which are not supported the table you told. > > My table was generated from /usr/include/X11/keysymdef.h, which has lines > like > this: > #define XK_a 0x0061 /* U+0061 LATIN SMALL > LETTER A > */ > From this line, we can say XK_a should be mapped to U+0061. > Only control characters and numeric pad are exceptions. > > In keysymdef.h, Hangul characters lack the comments like above (U+xxxx), > and it > seems Korean users don't assign these XK_Hangul_xxx to hardware keys. > They seem > using IMEs to input Hangul. So I didn't add Hangul characters in the > first CL, > but probably I should add them. > > I'll take a closer look on both of tables and update the table in this CL. > Could you review other part of this CL? > > https://codereview.chromium.org/430463005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
First, I changed the code a little bit and now it treats Latin 1 and "unicode style keysyms" in a special manner. Second, I checked what keysyms are included in KeysymUtil.cxx and this CL. This CL covers some additional (probably new) keysyms compared to KeysymUtil.cxx, except for Hangul. So let's use this table. For Hangul, there seem no trivial mappings from keysyms to unicode characters. I found several different mappings when I searched on the web. In addition, XLookupString maps XK_Hangul_A to U+1161 while KeysymUtil.cxx maps it to U+314F. They are basically the same character, but one is Hangul Jamo and the other is Hangul Compatibility Jamo. So, the mapping seem not trivial in Hangul. Since the mapping in this CL is used for direct character input (without IMEs), I think there is no need to support (or define) XK_Hangul_xxx to unicode mapping for the time being. Could you take another look?
https://codereview.chromium.org/430463005/diff/60001/ui/events/BUILD.gn File ui/events/BUILD.gn (right): https://codereview.chromium.org/430463005/diff/60001/ui/events/BUILD.gn#newco... ui/events/BUILD.gn:63: "keycodes/keysym_to_unicode_x.h", Since this conversion isn't anything to do with VKEY codes, should it live under the x/ subdirectory? You could then drop _x from the names. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... File ui/events/keycodes/keysym_to_unicode_x.cc (right): https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:45: // Control characters nit: I'd suggest adding a blank line before each section header comment, to break the table up visually, making it easier to grok. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:74: // Latin 1 (all keysyms are compatible with unicode) nit: Suggest "Latin 1 KeySyms map 1:1 to Unicode." https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:76: {XK_Aogonek, 0x0104}, // LATIN CAPITAL LETTER A WITH OGONEK Can we wrap these comments to start indented on the next line, rather than exceeding the 80-character limit? https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:192: // Latin 8 (all keysyms are unicode style) s/unicode/Unicode, here and below. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:763: // Armenian (all keysyms are unicode style) nit: Clarify/reword this; do these KeySyms map 1:1 to Unicode, or does Armenian use the new Unicode KeySym form? https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:780: g_keysym_to_unicode_table[i].unicode; Do you need a hash table, or could you get away with an STL binary search here? Is the table well-ordered? https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:790: // Unicode-style keysym s/keysym/KeySym here and throughout this impl https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:790: // Unicode-style keysym nit: Missing . https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:793: return (unicode & ~0xffff) ? 0 : static_cast<uint16_t>(unicode); Break out the unicode & ~0xffff test into an early-return 0? Also consider adding a comment to indicate that we don't handle Unicode KeySyms outside the Basic Plane. Doesn't this mean we can't handle e.g. Chinese GB13080? https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:796: // Other random keysyms nit: Missing . https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:796: // Other random keysyms nit: They're not "random"; they're a mixture of pre-Unicode and non-Unicode KeySyms for particular languages, or special usages. ;) https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... File ui/events/keycodes/keysym_to_unicode_x.h (right): https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.h:12: // Returns a unicode character corresponding to the given |keysym|. If the s/unicode/Unicode https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.h:14: uint16_t GetUnicodeCharacterFromXKeySym(unsigned long keysym); You're only returning a 16-bit value, so you can only return Unicode characters from the Basic Plane, surely?
https://codereview.chromium.org/430463005/diff/60001/ui/events/BUILD.gn File ui/events/BUILD.gn (right): https://codereview.chromium.org/430463005/diff/60001/ui/events/BUILD.gn#newco... ui/events/BUILD.gn:63: "keycodes/keysym_to_unicode_x.h", On 2014/08/07 23:37:11, Wez wrote: > Since this conversion isn't anything to do with VKEY codes, should it live under > the x/ subdirectory? You could then drop _x from the names. Done. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... File ui/events/keycodes/keysym_to_unicode_x.cc (right): https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:45: // Control characters On 2014/08/07 23:37:11, Wez wrote: > nit: I'd suggest adding a blank line before each section header comment, to > break the table up visually, making it easier to grok. Done. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:74: // Latin 1 (all keysyms are compatible with unicode) On 2014/08/07 23:37:12, Wez wrote: > nit: Suggest "Latin 1 KeySyms map 1:1 to Unicode." Done. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:76: {XK_Aogonek, 0x0104}, // LATIN CAPITAL LETTER A WITH OGONEK On 2014/08/07 23:37:11, Wez wrote: > Can we wrap these comments to start indented on the next line, rather than > exceeding the 80-character limit? I think that this table is just simple data. I generated this table with using grep, sed, etc. and I think other people may use those tools in future. So I'd like to keep the simple structure here. Everything is on one line and there are XK_xxx, Unicode, and optional comment describes Unicode character. Usually I don't want to break 80 column rule, but this is an exception. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:192: // Latin 8 (all keysyms are unicode style) On 2014/08/07 23:37:11, Wez wrote: > s/unicode/Unicode, here and below. Done. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:763: // Armenian (all keysyms are unicode style) On 2014/08/07 23:37:12, Wez wrote: > nit: Clarify/reword this; do these KeySyms map 1:1 to Unicode, or does Armenian > use the new Unicode KeySym form? Done. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:780: g_keysym_to_unicode_table[i].unicode; On 2014/08/07 23:37:11, Wez wrote: > Do you need a hash table, or could you get away with an STL binary search here? > Is the table well-ordered? If I can use C++11, it's a piece of cake. I changed the code to use std::unordered_map. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:790: // Unicode-style keysym On 2014/08/07 23:37:11, Wez wrote: > s/keysym/KeySym here and throughout this impl Done. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:790: // Unicode-style keysym On 2014/08/07 23:37:11, Wez wrote: > nit: Missing . Done. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:793: return (unicode & ~0xffff) ? 0 : static_cast<uint16_t>(unicode); On 2014/08/07 23:37:12, Wez wrote: > Break out the unicode & ~0xffff test into an early-return 0? > > Also consider adding a comment to indicate that we don't handle Unicode KeySyms > outside the Basic Plane. > > Doesn't this mean we can't handle e.g. Chinese GB13080? Done. If you assigned a character outside the Basic Plane to a key, and you used the key to input the character, we don't support such a case. But no worry, it usually doesn't happen. Those who'd like to type GB13080 characters use an IME. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:796: // Other random keysyms On 2014/08/07 23:37:11, Wez wrote: > nit: They're not "random"; they're a mixture of pre-Unicode and non-Unicode > KeySyms for particular languages, or special usages. ;) Done. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.cc:796: // Other random keysyms On 2014/08/07 23:37:11, Wez wrote: > nit: Missing . Done. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... File ui/events/keycodes/keysym_to_unicode_x.h (right): https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.h:12: // Returns a unicode character corresponding to the given |keysym|. If the On 2014/08/07 23:37:12, Wez wrote: > s/unicode/Unicode Done. https://codereview.chromium.org/430463005/diff/60001/ui/events/keycodes/keysy... ui/events/keycodes/keysym_to_unicode_x.h:14: uint16_t GetUnicodeCharacterFromXKeySym(unsigned long keysym); On 2014/08/07 23:37:12, Wez wrote: > You're only returning a 16-bit value, so you can only return Unicode characters > from the Basic Plane, surely? Yes, it's intentional. No one uses a keyboard whose key is assigned to a character outside the Basic Plane. Should be no problem. Note that ui::KeyEvent::GetCharacter() returns base::char16, so it's meaningless to support such a character for now.
lgtm https://codereview.chromium.org/430463005/diff/80001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/430463005/diff/80001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:846: const XKeyEvent* xkey; nit: Style guide requires local variables be initialized at declaration; consider initializing to &xev->xkey and then overriding that in the GenericEvent case? Or might xkey not be initialized in that case?
sadrul@, could you review as an owner of:
ui/events/BUILD.gn
https://codereview.chromium.org/430463005/diff/80001/ui/events/keycodes/keybo...
File ui/events/keycodes/keyboard_code_conversion_x.cc (right):
https://codereview.chromium.org/430463005/diff/80001/ui/events/keycodes/keybo...
ui/events/keycodes/keyboard_code_conversion_x.cc:846: const XKeyEvent* xkey;
On 2014/08/08 22:56:25, Wez wrote:
> nit: Style guide requires local variables be initialized at declaration;
> consider initializing to &xev->xkey and then overriding that in the
GenericEvent
> case? Or might xkey not be initialized in that case?
Done.
https://codereview.chromium.org/430463005/diff/120001/ui/events/x/keysym_to_u... File ui/events/x/keysym_to_unicode.cc (right): https://codereview.chromium.org/430463005/diff/120001/ui/events/x/keysym_to_u... ui/events/x/keysym_to_unicode.cc:32: #define XK_SINHALA Do these defines need to be before the X/keysym.h includes? If so, can you add a comment above this block? https://codereview.chromium.org/430463005/diff/120001/ui/events/x/keysym_to_u... ui/events/x/keysym_to_unicode.cc:799: }; Do you mind running this through 'git cl format'? https://codereview.chromium.org/430463005/diff/120001/ui/events/x/keysym_to_u... ui/events/x/keysym_to_unicode.cc:834: }; DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/430463005/diff/120001/ui/events/x/keysym_to_u... File ui/events/x/keysym_to_unicode.cc (right): https://codereview.chromium.org/430463005/diff/120001/ui/events/x/keysym_to_u... ui/events/x/keysym_to_unicode.cc:32: #define XK_SINHALA On 2014/08/11 18:59:26, sadrul wrote: > Do these defines need to be before the X/keysym.h includes? If so, can you add a > comment above this block? Done. https://codereview.chromium.org/430463005/diff/120001/ui/events/x/keysym_to_u... ui/events/x/keysym_to_unicode.cc:799: }; On 2014/08/11 18:59:26, sadrul wrote: > Do you mind running this through 'git cl format'? This is a simple data table and I'd like to apply the simplest format to it so we can easily use text utilities such as grep and sed. Actually I generated this table from <X11/keysym.h> with using grep, sed, etc., which are line-oriented text utilities. So if you don't mind, I'd like to keep this structure; one entry on one line. https://codereview.chromium.org/430463005/diff/120001/ui/events/x/keysym_to_u... ui/events/x/keysym_to_unicode.cc:834: }; On 2014/08/11 18:59:26, sadrul wrote: > DISALLOW_COPY_AND_ASSIGN Done.
sadrul@, could you take another look?
/cc +kpschoedel@ LGTM https://codereview.chromium.org/430463005/diff/140001/ui/events/x/keysym_to_u... File ui/events/x/keysym_to_unicode.cc (right): https://codereview.chromium.org/430463005/diff/140001/ui/events/x/keysym_to_u... ui/events/x/keysym_to_unicode.cc:836: KeySymToUnicodeMap keysym_to_unicode_map; keysym_to_unicode_map_;
Thank you guys for the review. Will commit. https://codereview.chromium.org/430463005/diff/140001/ui/events/x/keysym_to_u... File ui/events/x/keysym_to_unicode.cc (right): https://codereview.chromium.org/430463005/diff/140001/ui/events/x/keysym_to_u... ui/events/x/keysym_to_unicode.cc:836: KeySymToUnicodeMap keysym_to_unicode_map; On 2014/08/13 04:45:59, sadrul wrote: > keysym_to_unicode_map_; Done.
The CQ bit was checked by yukishiino@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yukishiino@chromium.org/430463005/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Message was sent while issue was closed.
Committed patchset #9 manually as 289272 (presubmit successful).
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/430463005/diff/160001/ui/events/x/keysym_to_u... File ui/events/x/keysym_to_unicode.cc (right): https://codereview.chromium.org/430463005/diff/160001/ui/events/x/keysym_to_u... ui/events/x/keysym_to_unicode.cc:38: #include <unordered_map> Looks like this adds an include of unordered_map – please don't use C++11 library features yet. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
