Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
590483002:
Check if Button is pressed for changing selection (Closed)
DescriptionCheck if Button is pressed for changing selection
Adding flags to check if SPen button is pressed
when gesture text selection is in progress.
The selection should be updated only when the
button is pressed.
BUG=416004
Committed: https://crrev.com/169b875abb32975d8374866e333bf6f477f6c182
Cr-Commit-Position: refs/heads/master@{#299149}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Updated according to review + Unittest #Patch Set 3 : Check only for Secondary Button #
Total comments: 3
Patch Set 4 : Updated as per review and unittests updated #
Total comments: 6
Patch Set 5 : updated according to review comments #Patch Set 6 : adding spaces in log #
Total comments: 2
Patch Set 7 : Review comments #
Messages
Total messages: 18 (2 generated)
avi.nitk@samsung.com changed reviewers: + changwan@chromium.org, jdduke@chromium.org
@jdduke, @ChangwanRyu Could you please take a look?
https://codereview.chromium.org/590483002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/gesture_text_selector.cc (right): https://codereview.chromium.org/590483002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/gesture_text_selector.cc:16: spen_button_pressed_(false), We're trying to keep this class generic, for any kind of stylus input so I don't think we should have any SPen-specific terms. https://codereview.chromium.org/590483002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/gesture_text_selector.cc:32: spen_button_pressed_ = ShouldStartTextSelection(event); Reusing this function with its current name is confusing. I think you really only need to check for the secondary button state here.
@jdduke Thanks for the comments. Could you PTAL? https://codereview.chromium.org/590483002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/gesture_text_selector.cc (right): https://codereview.chromium.org/590483002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/gesture_text_selector.cc:16: spen_button_pressed_(false), On 2014/09/19 17:34:22, jdduke wrote: > We're trying to keep this class generic, for any kind of stylus input so I don't > think we should have any SPen-specific terms. renamed to secondary_button_pressed_ https://codereview.chromium.org/590483002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/gesture_text_selector.cc:32: spen_button_pressed_ = ShouldStartTextSelection(event); On 2014/09/19 17:34:22, jdduke wrote: > Reusing this function with its current name is confusing. I think you really > only need to check for the secondary button state here. Changed to check only Button state.
https://codereview.chromium.org/590483002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/gesture_text_selector.cc (right): https://codereview.chromium.org/590483002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/gesture_text_selector.cc:57: if (secondary_button_pressed_) { So, what happens when the user repeatedly presses/releases the button? We don't adjust the anchor, so it seems like the selection will "jump" to the new stylus location? Is that the intended result? Ideally we would match whatever the native behavior is, e.g., should pressing the button again trigger a *new* selection region instead of extending the previous one? https://codereview.chromium.org/590483002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/gesture_text_selector_unittest.cc (right): https://codereview.chromium.org/590483002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/gesture_text_selector_unittest.cc:238: EXPECT_EQ(3u, event_log_.size()); // NO CHANGE What if they press the button again?
Actually, there are 4 possible ways to handle this condition : 1. To end selection on Button Release, and ignore every scroll and move event after that.This is irrespective of whether the button is pressed again or not, until pen is lifted. This is the behavior currently in Note 4 for SPen based text selection on any android EditField. 2. To end the selection on Button Release, and pass the subsequent move events forward, so that page scroll happens on move (which is the normal operation of stylus without button press). If the Button is pressed again, trigger a new selection. 3. To end selection on Button Release, and not do anything on subsequent move event. Trigger a new selection when button is pressed again and dragged. This is the ideal behavior and is closely aligned to general mouse selection behavior also. 4. To continue selection even after Button Release, so that when button is pressed again, selection is extended to the point where button is released. Which is the behavior observed with the code change here. If you think it is okay to go ahead with 3, I will make appropriate changes. https://codereview.chromium.org/590483002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/gesture_text_selector.cc (right): https://codereview.chromium.org/590483002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/gesture_text_selector.cc:57: if (secondary_button_pressed_) { On 2014/09/23 15:10:48, jdduke wrote: > So, what happens when the user repeatedly presses/releases the button? We don't > adjust the anchor, so it seems like the selection will "jump" to the new stylus > location? Is that the intended result? Ideally we would match whatever the > native behavior is, e.g., should pressing the button again trigger a *new* > selection region instead of extending the previous one? Yes, you are right. Triggering a new selection in this case is the ideal behavior in case of using Stylus as mouse.
On 2014/09/23 16:01:04, AviD wrote: > 3. To end selection on Button Release, and not do anything on subsequent move > event. Trigger a new selection when button is pressed again and dragged. This is > the ideal behavior and is closely aligned to general mouse selection behavior > also. Yup, I agree that this seems like the most intuitive behavior. Let's verify with changwan@ that this is acceptable/desirable.
On 2014/09/23 16:06:49, jdduke wrote: > On 2014/09/23 16:01:04, AviD wrote: > > 3. To end selection on Button Release, and not do anything on subsequent move > > event. Trigger a new selection when button is pressed again and dragged. This > is > > the ideal behavior and is closely aligned to general mouse selection behavior > > also. > > Yup, I agree that this seems like the most intuitive behavior. Let's verify with > changwan@ that this is acceptable/desirable. @ChangwanRyu: Could you please let us know your views? I have a patch ready for this behavior. If it is ok, I will upload the same.
On 2014/09/29 15:54:41, AviD wrote: > On 2014/09/23 16:06:49, jdduke wrote: > > On 2014/09/23 16:01:04, AviD wrote: > > > 3. To end selection on Button Release, and not do anything on subsequent > move > > > event. Trigger a new selection when button is pressed again and dragged. > This > > is > > > the ideal behavior and is closely aligned to general mouse selection > behavior > > > also. > > > > Yup, I agree that this seems like the most intuitive behavior. Let's verify > with > > changwan@ that this is acceptable/desirable. > > @ChangwanRyu: Could you please let us know your views? I have a patch ready for > this behavior. If it is ok, I will upload the same. I agree that 3) sounds most reasonable. Please go ahead.
Thanks, this is just about there. https://codereview.chromium.org/590483002/diff/60001/content/browser/renderer... File content/browser/renderer_host/input/gesture_text_selector.cc (right): https://codereview.chromium.org/590483002/diff/60001/content/browser/renderer... content/browser/renderer_host/input/gesture_text_selector.cc:51: secondary_button_pressed_ = text_selection_triggered_; Let's go ahead and do the BUTTON_SECONDARY check here explicitly, even though it seems redundant with |ShouldStartTextSelection()|. That function may change in the future. https://codereview.chromium.org/590483002/diff/60001/content/browser/renderer... File content/browser/renderer_host/input/gesture_text_selector.h (right): https://codereview.chromium.org/590483002/diff/60001/content/browser/renderer... content/browser/renderer_host/input/gesture_text_selector.h:63: int anchor_x_; These should be floats. https://codereview.chromium.org/590483002/diff/60001/content/browser/renderer... File content/browser/renderer_host/input/gesture_text_selector_unittest.cc (right): https://codereview.chromium.org/590483002/diff/60001/content/browser/renderer... content/browser/renderer_host/input/gesture_text_selector_unittest.cc:178: EXPECT_STREQ("SelectRange", event_log_.back().c_str()); Ideally we could check the actual SelectRange coordinates. Maybe we can adjust the logging code to include the coordinates in the string (as integers for simplicity), so the check string would look something like: "SelectRange(90, 70, 110, 90)" (those numbers might be off).
@jdduke: Thanks for your review. Could you please take another look? https://codereview.chromium.org/590483002/diff/60001/content/browser/renderer... File content/browser/renderer_host/input/gesture_text_selector.cc (right): https://codereview.chromium.org/590483002/diff/60001/content/browser/renderer... content/browser/renderer_host/input/gesture_text_selector.cc:51: secondary_button_pressed_ = text_selection_triggered_; On 2014/10/10 14:37:33, jdduke wrote: > Let's go ahead and do the BUTTON_SECONDARY check here explicitly, even though it > seems redundant with |ShouldStartTextSelection()|. That function may change in > the future. Done. https://codereview.chromium.org/590483002/diff/60001/content/browser/renderer... File content/browser/renderer_host/input/gesture_text_selector.h (right): https://codereview.chromium.org/590483002/diff/60001/content/browser/renderer... content/browser/renderer_host/input/gesture_text_selector.h:63: int anchor_x_; On 2014/10/10 14:37:33, jdduke wrote: > These should be floats. Done. https://codereview.chromium.org/590483002/diff/60001/content/browser/renderer... File content/browser/renderer_host/input/gesture_text_selector_unittest.cc (right): https://codereview.chromium.org/590483002/diff/60001/content/browser/renderer... content/browser/renderer_host/input/gesture_text_selector_unittest.cc:178: EXPECT_STREQ("SelectRange", event_log_.back().c_str()); On 2014/10/10 14:37:33, jdduke wrote: > Ideally we could check the actual SelectRange coordinates. Maybe we can adjust > the logging code to include the coordinates in the string (as integers for > simplicity), so the check string would look something like: "SelectRange(90, 70, > 110, 90)" (those numbers might be off). I have added a function to convert the float values to string. I'm not sure if thats the correct way of doing it. Somehow, the std::to_string causes build error.
lgtm with nit. https://codereview.chromium.org/590483002/diff/170001/content/browser/rendere... File content/browser/renderer_host/input/gesture_text_selector_unittest.cc (right): https://codereview.chromium.org/590483002/diff/170001/content/browser/rendere... content/browser/renderer_host/input/gesture_text_selector_unittest.cc:46: event_log_.push_back("SelectRange(" + to_string(x1) + ", " + to_string(y1) + Nit: No need for a helper function here, just do: std::stringstream ss; ss << "SelectRange(" << x1 << ", " << y1 << ", " << x2 << ", " << y2 << ")"; events_log_.push_back(ss.str());
https://codereview.chromium.org/590483002/diff/170001/content/browser/rendere... File content/browser/renderer_host/input/gesture_text_selector.cc (right): https://codereview.chromium.org/590483002/diff/170001/content/browser/rendere... content/browser/renderer_host/input/gesture_text_selector.cc:40: DCHECK(client); Also initialize anchor_x_/anchor_y_ in the constructor initializer list.
On 2014/10/10 16:23:21, jdduke wrote: > https://codereview.chromium.org/590483002/diff/170001/content/browser/rendere... > File content/browser/renderer_host/input/gesture_text_selector.cc (right): > > https://codereview.chromium.org/590483002/diff/170001/content/browser/rendere... > content/browser/renderer_host/input/gesture_text_selector.cc:40: DCHECK(client); > Also initialize anchor_x_/anchor_y_ in the constructor initializer list. Thanks.
The CQ bit was checked by avi.nitk@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/590483002/240001
Message was sent while issue was closed.
Committed patchset #7 (id:240001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/169b875abb32975d8374866e333bf6f477f6c182 Cr-Commit-Position: refs/heads/master@{#299149} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
