Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
429633002:
Added multi-finger gestures to touch_exploration_controller (Closed)
DescriptionAdded multi-finger gestures to touch_exploration_controller
Two fingers: up is go to top, down is read from here, left is browser back, right is browser forward.
Three fingers: up and down map to page up/down.
Three fingers: up and down scroll by a fixed amount, left and right scroll between tabs.
Four fingers: up is home page, down is refresh, left and right toggle brightness.
BUG=387304
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288857
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289334
Patch Set 1 #Patch Set 2 : fixed weird indents #
Total comments: 6
Patch Set 3 : updated swipe gesture code, added test #
Total comments: 20
Patch Set 4 : removed some tap timer checks, updated comments, nit fixes #Patch Set 5 : added closure mappings to release key events for gestures #
Total comments: 6
Patch Set 6 : code review #Patch Set 7 : from the original branch #
Total comments: 35
Patch Set 8 : reshuffled code - NOTREACHED - OnGestureEvent #Patch Set 9 : gesture provider doesn't recieve all events and is reset in ResetToNoFingersDown #
Total comments: 8
Patch Set 10 : nit changes #
Total comments: 20
Patch Set 11 : SetState function #
Total comments: 10
Patch Set 12 : check closures exist before running #
Total comments: 8
Patch Set 13 : rebased patch for Lisa #Patch Set 14 : bind key events in a function #Patch Set 15 : permutations test disabled since it takes so long #Patch Set 16 : rebased #
Messages
Total messages: 43 (0 generated)
https://codereview.chromium.org/429633002/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:386: if (!(type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED || nit: this can be simplified to if (type != ui::ET_TOUCH_RELEASED && type != ui::ET_TOUCH_CANCELLED && ... Not much in it, but I find that way more readable as it's easy to miss the ! at the start of the big disjunction there. https://codereview.chromium.org/429633002/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:717: DispatchShiftSearchKeyEvent(ui::VKEY_LEFT); Speculation: it might be nice to keep track of these in a data structure rather than in code like this. Since you're forced to check the direction manually, you could have a set of maps something like: map<int, base::Closure> left_swipe_gestures = ... left_swipe_gestures[1] = base::Bind(&DispatchShiftSearchKeyEvent, ui::VKEY_LEFT); ... and then if (event_details.swipe_left()) { left_swipe_gestures[num_fingers].Run() } else if (event_details.swipe_right() ... Other reviewers might think this is a silly idea, though :) You'd also need to handle the case where no action was mapped for that combination of swipe direction and num_fingers. https://codereview.chromium.org/429633002/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/429633002/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:402: // It's initialized to false and set to false in ResetToNoFingersDown. Where is it set to true?
Multi finger gestures! I think I'm ready for review now - there are a few changes from the way the code looked when we were demoing. I'll soon try Alice's changes to how gesture triggered events are dispatched. https://codereview.chromium.org/429633002/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:386: if (!(type == ui::ET_TOUCH_RELEASED || type == ui::ET_TOUCH_CANCELLED || On 2014/07/30 00:25:15, aboxhall wrote: > nit: this can be simplified to > if (type != ui::ET_TOUCH_RELEASED && type != ui::ET_TOUCH_CANCELLED && ... > Not much in it, but I find that way more readable as it's easy to miss the ! at > the start of the big disjunction there. Done. https://codereview.chromium.org/429633002/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:717: DispatchShiftSearchKeyEvent(ui::VKEY_LEFT); On 2014/07/30 00:25:15, aboxhall wrote: > Speculation: it might be nice to keep track of these in a data structure rather > than in code like this. Since you're forced to check the direction manually, you > could have a set of maps something like: > map<int, base::Closure> left_swipe_gestures = ... > left_swipe_gestures[1] = base::Bind(&DispatchShiftSearchKeyEvent, > ui::VKEY_LEFT); > ... > and then > if (event_details.swipe_left()) { > left_swipe_gestures[num_fingers].Run() > } else if (event_details.swipe_right() ... > Other reviewers might think this is a silly idea, though :) You'd also need to > handle the case where no action was mapped for that combination of swipe > direction and num_fingers. This is pretty cool! I'm not that familiar with maps, closures, and binds in C++, but I'll definitely take a look. I'm going to upload what I have right now so reviewers can compare the two approaches - and also because I'd like review on the rest of what I've done so far :) https://codereview.chromium.org/429633002/diff/20001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/429633002/diff/20001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:402: // It's initialized to false and set to false in ResetToNoFingersDown. On 2014/07/30 00:25:16, aboxhall wrote: > Where is it set to true? (this variable was removed, the code flow should be a bit smoother)
Looking at tests now... https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:84: // Always process gesture events in the gesture provider. Only one swipe I don't understand the second sentence of this comment. https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:253: if (tap_timer_.IsRunning()) This check shouldn't be necessary: https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.h... https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:392: // If no gesture is registered before the tap timer times out, the state So the gesture grace period is the same as the double tap delay? https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:591: if(current_touch_ids_.size() == 1){ nit: space between if and (, and ) and { https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:757: // Brightness can be important for vision users, but none of these nit: low vision users https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:758: // mappings are permanent. Four finger gestures shoudl probably nit: "shoudl" https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:219: // Dispatches a single key with no flags. Nit: this comment is clearly out of date.
Tests mostly look good; I'm a little confused as to why the slide gesture/volume change tests are different now, though... https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1198: VLOG(0) << "delta time is: " << delta_time_ms; logspam? https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1211: // Most of the time this is 2 right now, but two of the two finger// nit: trailing // https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1494: initial_press.x() + gesture_detector_config_.touch_slop + 1, 1); I'm confused by this - this looks like it would move it off the right of the screen?
Thanks for the fast reply Alice! When I added multi finger gestures, I changed the way that gestures in general are processed e.g. the fact that gesture events are always sent to the gesture provider in rewrite_event. This changed when slide events were discovered in the code, and also uncovered to us that they were probably being doubly processed before (since there were 2 earcons, and now there was 1). It was a bit confusing for me, so definitely call us out on anything you still don't think should have needed to be changed. https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:84: // Always process gesture events in the gesture provider. Only one swipe On 2014/08/04 23:38:53, aboxhall wrote: > I don't understand the second sentence of this comment. Done. https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:253: if (tap_timer_.IsRunning()) On 2014/08/04 23:38:53, aboxhall wrote: > This check shouldn't be necessary: > https://code.google.com/p/chromium/codesearch#chromium/src/base/timer/timer.h... Thanks for catching that! Updated some other parts of the code too. https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:392: // If no gesture is registered before the tap timer times out, the state On 2014/08/04 23:38:53, aboxhall wrote: > So the gesture grace period is the same as the double tap delay? Yes, any time in this code that the tap timer is started, we use the double tap timeout. This happens: * when a finger is pressed and the user enters SINGLE_TAP_PRESSED state * when the user adds another tap after a first has been released and the state changes to DOUBLE_TAP_PENDING * when the user lifts a finger from touch exploration and enters TOUCH_EXPLORE_RELEASED (which is waiting for a potential tap to send a single tap click) In this case, the tap timer was started in the first of those three. Do you think this is a bit confusing how we did it? And would you suggest using different timeouts for any of those three? on a related side note: Lisa is currently doing a longer tap timer to use for entering passthrough mode on corner passthrough, and she'll add it to the double tap press passthrough mode too. https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:591: if(current_touch_ids_.size() == 1){ On 2014/08/04 23:38:53, aboxhall wrote: > nit: space between if and (, and ) and { Done. https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:757: // Brightness can be important for vision users, but none of these On 2014/08/04 23:38:53, aboxhall wrote: > nit: low vision users Thanks! That's what I meant :) https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:758: // mappings are permanent. Four finger gestures shoudl probably On 2014/08/04 23:38:53, aboxhall wrote: > nit: "shoudl" Done. https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:219: // Dispatches a single key with no flags. On 2014/08/04 23:38:53, aboxhall wrote: > Nit: this comment is clearly out of date. Done. https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1198: VLOG(0) << "delta time is: " << delta_time_ms; On 2014/08/04 23:58:09, aboxhall wrote: > logspam? Done. https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1211: // Most of the time this is 2 right now, but two of the two finger// On 2014/08/04 23:58:09, aboxhall wrote: > nit: trailing // Done. https://codereview.chromium.org/429633002/diff/40001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller_unittest.cc:1494: initial_press.x() + gesture_detector_config_.touch_slop + 1, 1); On 2014/08/04 23:58:09, aboxhall wrote: > I'm confused by this - this looks like it would move it off the right of the > screen? You're right, this should be in the y direction.
Events from gestures are now dispatched through a int (number of fingers) to closure (keys dispatched) mapping
This looks great, but what happened to the test? https://codereview.chromium.org/429633002/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:713: if (event_details.swipe_left()) This looks so good now! https://codereview.chromium.org/429633002/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:973: // vision users. However, but none of these mappings are permanent. nit: slightly awkward phrasing (however, but) https://codereview.chromium.org/429633002/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/429633002/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:346: void InitializeSwipeClosures(); I think the fact that they're closures is an implementation detail; I'd call this something like InitializeSwipeGestureMaps().
Tests are gone because it's actually a diff from the version without closures. You said there was a chance other reviewers wouldn't agree with your idea, and also I wanted to be able to continue with code review on the original if this took a while :) I can push the changes up to the original branch now if you'd like, now that we know it works. https://codereview.chromium.org/429633002/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:713: if (event_details.swipe_left()) On 2014/08/05 23:43:41, aboxhall wrote: > This looks so good now! Acknowledged. https://codereview.chromium.org/429633002/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.cc:973: // vision users. However, but none of these mappings are permanent. On 2014/08/05 23:43:41, aboxhall wrote: > nit: slightly awkward phrasing (however, but) Done. https://codereview.chromium.org/429633002/diff/80001/ui/chromeos/touch_explor... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/429633002/diff/80001/ui/chromeos/touch_explor... ui/chromeos/touch_exploration_controller.h:346: void InitializeSwipeClosures(); On 2014/08/05 23:43:42, aboxhall wrote: > I think the fact that they're closures is an implementation detail; I'd call > this something like InitializeSwipeGestureMaps(). Done.
On 2014/08/05 23:51:19, evy wrote: > Tests are gone because it's actually a diff from the version without closures. > You said there was a chance other reviewers wouldn't agree with your idea, and > also I wanted to be able to continue with code review on the original if this > took a while :) > > I can push the changes up to the original branch now if you'd like, now that we > know it works. Yeah, let's do that - you can always recover the old version from git history if need be.
On 2014/08/05 23:58:13, aboxhall wrote: > On 2014/08/05 23:51:19, evy wrote: > > Tests are gone because it's actually a diff from the version without closures. > > You said there was a chance other reviewers wouldn't agree with your idea, and > > also I wanted to be able to continue with code review on the original if this > > took a while :) > > > > I can push the changes up to the original branch now if you'd like, now that > we > > know it works. > > Yeah, let's do that - you can always recover the old version from git history if > need be. Done :)
https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:85: // Always process gesture events in the gesture provider. After a swipe Are you sure you want to do gesture recognition before updating current_touch_ids_? Is there a good reason to? This breaks the pattern... https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:89: gesture_provider_.OnTouchEvent(touch_event); Glad to see you are using GestureProviderAura for this. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:91: ProcessGestureEvents(); Would it be worth making this return a bool for consumed/not consumed, for state update/validation or an early exit? https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:91: ProcessGestureEvents(); Seems like you only want to call ProcessGestureEvents() in GESTURE_IN_PROGRESS state? Also, this entire block of code with gesture evaluation can probably be skipped in certain states like TOUCH_EXPLORATION. You can just create a new instance of the gesture provider if you need it again or when all fingers are lifted. Less state updating this way, so seems like it could be worth it. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:189: return InGestureInProgress(event, rewritten_event); Should we consider making 2-finger interactions subject to the same velocity/slop evaluation as one-finger interaction? It seems like currently the user cannot simply put 2 fingers down and start scrolling - they need to wait for the timeout to enter TE mode first? https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:385: if (type != ui::ET_TOUCH_RELEASED && type != ui::ET_TOUCH_CANCELLED && This is done in a bunch of different places in slightly different ways. Some In<State> methods are missing this check though, e.g. InTouchExploration. I'd just ditch this check altogether or perhaps move to RewriteEvent. If you really want to keep it for some reason - I'd make a method out of it and call it everywhere consistently. It doesn't have to return anything - it can just do a bunch DCHECK(), since we really don't expect it to ever evaluate to false. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:622: delete gesture_provider_.GetAndResetPendingGestures(); This seems wrong. You've already done GetAndResetPendingGestures() in ProcessGestureEvents() and you are iterating over the gestures now. Calling GetAndResetPendingGestures() probably will likely (almost?) always be a no-op. Instead you probably want to return a bool in this method for consumed/not consumed and cleanup appropriately in the calling method. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:633: void TouchExplorationController::ProcessGestureEvents() { return a bool? https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:647: OnGestureEvent(*i); break the loop if this returns true https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:713: if (event_details.swipe_left()) I'd check left_swipe_gestures_[num_fingers] to be safe. Same for other directions. Or even better - call abstracted callbacks on the delegate - see below. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:751: void TouchExplorationController::DispatchShiftSearchKeyEvent( Hmm.. I think it would be cleaner if TEC just called abstracted methods on its delegate (e.g. Swipe(Direction direction, int num_fingers)), and the DispatchXYZ methods for specific functionality lived in that delegate. This would let us remove quite a bit of code from TEC which is getting pretty big. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:61: // user holds down the corner of the screen until an earcon sounds, all What is the "earcon sound"? I don't see it mentioned anywhere else in the long version or the code comments. Also, do you have to keep holding the finger on the corner in order to stay in this mode? If you can release it - then how can you exit this mode? Perhaps this is worth elaborating on in the "long version" https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:100: // completing any more actions. "user must lift all fingers before completing any more actions" - can't they still interact with the content, e.g. just put two fingers down and start scrolling? Above we say "When two or more fingers are pressed initially, from then on the events are passed through, but with the initial finger removed".
https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:61: // user holds down the corner of the screen until an earcon sounds, all I think some of my changes might have accidentally gotten placed in evy's change list. The earcons have nothing to do with this CL. On 2014/08/06 16:11:49, mfomitchev wrote: > What is the "earcon sound"? I don't see it mentioned anywhere else in the long > version or the code comments. Also, do you have to keep holding the finger on > the corner in order to stay in this mode? If you can release it - then how can > you exit this mode? Perhaps this is worth elaborating on in the "long version"
This is an intermediate step, just for clarity of each part of the change, and feedback as I go. From here I need to address a few more of the comments, mainly: 1. Don't call ProcessGestureEvents all the time. I wrote a reply explaining why I decided to do it, and hopefully we can find an alternative. 2. Make abstract methods TEC can call on its delegate (e.g. Swipe(Direction direction, int num_fingers)) This is a totally different thing (that I'm going to have to look into a bit), so I'd like to fix the first part before I start on it. Thanks for all the great feedback Mikhail! https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:85: // Always process gesture events in the gesture provider. After a swipe On 2014/08/06 16:11:49, mfomitchev wrote: > Are you sure you want to do gesture recognition before updating > current_touch_ids_? Is there a good reason to? This breaks the pattern... We wanted to process every event, and checking the ids sometimes returns from RewriteEvent. This will probably be changed though, from your comments. See my question two comments down. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:89: gesture_provider_.OnTouchEvent(touch_event); On 2014/08/06 16:11:49, mfomitchev wrote: > Glad to see you are using GestureProviderAura for this. Acknowledged. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:91: ProcessGestureEvents(); On 2014/08/06 16:11:49, mfomitchev wrote: > Seems like you only want to call ProcessGestureEvents() in GESTURE_IN_PROGRESS > state? > > Also, this entire block of code with gesture evaluation can probably be skipped > in certain states like TOUCH_EXPLORATION. You can just create a new instance of > the gesture provider if you need it again or when all fingers are lifted. Less > state updating this way, so seems like it could be worth it. I put it here because if I only updated it when in GESTURE_IN_PROGRESS state the finger ids wouldn't reset, and if I did a 3 finger gesture then a 1 finger gesture, the 1 finger was processed as a 3 finger gesture. From what you've said here, it seems like we could just keep the same instance of the gesture provider, and process gesture events in GESTURE_IN_PROGRESS and then again in ResetToNoFingersDown - would this work? https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:189: return InGestureInProgress(event, rewritten_event); On 2014/08/06 16:11:49, mfomitchev wrote: > Should we consider making 2-finger interactions subject to the same > velocity/slop evaluation as one-finger interaction? It seems like currently the > user cannot simply put 2 fingers down and start scrolling - they need to wait > for the timeout to enter TE mode first? Actually, two fingers are only used for gestures now. From our survey feedback, users prefer multifinger gestures to passthrough mode. (we have passthrough implemented with a double tap hold, and also a corner passthrough mode Lisa will be committing soon) When the tap timer times out, the user just leaves gesture mode and enters a wait state for all the fingers to be lifted again. On a slightly related note, we're going to be adding that two fingers held stops the voice from speaking, so soon there will actually be a timer/slop check here soon. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:385: if (type != ui::ET_TOUCH_RELEASED && type != ui::ET_TOUCH_CANCELLED && On 2014/08/06 16:11:49, mfomitchev wrote: > This is done in a bunch of different places in slightly different ways. Some > In<State> methods are missing this check though, e.g. InTouchExploration. I'd > just ditch this check altogether or perhaps move to RewriteEvent. If you really > want to keep it for some reason - I'd make a method out of it and call it > everywhere consistently. It doesn't have to return anything - it can just do a > bunch DCHECK(), since we really don't expect it to ever evaluate to false. > InTouchExploration checks each of them separately - a few functions check at the top if it handles all events the same. If other touch events are ever added, it'd be good to have the NOTREACHED() catch it. I think I'll move the check to RewriteEvent - thanks for that idea. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:622: delete gesture_provider_.GetAndResetPendingGestures(); On 2014/08/06 16:11:49, mfomitchev wrote: > This seems wrong. You've already done GetAndResetPendingGestures() in > ProcessGestureEvents() and you are iterating over the gestures now. Calling > GetAndResetPendingGestures() probably will likely (almost?) always be a no-op. > Instead you probably want to return a bool in this method for consumed/not > consumed and cleanup appropriately in the calling method. I thought I had deleted these because we're always processing them. I'll clean that up. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:61: // user holds down the corner of the screen until an earcon sounds, all On 2014/08/06 16:15:10, lisayin wrote: > I think some of my changes might have accidentally gotten placed in evy's change > list. The earcons have nothing to do with this CL. > > On 2014/08/06 16:11:49, mfomitchev wrote: > > What is the "earcon sound"? I don't see it mentioned anywhere else in the long > > version or the code comments. Also, do you have to keep holding the finger on > > the corner in order to stay in this mode? If you can release it - then how can > > you exit this mode? Perhaps this is worth elaborating on in the "long version" > Whoops - thanks for catching that! https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:100: // completing any more actions. On 2014/08/06 16:11:50, mfomitchev wrote: > "user must lift all fingers before completing any more actions" - can't they > still interact with the content, e.g. just put two fingers down and start > scrolling? Above we say "When two or more fingers are pressed initially, from > then on the events are passed through, but with the initial finger removed". The first comment is right - the passthrough comment needs to be updated, sorry.
Events are now only processed through the gesture provider when needed. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:91: ProcessGestureEvents(); On 2014/08/06 18:46:12, evy wrote: > On 2014/08/06 16:11:49, mfomitchev wrote: > > Seems like you only want to call ProcessGestureEvents() in GESTURE_IN_PROGRESS > > state? > > > > Also, this entire block of code with gesture evaluation can probably be > skipped > > in certain states like TOUCH_EXPLORATION. You can just create a new instance > of > > the gesture provider if you need it again or when all fingers are lifted. Less > > state updating this way, so seems like it could be worth it. > > I put it here because if I only updated it when in GESTURE_IN_PROGRESS state the > finger ids wouldn't reset, and if I did a 3 finger gesture then a 1 finger > gesture, the 1 finger was processed as a 3 finger gesture. > From what you've said here, it seems like we could just keep the same instance > of the gesture provider, and process gesture events in GESTURE_IN_PROGRESS and > then again in ResetToNoFingersDown - would this work? > This is now right before the events are sent to be rewritten, and only happens if the user is in a state that might lead to gestures. I also reset the provider in ResetToNoFingersDown https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:633: void TouchExplorationController::ProcessGestureEvents() { On 2014/08/06 16:11:49, mfomitchev wrote: > return a bool? I'm doing it a bit differently - tell me what you think :) https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:647: OnGestureEvent(*i); On 2014/08/06 16:11:49, mfomitchev wrote: > break the loop if this returns true breaks the loop on a return if a swipe or slide gesture goes through
https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:577: return; This should be a break so that the mouse moves can be sent. https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:618: // fingers to be released. nit: move this line to the line with // all https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:90: // which will be interpreted and used to control the UI. One finger gestures Should this be multifinger gestures? https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1549: } nit: Extra space.
https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:577: return; On 2014/08/06 22:43:29, lisayin wrote: > This should be a break so that the mouse moves can be sent. Done. https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:618: // fingers to be released. On 2014/08/06 22:43:29, lisayin wrote: > nit: move this line to the line with // all Done. https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:90: // which will be interpreted and used to control the UI. One finger gestures On 2014/08/06 22:43:29, lisayin wrote: > Should this be multifinger gestures? Done. https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller_unittest.cc (right): https://codereview.chromium.org/429633002/diff/160001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller_unittest.cc:1549: } On 2014/08/06 22:43:29, lisayin wrote: > nit: Extra space. Done.
https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:85: // Always process gesture events in the gesture provider. After a swipe EVENT_REWRITE_CONTINUE should only be returned in that code if the controller was activated while there were fingers down. You wouldn't want events from those fingers go through gesture recognizer anyway, since it wasn't around to receive the PRESS events for them, so it would just ignore move/release from them (in the best case) or would end up i could end up in a bad state (in the worst case). I am not so sure about FindEdgesWithinBounds() stuff - not quite sure what is going on there... https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:189: return InGestureInProgress(event, rewritten_event); Ah, okay, this clears things up, thanks. https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:46: gesture_provider_.reset(new GestureProviderAura(this)); nit: I think you can just add gesture_provider_(new GestureProviderAura(this)) to initialization list (in place of gesture_provider_(this)) https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:96: if (current_touch_ids_.size() == 0) { Should this be == 1 instead? You shouldn't get a release if touch ids is empty (unless the controller was activated when some fingers were already down)... Also, does this really belong in the block of code updating current_touch_ids_? Perhaps it would be worth moving it out to keep separate logical pieces things separated? https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:130: if (state_ == NO_FINGERS_DOWN || Hmm.. perhaps it would be better to do gesture_provider_.reset(NULL) whenever transition to a state where you don't need a gesture recognizer anymore. You could add a SetState() method to ensure you always do that (then you can also ditch ResetToNoFingersDown()). One advantage is that you won't have gesture_provider_ evaluating things like long presses in the background when you no longer need it. Also, you reinforce the paradigm that once you go into a state where gestures aren't supported - you have to go through NO_FINGERS_DOWN to re-enable gestures. And then here you can simply check if (gesture_provider_.get()). https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:395: // will change to wait for no fingers down and this function will stop "the state will change to wait for no fingers down" - could change to exploration mode as well if only one finger is down, right? https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:601: // This is an override. We're using other more specific functions for possible Would be good to clarify that this is only called for timer-based events like long press. Events that are created synchronously as a result of certain touch events are added to the vector accessible via GetAndResetPendingGestures(). We only care about swipes (which are created synchronously), so we ignore this callback. https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:625: return; Are you sure we want to ignore all the subsequent gestures (if any) in this case? I'd think we'd want to process all scrolls? https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:633: CHECK(gesture->IsGestureEvent()); I don't think this can ever fail since gesture is an instance of GestureEvent? https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:634: CHECK(gesture->IsScrollGestureEvent()); If you do this, I don't think you need the above? Also, why not DCHECK instead of CHECK? CHECKs waste CPU time in prod. https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:684: CHECK(swipe_gesture->IsGestureEvent()); I don't think this can ever fail since gesture is an instance of GestureEvent?
Thanks for the great ideas Mikhail How does it look now? https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:85: // Always process gesture events in the gesture provider. After a swipe On 2014/08/07 18:34:58, mfomitchev wrote: > EVENT_REWRITE_CONTINUE should only be returned in that code if the controller > was activated while there were fingers down. You wouldn't want events from those > fingers go through gesture recognizer anyway, since it wasn't around to receive > the PRESS events for them, so it would just ignore move/release from them (in > the best case) or would end up i > could end up in a bad state (in the worst case). > > I am not so sure about FindEdgesWithinBounds() stuff - not quite sure what is > going on there... which EVENT_REWRITE_CONTINUE are you talking about? The FindEdgesWithinBounds() stuff was written before we started, and I'm not quite sure what the logic behind it is - I'll make a note to ask Dominic next week and get back to you. https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:46: gesture_provider_.reset(new GestureProviderAura(this)); On 2014/08/07 18:34:58, mfomitchev wrote: > nit: I think you can just add gesture_provider_(new GestureProviderAura(this)) > to initialization list (in place of gesture_provider_(this)) Done. https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:96: if (current_touch_ids_.size() == 0) { On 2014/08/07 18:34:58, mfomitchev wrote: > Should this be == 1 instead? You shouldn't get a release if touch ids is empty > (unless the controller was activated when some fingers were already down)... > > Also, does this really belong in the block of code updating current_touch_ids_? > Perhaps it would be worth moving it out to keep separate logical pieces things > separated? This block of code was actually here before we started. I don't even see where the touch id is removed from current_touch_ids, but my guess is that it was supposed to be updated before that size check. I'll look into it, and ask Dominic next week if I can't find a clear answer. https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:130: if (state_ == NO_FINGERS_DOWN || On 2014/08/07 18:34:58, mfomitchev wrote: > Hmm.. perhaps it would be better to do gesture_provider_.reset(NULL) whenever > transition to a state where you don't need a gesture recognizer anymore. You > could add a SetState() method to ensure you always do that (then you can also > ditch ResetToNoFingersDown()). One advantage is that you won't have > gesture_provider_ evaluating things like long presses in the background when you > no longer need it. Also, you reinforce the paradigm that once you go into a > state where gestures aren't supported - you have to go through NO_FINGERS_DOWN > to re-enable gestures. And then here you can simply check if > (gesture_provider_.get()). Done. Great idea - feels much cleaner this way. https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:395: // will change to wait for no fingers down and this function will stop On 2014/08/07 18:34:59, mfomitchev wrote: > "the state will change to wait for no fingers down" - could change to > exploration mode as well if only one finger is down, right? yup - thanks for catching that https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:601: // This is an override. We're using other more specific functions for possible On 2014/08/07 18:34:58, mfomitchev wrote: > Would be good to clarify that this is only called for timer-based events like > long press. Events that are created synchronously as a result of certain touch > events are added to the vector accessible via GetAndResetPendingGestures(). We > only care about swipes (which are created synchronously), so we ignore this > callback. Done. https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:625: return; On 2014/08/07 18:34:58, mfomitchev wrote: > Are you sure we want to ignore all the subsequent gestures (if any) in this > case? I'd think we'd want to process all scrolls? yeah, we would - thanks https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:633: CHECK(gesture->IsGestureEvent()); On 2014/08/07 18:34:58, mfomitchev wrote: > I don't think this can ever fail since gesture is an instance of GestureEvent? Done. https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:634: CHECK(gesture->IsScrollGestureEvent()); On 2014/08/07 18:34:58, mfomitchev wrote: > If you do this, I don't think you need the above? Also, why not DCHECK instead > of CHECK? CHECKs waste CPU time in prod. Done. https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:684: CHECK(swipe_gesture->IsGestureEvent()); On 2014/08/07 18:34:58, mfomitchev wrote: > I don't think this can ever fail since gesture is an instance of GestureEvent? Yeah there are a few of these checks around - I'll remove them
https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:85: // Always process gesture events in the gesture provider. After a swipe On 2014/08/07 20:40:26, evy wrote: > On 2014/08/07 18:34:58, mfomitchev wrote: > > EVENT_REWRITE_CONTINUE should only be returned in that code if the controller > > was activated while there were fingers down. You wouldn't want events from > those > > fingers go through gesture recognizer anyway, since it wasn't around to > receive > > the PRESS events for them, so it would just ignore move/release from them (in > > the best case) or would end up i > > could end up in a bad state (in the worst case). > > > > I am not so sure about FindEdgesWithinBounds() stuff - not quite sure what is > > going on there... > > which EVENT_REWRITE_CONTINUE are you talking about? > The FindEdgesWithinBounds() stuff was written before we started, and I'm not > quite sure what the logic behind it is - I'll make a note to ask Dominic next > week and get back to you. IIRC, lisayin@ wrote the FindEdgesWithinBounds logic. It's pretty straightforward: it checks whether it's within |bounds| DIPs of one or more edges, and returns a bitmap indicating which edges it's near. I'm sure she'd be able to explain in more detail if you had any further questions. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:236: NOTREACHED() << "Unexpected event type received: " << event.name(); What happened to these useful logs?
https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:85: // Always process gesture events in the gesture provider. After a swipe There's some conversion to DIPS within the actual implementation details because event locations aren't in DIPS but the window dimensions are, but other than that it's pretty much exactly what Alice described it to be. FindEdgesWithinBounds takes a location and determines if the location is within the given bounds of an edge. It shouldn't affect Evy's code though. In this iteration, bounds are to keep people from being able to make a single tap by moving off the screen and back onto the screen without lifting their finger. On 2014/08/07 20:53:21, aboxhall wrote: > On 2014/08/07 20:40:26, evy wrote: > > On 2014/08/07 18:34:58, mfomitchev wrote: > > > EVENT_REWRITE_CONTINUE should only be returned in that code if the > controller > > > was activated while there were fingers down. You wouldn't want events from > > those > > > fingers go through gesture recognizer anyway, since it wasn't around to > > receive > > > the PRESS events for them, so it would just ignore move/release from them > (in > > > the best case) or would end up i > > > could end up in a bad state (in the worst case). > > > > > > I am not so sure about FindEdgesWithinBounds() stuff - not quite sure what > is > > > going on there... > > > > which EVENT_REWRITE_CONTINUE are you talking about? > > The FindEdgesWithinBounds() stuff was written before we started, and I'm not > > quite sure what the logic behind it is - I'll make a note to ask Dominic next > > week and get back to you. > > IIRC, lisayin@ wrote the FindEdgesWithinBounds logic. It's pretty > straightforward: it checks whether it's within |bounds| DIPs of one or more > edges, and returns a bitmap indicating which edges it's near. > > I'm sure she'd be able to explain in more detail if you had any further > questions.
https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:236: NOTREACHED() << "Unexpected event type received: " << event.name(); On 2014/08/07 20:53:21, aboxhall wrote: > What happened to these useful logs? They're all in RewriteEvent now, before event sending an event to any of these functions. I kept the NOTREACHED() just in case this function is somehow called through another means where the touch event could be wild - should I keep the logs here too?
https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:85: // Always process gesture events in the gesture provider. After a swipe >which EVENT_REWRITE_CONTINUE are you talking about? @evy: You said "checking the ids sometimes returns from RewriteEvent". There are two returns, they return EVENT_REWRITE_CONTINUE FindEdgesWithinBounds stuff: Ok, so I think it would be cleaner to move FindEdgesWithinBounds logic out of that code block. Logically that entire code block was supposed to be for updating current_touch_ids_. It's easy to reason about it that way - whether it needs to be before or after gesture detection, etc. It should probably even be moved to a separate method. If we insert FindEdgesWithinBounds in there, that block becomes harder to reason about. https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:96: if (current_touch_ids_.size() == 0) { Sounds like lisayin@ wrote this, maybe ask her? The touch id is removed just a few lines below - "current_touch_ids_.erase(it);" Incidentally, if this code is moved out of the current_touch_ids_ update block, I think it becomes correct :) https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:392: // will change to wait for no fingers down and this function will stop It still sounds a bit confusing/contradicting How about something like "the state will change to "wait for no fingers down" or "touch exploration" depending on the number of fingers down, and this function will stop being called." https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:621: VLOG_STATE(); Put VLOG_STATE() into SetState() and get rid of it elsewhere? https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:635: delegate_->PlayVolumeAdjustSound(); This is not your CL, but I'd keep the delegate's methods names focused on gestures unless there's a good reason not to. E.g. SlideScrollBegin(), SlideScrollEnd(), and SlideScroll(float ratio) or something along these lines, and then keep the logic for handling these (i.e. volume changes) entirely within the delegate. Maybe add a TODO here. https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:815: else if (gesture_provider_.get() && I'd recommend putting a switch(state_) here with no default clause. This way if a new state is added, we will get a compile error if we don't update this block - which would be a good thing.
https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:713: if (event_details.swipe_left()) On 2014/08/06 16:11:49, mfomitchev wrote: > I'd check left_swipe_gestures_[num_fingers] to be safe. Same for other > directions. Or even better - call abstracted callbacks on the delegate - see > below. I did a check for is_null() - let me know if this isn't the right function to do this check. https://codereview.chromium.org/429633002/diff/120001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:751: void TouchExplorationController::DispatchShiftSearchKeyEvent( On 2014/08/06 16:11:49, mfomitchev wrote: > Hmm.. I think it would be cleaner if TEC just called abstracted methods on its > delegate (e.g. Swipe(Direction direction, int num_fingers)), and the DispatchXYZ > methods for specific functionality lived in that delegate. This would let us > remove quite a bit of code from TEC which is getting pretty big. This would be awesome - our team has talked about this a bit already. I'll submit this changelist first, add a TODO, and see if I can get it working next week. https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/180001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:96: if (current_touch_ids_.size() == 0) { On 2014/08/08 20:23:13, mfomitchev wrote: > Sounds like lisayin@ wrote this, maybe ask her? The touch id is removed just a > few lines below - "current_touch_ids_.erase(it);" > > Incidentally, if this code is moved out of the current_touch_ids_ update block, > I think it becomes correct :) Done. https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:392: // will change to wait for no fingers down and this function will stop On 2014/08/08 20:23:13, mfomitchev wrote: > It still sounds a bit confusing/contradicting > How about something like "the state will change to "wait for no fingers down" or > "touch exploration" depending on the number of fingers down, and this function > will stop being called." Done. https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:621: VLOG_STATE(); On 2014/08/08 20:23:13, mfomitchev wrote: > Put VLOG_STATE() into SetState() and get rid of it elsewhere? It logs the function it's called from - which has been really helpful to us - and moving it into SetState would remove that... actually, I'll send the function name to set state and then from there to VlogState. https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:635: delegate_->PlayVolumeAdjustSound(); On 2014/08/08 20:23:13, mfomitchev wrote: > This is not your CL, but I'd keep the delegate's methods names focused on > gestures unless there's a good reason not to. E.g. SlideScrollBegin(), > SlideScrollEnd(), and SlideScroll(float ratio) or something along these lines, > and then keep the logic for handling these (i.e. volume changes) entirely within > the delegate. Maybe add a TODO here. Lisa is doing it now :) https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:815: else if (gesture_provider_.get() && On 2014/08/08 20:23:13, mfomitchev wrote: > I'd recommend putting a switch(state_) here with no default clause. This way if > a new state is added, we will get a compile error if we don't update this block > - which would be a good thing. Awesome.
lgtm https://codereview.chromium.org/429633002/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:802: // These are the states the user can be in that will never result in a There isn't supposed to be anything between the closing } and the else...put the comments inside the else block instead. https://codereview.chromium.org/429633002/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:906: base::Bind(&TouchExplorationController::DispatchShiftSearchKeyEvent, I think you could create a helper function BindKeyEvent() that takes a key and calls base::Bind(&TouchExplorationController::DispatchShiftSearchKeyEvent, base::Unretained(this), key) - that would make this function a lot more readable. https://codereview.chromium.org/429633002/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/429633002/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:403: // Maps swipes with the resulting functions that dispatch key events. Please document more clearly that the "int" argument is the number of fingers.
lgtm with a nit
https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:621: VLOG_STATE(); Yup, take a look at FROM_HERE in base/location.h
https://codereview.chromium.org/429633002/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:796: if(new_state == NO_FINGERS_DOWN){ You make switch() top-level, put NO_FINGERS_DOWN handling into the switch, and do the if check for gesture_provider_.get() under the cases
https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/200001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:621: VLOG_STATE(); On 2014/08/11 14:56:32, mfomitchev wrote: > Yup, take a look at FROM_HERE in base/location.h I think that's along the lines of what we were using :) https://codereview.chromium.org/429633002/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.cc (right): https://codereview.chromium.org/429633002/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:796: if(new_state == NO_FINGERS_DOWN){ On 2014/08/11 15:05:08, mfomitchev wrote: > You make switch() top-level, put NO_FINGERS_DOWN handling into the switch, and > do the if check for gesture_provider_.get() under the cases Done. https://codereview.chromium.org/429633002/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:802: // These are the states the user can be in that will never result in a On 2014/08/11 08:24:55, dmazzoni wrote: > There isn't supposed to be anything between the closing } and the else...put the > comments inside the else block instead. Done. https://codereview.chromium.org/429633002/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.cc:906: base::Bind(&TouchExplorationController::DispatchShiftSearchKeyEvent, On 2014/08/11 08:24:56, dmazzoni wrote: > I think you could create a helper function BindKeyEvent() that takes a key and > calls base::Bind(&TouchExplorationController::DispatchShiftSearchKeyEvent, > base::Unretained(this), key) - that would make this function a lot more > readable. > > Done. https://codereview.chromium.org/429633002/diff/220001/ui/chromeos/touch_explo... File ui/chromeos/touch_exploration_controller.h (right): https://codereview.chromium.org/429633002/diff/220001/ui/chromeos/touch_explo... ui/chromeos/touch_exploration_controller.h:403: // Maps swipes with the resulting functions that dispatch key events. On 2014/08/11 08:24:56, dmazzoni wrote: > Please document more clearly that the "int" argument is the number of fingers. Done.
The CQ bit was checked by evy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/evy@chromium.org/429633002/280001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
The CQ bit was checked by evy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/evy@chromium.org/429633002/280001
Message was sent while issue was closed.
Change committed as 288857
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/466653002/ by tapted@chromium.org. The reason for reverting is: tree-closer ui_unittests failing on Linux Chromium OS ASan LSan Tests (3) test TouchExplorationTest.AllFingerPermutations failing since http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20... I don't think the failing test itself is generating output, but there are some LOG(INFO)s that have been snuck through with the VLOG(0) trick which might help diagnose..
The CQ bit was checked by evy@chromium.org
The CQ bit was unchecked by evy@chromium.org
The CQ bit was checked by evy@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/evy@chromium.org/429633002/400001
Message was sent while issue was closed.
Committed patchset #16 (400001) as 289334 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
