Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
243143002:
linux-aura: Supports Compose key with XIM. (Closed)
|
Created:
6 years, 8 months ago by Yuki Modified:
6 years, 7 months ago CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, penghuang+watch_chromium.org, nona+watch_chromium.org, kalyank, ben+aura_chromium.org, James Su, pkotwicz Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionlinux-aura: Supports Compose key with XIM.
Supports Compose key with XIM and makes .XCompose settings effective with Linux Aura build.
BUG=351230, 360388
TEST=Done manually. Tested with GTK_IM_MODULE=xim XMODIFIERS=@im=none and ~/.XCompose with custom contents.
R=erg@chromium.org, sadrul@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266651
Patch Set 1 #
Total comments: 4
Patch Set 2 : Synced. #Patch Set 3 : Addressed review comments. #
Total comments: 2
Patch Set 4 : Addressed a review comment. #Patch Set 5 : Synced. #Patch Set 6 : Synced. #Patch Set 7 : Disabled EF_IME_FABRICATED_KEY flag on CrOS. #
Messages
Total messages: 48 (0 generated)
Could you guys review this CL? erg@: c/b/ui/libgtk2ui/ sadrul@: ui/aura/ ui/events/
lgtm
https://codereview.chromium.org/243143002/diff/1/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/243143002/diff/1/ui/events/event.h#newcode591 ui/events/event.h:591: bool is_fabricated_by_ime_; How is this different from TranslatedKeyEvent? What's the plan for addressing crbug.com/339355?
https://codereview.chromium.org/243143002/diff/1/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/243143002/diff/1/ui/events/event.h#newcode591 ui/events/event.h:591: bool is_fabricated_by_ime_; On 2014/04/18 19:01:48, sadrul wrote: > How is this different from TranslatedKeyEvent? What's the plan for addressing > crbug.com/339355? TranslatedKeyEvent represents that: 1. The key event was generated by a real user action as same as KeyEvent, and 2. Our own IMF/IME has already processed the event once, so there is no need to pass the event to IMF/IME again. (So we can avoid infinite loop.) is_fabricated_by_ime_ represents that: 1. The key event was NOT DIRECTLY generated by a user action. Some IMEs need hacky tricks that add fake key events to the event queue. This is that kind of fake key event. 2. Thus, we need to pass this key event to the underlying IME. So they're totally different things. I remember crbug.com/339355 and will address separately.
https://codereview.chromium.org/243143002/diff/1/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/243143002/diff/1/ui/events/event.h#newcode591 ui/events/event.h:591: bool is_fabricated_by_ime_; On 2014/04/21 02:38:30, Yuki wrote: > On 2014/04/18 19:01:48, sadrul wrote: > > How is this different from TranslatedKeyEvent? What's the plan for addressing > > crbug.com/339355? > > TranslatedKeyEvent represents that: > 1. The key event was generated by a real user action as same as KeyEvent, and > 2. Our own IMF/IME has already processed the event once, so there is no need to > pass the event to IMF/IME again. (So we can avoid infinite loop.) > > is_fabricated_by_ime_ represents that: > 1. The key event was NOT DIRECTLY generated by a user action. Some IMEs need > hacky tricks that add fake key events to the event queue. This is that kind of > fake key event. > 2. Thus, we need to pass this key event to the underlying IME. > > So they're totally different things. I remember crbug.com/339355 and will > address separately. I think it will be cleaner to have this as a KeyEventFlag instead.
https://codereview.chromium.org/243143002/diff/1/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/243143002/diff/1/ui/events/event.h#newcode591 ui/events/event.h:591: bool is_fabricated_by_ime_; On 2014/04/22 15:34:29, sadrul wrote: > On 2014/04/21 02:38:30, Yuki wrote: > > On 2014/04/18 19:01:48, sadrul wrote: > > > How is this different from TranslatedKeyEvent? What's the plan for > addressing > > > crbug.com/339355? > > > > TranslatedKeyEvent represents that: > > 1. The key event was generated by a real user action as same as KeyEvent, and > > 2. Our own IMF/IME has already processed the event once, so there is no need > to > > pass the event to IMF/IME again. (So we can avoid infinite loop.) > > > > is_fabricated_by_ime_ represents that: > > 1. The key event was NOT DIRECTLY generated by a user action. Some IMEs need > > hacky tricks that add fake key events to the event queue. This is that kind > of > > fake key event. > > 2. Thus, we need to pass this key event to the underlying IME. > > > > So they're totally different things. I remember crbug.com/339355 and will > > address separately. > > I think it will be cleaner to have this as a KeyEventFlag instead. Done.
https://codereview.chromium.org/243143002/diff/40001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/243143002/diff/40001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:822: const bool fabricated_by_xim = xev->xkey.keycode == 0 && xev->xkey.state == 0; Can you add this function in events_x.cc instead (or just use this logic in GetEventFlagsFromXKeyEvent() with the comment)? It's small enough and isn't used elsewhere. DCHECK on xev->type being one of KeyPress/KeyRelease.
https://codereview.chromium.org/243143002/diff/40001/ui/events/keycodes/keybo... File ui/events/keycodes/keyboard_code_conversion_x.cc (right): https://codereview.chromium.org/243143002/diff/40001/ui/events/keycodes/keybo... ui/events/keycodes/keyboard_code_conversion_x.cc:822: const bool fabricated_by_xim = xev->xkey.keycode == 0 && xev->xkey.state == 0; On 2014/04/23 14:30:58, sadrul wrote: > Can you add this function in events_x.cc instead (or just use this logic in > GetEventFlagsFromXKeyEvent() with the comment)? It's small enough and isn't used > elsewhere. > > DCHECK on xev->type being one of KeyPress/KeyRelease. Done.
lgtm
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/243143002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
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/243143002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
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/243143002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
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/243143002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
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/243143002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
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/243143002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
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/243143002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
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/243143002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
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/243143002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on android_dbg_triggered_tests
Message was sent while issue was closed.
Committed patchset #7 manually as r266651 (presubmit successful).
Message was sent while issue was closed.
On 2014/04/28 20:50:08, piman wrote: > Committed patchset #7 manually as r266651 (presubmit successful). FYI, I committed manually to make sure it makes the dev channel this week.
Message was sent while issue was closed.
On 2014/04/28 20:50:37, piman wrote: > On 2014/04/28 20:50:08, piman wrote: > > Committed patchset #7 manually as r266651 (presubmit successful). > > FYI, I committed manually to make sure it makes the dev channel this week. Thanks for committing. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
