Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2792413002:
Fix accelerator history tracking (Closed)
DescriptionFix accelerator history tracking
Track the currently pressed keys so that we don't mistakenly store an
already pressed key as a new keypress after another key has been released.
BUG=704280
TEST=covered by tests.
Review-Url: https://codereview.chromium.org/2792413002
Cr-Commit-Position: refs/heads/master@{#465337}
Committed: https://chromium.googlesource.com/chromium/src/+/820f3f7b4622520426a295d93852701caa204a2f
Patch Set 1 #Patch Set 2 : Add test #
Total comments: 2
Messages
Total messages: 27 (16 generated)
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
afakhry@chromium.org changed reviewers: + sadrul@chromium.org
Sadrul, could you please take a look? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by afakhry@chromium.org to run a CQ dry run
Sadrul, friendly ping.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2792413002/diff/20001/ui/base/accelerators/ac... File ui/base/accelerators/accelerator_history.cc (right): https://codereview.chromium.org/2792413002/diff/20001/ui/base/accelerators/ac... ui/base/accelerators/accelerator_history.cc:22: if (!currently_pressed_keys_.emplace(accelerator.key_code()).second) What code is generating the second Accelerator? Is it the repeat key-press event for the search key? Does that event have the REPEAT flag turned on? Can we filter based on that flag?
https://codereview.chromium.org/2792413002/diff/20001/ui/base/accelerators/ac... File ui/base/accelerators/accelerator_history.cc (right): https://codereview.chromium.org/2792413002/diff/20001/ui/base/accelerators/ac... ui/base/accelerators/accelerator_history.cc:22: if (!currently_pressed_keys_.emplace(accelerator.key_code()).second) On 2017/04/07 05:43:39, sadrul wrote: > What code is generating the second Accelerator? Is it the repeat key-press event > for the search key? Does that event have the REPEAT flag turned on? Can we > filter based on that flag? Unfortunately, REPEAT is unreliable in this case. When we press and hold Alt, Search, then release Alt (while still holding Search) a new key_event is generated for Search right after releasing Alt, which is not marked as REPEAT. The way that REPEAT flag is set is broken in this case since it only compares the current event with the previous event: https://cs.chromium.org/chromium/src/ui/events/event.cc?l=1123. The Alt_Release event in the middle of the Search repeats breaks this. We need to track the currently pressed events.
On 2017/04/07 15:42:57, afakhry wrote: > https://codereview.chromium.org/2792413002/diff/20001/ui/base/accelerators/ac... > File ui/base/accelerators/accelerator_history.cc (right): > > https://codereview.chromium.org/2792413002/diff/20001/ui/base/accelerators/ac... > ui/base/accelerators/accelerator_history.cc:22: if > (!currently_pressed_keys_.emplace(accelerator.key_code()).second) > On 2017/04/07 05:43:39, sadrul wrote: > > What code is generating the second Accelerator? Is it the repeat key-press > event > > for the search key? Does that event have the REPEAT flag turned on? Can we > > filter based on that flag? > > Unfortunately, REPEAT is unreliable in this case. When we press and hold Alt, > Search, then release Alt (while still holding Search) a new key_event is > generated for Search right after releasing Alt, which is not marked as REPEAT. > > The way that REPEAT flag is set is broken in this case since it only compares > the current event with the previous event: > https://cs.chromium.org/chromium/src/ui/events/event.cc?l=1123. > The Alt_Release event in the middle of the Search repeats breaks this. We need > to track the currently pressed events. Sadrul, friendly ping.
The entire AcceleratorHistory business seems ... hacky. I am going to take another look at this change, but it would be good to investigate if we can do things better.
On 2017/04/13 13:41:54, sadrul wrote: > The entire AcceleratorHistory business seems ... hacky. I am going to take > another look at this change, but it would be good to investigate if we can do > things better. Not just the AcceleratorHistory, but honestly the entire accelerator handling business. AcceleratorHistory is kind of a hack to fix another hack: for example, for key combos where we need to make sure they're handled only if the previous accelerator is of a particular value. Examples are: - CanHandleDisableCapsLock(): https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller.... - CanHandleToggleAppList(): https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller.... - CanHandleToggleCapsLock(): https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller.... All of the above don't work reliably all the time because of the issue this CL is trying to fix. But I agree with you, a much bigger refactor is needed. :/
On 2017/04/14 18:03:46, afakhry wrote: > On 2017/04/13 13:41:54, sadrul wrote: > > The entire AcceleratorHistory business seems ... hacky. I am going to take > > another look at this change, but it would be good to investigate if we can do > > things better. > > Not just the AcceleratorHistory, but honestly the entire accelerator handling > business. > AcceleratorHistory is kind of a hack to fix another hack: for example, for key > combos where > we need to make sure they're handled only if the previous accelerator is of a > particular value. > > Examples are: > - CanHandleDisableCapsLock(): > https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller.... > > - CanHandleToggleAppList(): > https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller.... > > - CanHandleToggleCapsLock(): > https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller.... > > All of the above don't work reliably all the time because of the issue this CL > is trying to fix. > > But I agree with you, a much bigger refactor is needed. :/ I think the fact that we process the accelerators implicitly during event-dispatch is a problem. Accelerator handling should be a separate and explicit step during the dispatch. It would be awesome if someone worked on cleaning things up. lgtm
On 2017/04/18 17:27:41, sadrul wrote: > On 2017/04/14 18:03:46, afakhry wrote: > > On 2017/04/13 13:41:54, sadrul wrote: > > > The entire AcceleratorHistory business seems ... hacky. I am going to take > > > another look at this change, but it would be good to investigate if we can > do > > > things better. > > > > Not just the AcceleratorHistory, but honestly the entire accelerator handling > > business. > > AcceleratorHistory is kind of a hack to fix another hack: for example, for key > > combos where > > we need to make sure they're handled only if the previous accelerator is of a > > particular value. > > > > Examples are: > > - CanHandleDisableCapsLock(): > > > https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller.... > > > > - CanHandleToggleAppList(): > > > https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller.... > > > > - CanHandleToggleCapsLock(): > > > https://cs.chromium.org/chromium/src/ash/accelerators/accelerator_controller.... > > > > All of the above don't work reliably all the time because of the issue this CL > > is trying to fix. > > > > But I agree with you, a much bigger refactor is needed. :/ > > I think the fact that we process the accelerators implicitly during > event-dispatch is a problem. Accelerator handling should be a separate and > explicit step during the dispatch. It would be awesome if someone worked on > cleaning things up. > > lgtm Thanks! I completely agree. I already spoke to Albert about the need for this, and from my understanding, he added this task to our backlog so we can get to it when priorities permit.
The CQ bit was checked by afakhry@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492539299336640,
"parent_rev": "b6f9cf966fc35b569f749c379d6536f44c8667ba", "commit_rev":
"820f3f7b4622520426a295d93852701caa204a2f"}
Message was sent while issue was closed.
Description was changed from ========== Fix accelerator history tracking Track the currently pressed keys so that we don't mistakenly store an already pressed key as a new keypress after another key has been released. BUG=704280 TEST=covered by tests. ========== to ========== Fix accelerator history tracking Track the currently pressed keys so that we don't mistakenly store an already pressed key as a new keypress after another key has been released. BUG=704280 TEST=covered by tests. Review-Url: https://codereview.chromium.org/2792413002 Cr-Commit-Position: refs/heads/master@{#465337} Committed: https://chromium.googlesource.com/chromium/src/+/820f3f7b4622520426a295d93852... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/820f3f7b4622520426a295d93852... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
