Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2097273002:
[Chromoting] Add InputMonitor and InputState (Closed)
Description[Chromoting] Add InputMonitor and InputState
Since gesture detection logic has no relationship with gesture handling logic,
this change adds InputMonitor and InputState to detect gestures and raise a set
of events to indicate different gestures.
This change won't impact existing logic, so should have no end user impacts.
Starting from this change, instead of refactoring existing code, a set of new
classes will be added. So we can easily test the differences between existing
implementation and new implementation. So currently no tests are added for
InputMonitor. For further test and replacement plan, please refer to the design
doc.
This is part of Remote Desktop Android Client Refactor work. Design doc can be
found at https://goo.gl/MA6zjx.
BUG=615277
Committed: https://crrev.com/1b741db696756e36216cb69719c162cb6617fde6
Cr-Commit-Position: refs/heads/master@{#406198}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Resolve review comments #
Messages
Total messages: 28 (17 generated)
Description was changed from ========== [Chromoting] Add InputMonitor and InputState Since gesture detection logic has no relationship with gesture handling logic, this change adds InputMonitor and InputState to detect gestures and raise a set of events to indicate different gestures. Starting from this change, instead of refactoring existing code, a set of new classes will be added. So we can easily test the differences between existing implementation and new implementation. So currently a very small set of tests are added for InputMonitor. For further test and replacement plan, please refer to the design doc. This is part of Remote Desktop Android Client Refactor work. Design doc can be found at https://goo.gl/MA6zjx. BUG=615277 ========== to ========== [Chromoting] Add InputMonitor and InputState with tests Since gesture detection logic has no relationship with gesture handling logic, this change adds InputMonitor and InputState to detect gestures and raise a set of events to indicate different gestures. Starting from this change, instead of refactoring existing code, a set of new classes will be added. So we can easily test the differences between existing implementation and new implementation. So currently a very small set of tests are added for InputMonitor. For further test and replacement plan, please refer to the design doc. This is part of Remote Desktop Android Client Refactor work. Design doc can be found at https://goo.gl/MA6zjx. BUG=615277 ==========
Description was changed from ========== [Chromoting] Add InputMonitor and InputState with tests Since gesture detection logic has no relationship with gesture handling logic, this change adds InputMonitor and InputState to detect gestures and raise a set of events to indicate different gestures. Starting from this change, instead of refactoring existing code, a set of new classes will be added. So we can easily test the differences between existing implementation and new implementation. So currently a very small set of tests are added for InputMonitor. For further test and replacement plan, please refer to the design doc. This is part of Remote Desktop Android Client Refactor work. Design doc can be found at https://goo.gl/MA6zjx. BUG=615277 ========== to ========== [Chromoting] Add InputMonitor and InputState with tests Since gesture detection logic has no relationship with gesture handling logic, this change adds InputMonitor and InputState to detect gestures and raise a set of events to indicate different gestures. This change won't impact existing logic, so should have no end user impacts. Starting from this change, instead of refactoring existing code, a set of new classes will be added. So we can easily test the differences between existing implementation and new implementation. So currently a very small set of tests are added for InputMonitor. For further test and replacement plan, please refer to the design doc. This is part of Remote Desktop Android Client Refactor work. Design doc can be found at https://goo.gl/MA6zjx. BUG=615277 ==========
Description was changed from ========== [Chromoting] Add InputMonitor and InputState with tests Since gesture detection logic has no relationship with gesture handling logic, this change adds InputMonitor and InputState to detect gestures and raise a set of events to indicate different gestures. This change won't impact existing logic, so should have no end user impacts. Starting from this change, instead of refactoring existing code, a set of new classes will be added. So we can easily test the differences between existing implementation and new implementation. So currently a very small set of tests are added for InputMonitor. For further test and replacement plan, please refer to the design doc. This is part of Remote Desktop Android Client Refactor work. Design doc can be found at https://goo.gl/MA6zjx. BUG=615277 ========== to ========== [Chromoting] Add InputMonitor and InputState Since gesture detection logic has no relationship with gesture handling logic, this change adds InputMonitor and InputState to detect gestures and raise a set of events to indicate different gestures. This change won't impact existing logic, so should have no end user impacts. Starting from this change, instead of refactoring existing code, a set of new classes will be added. So we can easily test the differences between existing implementation and new implementation. So currently no tests are added for InputMonitor. For further test and replacement plan, please refer to the design doc. This is part of Remote Desktop Android Client Refactor work. Design doc can be found at https://goo.gl/MA6zjx. BUG=615277 ==========
zijiehe@chromium.org changed reviewers: + lambroslambrou@chromium.org
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
Would it make sense to put more of the input handling logic to C++, where it can be shared with the iOS client?
On 2016/06/27 18:28:02, Sergey Ulanov wrote: > Would it make sense to put more of the input handling logic to C++, where it can > be shared with the iOS client? I would like to do so, and this change is part of the achievement. This change targets to decouple gesture detection logic with input handling logic, so input handling logic can be handled separately. Most of gesture detection are using stock Android APIs, such as GestureDetector / ScaleGestureDetector / TapGestureDetector. And InputMonitor is a container of all these detectors, and exports a set of events to indicate gestures we are interesting at.
Kindly ping.
lgtm, please wait for Sergey's feedback as well. Ultimately, it would be nice to have all OS-specific gesture-detection kept within InputMonitor. At the moment, you are still leaking Android MotionEvents to other classes, for example: mOnScrollFling.raise(new TwoPointsEventParameter(e1, e2, velocityX, velocityY)); The output of InputMonitor should probably be gesture data structures that don't use Android-specific MotionEvents. Then they can be passed to native code to implement gesture policy. https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/InputMonitor.java (right): https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/InputMonitor.java:62: // TouchInputHandler treats a gesture with three fingers as a swipe. To make sure we can easily I don't see the need for this flag and I don't think we will ever be concerned about 4-finger swipes. If, say, a 3-finger swipe shows the keyboard, and a user swipes 4 fingers and the keyboard still appears (or it fails to appear), will anybody complain? I would favor removing this flag and reporting all swipe events (4 finger or otherwise) to the downstream gesture handlers. Let downstream policy decide what to do with >= 4-finger gestures. Keep this file as a low-level gesture reporter, without making policy decisions here. https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/InputMonitor.java:63: // test the differences between TouchInputHandler and InputMonitor, we can set this variable to Also, better to avoid flags that make code behave differently under test than production. https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/InputState.java (right): https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/InputState.java:9: * finger count, detected action, etc. This class is not thread-safe. You can remove the thread-safety comment if you like. The default assumption is thread-unsafe. And code that deals with UI and input can be assumed to run on the main UI thread. https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/InputState.java:77: public boolean suppressCursorMovement() { Name this as a boolean-noun instead of a verb? Perhaps: getSuppressCursorMovement() or shouldSuppressCursorMovement() or suppressesCursorMovement() https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/InputState.java:83: public boolean suppressFling() { Same here. https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/InputState.java:88: public boolean scrollFling() { isScrollFling()
The CQ bit was checked by zijiehe@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...
Patchset #2 (id:20001) has been deleted
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by zijiehe@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...
I have also updated TwoPointsEventParameter to use two TapEventParameter(s) instead of two MotionEvent(s). https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/InputMonitor.java (right): https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/InputMonitor.java:62: // TouchInputHandler treats a gesture with three fingers as a swipe. To make sure we can easily On 2016/07/14 18:58:07, Lambros wrote: > I don't see the need for this flag and I don't think we will ever be concerned > about 4-finger swipes. If, say, a 3-finger swipe shows the keyboard, and a user > swipes 4 fingers and the keyboard still appears (or it fails to appear), will > anybody complain? > > I would favor removing this flag and reporting all swipe events (4 finger or > otherwise) to the downstream gesture handlers. Let downstream policy decide what > to do with >= 4-finger gestures. Keep this file as a low-level gesture reporter, > without making policy decisions here. > > > Personally I think swiping with three or four fingers should be treated similarly. But current implementation does not handle swiping with four fingers. But I would prefer to have automatically testing, i.e. generate random gesture events, send to a view, and compare the output of this InputMonitor and current implementation. Which means, I need to make sure this class behalves exactly the same as current implementation. But if you do not have strong opinion about swiping three or four fingers, I will change current implementation to avoid this flag. https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/InputMonitor.java:63: // test the differences between TouchInputHandler and InputMonitor, we can set this variable to On 2016/07/14 18:58:07, Lambros wrote: > Also, better to avoid flags that make code behave differently under test than > production. Done. https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/InputState.java (right): https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/InputState.java:9: * finger count, detected action, etc. This class is not thread-safe. On 2016/07/14 18:58:07, Lambros wrote: > You can remove the thread-safety comment if you like. > The default assumption is thread-unsafe. And code that deals with UI and input > can be assumed to run on the main UI thread. Done. https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/InputState.java:77: public boolean suppressCursorMovement() { On 2016/07/14 18:58:07, Lambros wrote: > Name this as a boolean-noun instead of a verb? Perhaps: > getSuppressCursorMovement() or > shouldSuppressCursorMovement() or > suppressesCursorMovement() > > Done. https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/InputState.java:83: public boolean suppressFling() { On 2016/07/14 18:58:07, Lambros wrote: > Same here. Done. https://codereview.chromium.org/2097273002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/InputState.java:88: public boolean scrollFling() { On 2016/07/14 18:58:07, Lambros wrote: > isScrollFling() Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi, Sergey, if you have no concern about this change, I will submit it by the end of tomorrow.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org Link to the patchset: https://codereview.chromium.org/2097273002/#ps40001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Add InputMonitor and InputState Since gesture detection logic has no relationship with gesture handling logic, this change adds InputMonitor and InputState to detect gestures and raise a set of events to indicate different gestures. This change won't impact existing logic, so should have no end user impacts. Starting from this change, instead of refactoring existing code, a set of new classes will be added. So we can easily test the differences between existing implementation and new implementation. So currently no tests are added for InputMonitor. For further test and replacement plan, please refer to the design doc. This is part of Remote Desktop Android Client Refactor work. Design doc can be found at https://goo.gl/MA6zjx. BUG=615277 ========== to ========== [Chromoting] Add InputMonitor and InputState Since gesture detection logic has no relationship with gesture handling logic, this change adds InputMonitor and InputState to detect gestures and raise a set of events to indicate different gestures. This change won't impact existing logic, so should have no end user impacts. Starting from this change, instead of refactoring existing code, a set of new classes will be added. So we can easily test the differences between existing implementation and new implementation. So currently no tests are added for InputMonitor. For further test and replacement plan, please refer to the design doc. This is part of Remote Desktop Android Client Refactor work. Design doc can be found at https://goo.gl/MA6zjx. BUG=615277 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Add InputMonitor and InputState Since gesture detection logic has no relationship with gesture handling logic, this change adds InputMonitor and InputState to detect gestures and raise a set of events to indicate different gestures. This change won't impact existing logic, so should have no end user impacts. Starting from this change, instead of refactoring existing code, a set of new classes will be added. So we can easily test the differences between existing implementation and new implementation. So currently no tests are added for InputMonitor. For further test and replacement plan, please refer to the design doc. This is part of Remote Desktop Android Client Refactor work. Design doc can be found at https://goo.gl/MA6zjx. BUG=615277 ========== to ========== [Chromoting] Add InputMonitor and InputState Since gesture detection logic has no relationship with gesture handling logic, this change adds InputMonitor and InputState to detect gestures and raise a set of events to indicate different gestures. This change won't impact existing logic, so should have no end user impacts. Starting from this change, instead of refactoring existing code, a set of new classes will be added. So we can easily test the differences between existing implementation and new implementation. So currently no tests are added for InputMonitor. For further test and replacement plan, please refer to the design doc. This is part of Remote Desktop Android Client Refactor work. Design doc can be found at https://goo.gl/MA6zjx. BUG=615277 Committed: https://crrev.com/1b741db696756e36216cb69719c162cb6617fde6 Cr-Commit-Position: refs/heads/master@{#406198} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1b741db696756e36216cb69719c162cb6617fde6 Cr-Commit-Position: refs/heads/master@{#406198} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
