Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1127133003:
Adjusts dragging logic to be less likely to trigger false positive switch from snapping to docking (Closed)
DescriptionAdjusts dragging logic to be less likely to trigger false positive switch from
snapping to docking.
This CL adjusts the logic which selects whether a window will be snapped to
half of the workspace width or will be docked at the end of the drag when the
window is dragged to the edge of the workspace in the special case of the dock
being visible. The CL makes it easier to snap a window to half the workspace
width in this special case (and harder to inadvertently dock the window)
BUG=484877
TEST=None
Committed: https://crrev.com/988c697aff35b3fd347dec35f2f2ac159c8328a9
Cr-Commit-Position: refs/heads/master@{#329268}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Adjusts docking dragging logic to be more tolerant (comments) #Patch Set 3 : Adjusts docking dragging logic to be more tolerant (adjustment to allow touch) #
Total comments: 5
Patch Set 4 : Adjusts docking dragging logic to be more tolerant (optimization) #
Total comments: 8
Patch Set 5 : Adjusts docking dragging logic to be more tolerant (nits) #
Messages
Total messages: 32 (11 generated)
varkha@chromium.org changed reviewers: + pkotwicz@chromium.org
pkotwicz@, can you please take a look - see if you think this logic can solve what they had difficulty with in the bug? It is mostly resetting some of the logic after a pause and demands movement in preferred direction rather than any movement as before. Thanks!
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127133003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mostly looks good. Can you make the CL description more descriptive? It has been a while since I looked at the docking code and it took me a while to figure out what the change in behavior was when no window is docked and what the change in behavior is when a window is docked. https://codereview.chromium.org/1127133003/diff/1/ash/wm/workspace/two_step_e... File ash/wm/workspace/two_step_edge_cycler.cc (right): https://codereview.chromium.org/1127133003/diff/1/ash/wm/workspace/two_step_e... ash/wm/workspace/two_step_edge_cycler.cc:17: // . The mouse is moved |kMaxMoves| times. Can you please update this comment? https://codereview.chromium.org/1127133003/diff/1/ash/wm/workspace/two_step_e... File ash/wm/workspace/two_step_edge_cycler.h (right): https://codereview.chromium.org/1127133003/diff/1/ash/wm/workspace/two_step_e... ash/wm/workspace/two_step_edge_cycler.h:52: SnapType snap_type_; Can we use a different enum? I find it kind of ugly to use the enum from WorkspaceWindowResizer. (Especially since we do not care for the SNAP_NONE value.
Thanks! PTAL. https://codereview.chromium.org/1127133003/diff/1/ash/wm/workspace/two_step_e... File ash/wm/workspace/two_step_edge_cycler.cc (right): https://codereview.chromium.org/1127133003/diff/1/ash/wm/workspace/two_step_e... ash/wm/workspace/two_step_edge_cycler.cc:17: // . The mouse is moved |kMaxMoves| times. On 2015/05/07 17:10:28, pkotwicz wrote: > Can you please update this comment? Done. https://codereview.chromium.org/1127133003/diff/1/ash/wm/workspace/two_step_e... File ash/wm/workspace/two_step_edge_cycler.h (right): https://codereview.chromium.org/1127133003/diff/1/ash/wm/workspace/two_step_e... ash/wm/workspace/two_step_edge_cycler.h:52: SnapType snap_type_; On 2015/05/07 17:10:28, pkotwicz wrote: > Can we use a different enum? I find it kind of ugly to use the enum from > WorkspaceWindowResizer. (Especially since we do not care for the SNAP_NONE > value. Done.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127133003/20001
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127133003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
pkotwicz@, ping?
Sorry, I was on vacation. I think that this CL makes it harder to get into docked mode when the dock is not visible. (Now it is necessary to move the cursor vertically a bit in order to get to docked mode when the dock is not visible) I have modified the CL description to better reflect what I think this CL is doing. I think the goal of the CL is to make it easier to enter the 50% snapped mode when the dock is visible. I added a suggestion. It feels like we should always check the direction for clarity (even in the cases where doing so does not matter) https://codereview.chromium.org/1127133003/diff/40001/ash/wm/workspace/two_st... File ash/wm/workspace/two_step_edge_cycler.cc (right): https://codereview.chromium.org/1127133003/diff/40001/ash/wm/workspace/two_st... ash/wm/workspace/two_step_edge_cycler.cc:16: // |kMaxPixelsAfterPause| (horizontal distance). Nit: "(horizontal distance)" -> "horizontal pixels" https://codereview.chromium.org/1127133003/diff/40001/ash/wm/workspace/two_st... ash/wm/workspace/two_step_edge_cycler.cc:45: I would be tempted to do something like this: if ((base::TimeTicks::Now() - time_last_move_).InMilliseconds() > kMaxDelay) { ... } time_last_move_ = base::TimeTicks::Now(); int compare_x = paused_ ? paused_x_ : start_x_; if (location.x() != compare_x && (location.x() < compare_x) != (direction == DIRECTION_LEFT) { return; } if (location.x() != compare_x) ++num_moves_; bool moved_in_the_same_direction_after_pause =...
Sorry, I was on vacation. I think that this CL makes it harder to get into docked mode when the dock is not visible. (Now it is necessary to move the cursor vertically a bit in order to get to docked mode when the dock is not visible) I have modified the CL description to better reflect what I think this CL is doing. I think the goal of the CL is to make it easier to enter the 50% snapped mode when the dock is visible. I added a suggestion. It feels like we should always check the direction for clarity (even in the cases where doing so does not matter)
https://codereview.chromium.org/1127133003/diff/40001/ash/wm/workspace/two_st... File ash/wm/workspace/two_step_edge_cycler.cc (right): https://codereview.chromium.org/1127133003/diff/40001/ash/wm/workspace/two_st... ash/wm/workspace/two_step_edge_cycler.cc:45: Correction: I would be tempted to do something like this: if ((base::TimeTicks::Now() - time_last_move_).InMilliseconds() > kMaxDelay) { ... } time_last_move_ = base::TimeTicks::Now(); int compare_x = paused_ ? paused_x_ : start_x_; if (location.x() != compare_x && (location.x() < compare_x) != (direction == DIRECTION_LEFT) { return; } ++num_moves_; bool moved_in_the_same_direction_after_pause =...
Thanks for a suggestion, it also makes the code a bit more optimal in some cases! https://codereview.chromium.org/1127133003/diff/40001/ash/wm/workspace/two_st... File ash/wm/workspace/two_step_edge_cycler.cc (right): https://codereview.chromium.org/1127133003/diff/40001/ash/wm/workspace/two_st... ash/wm/workspace/two_step_edge_cycler.cc:16: // |kMaxPixelsAfterPause| (horizontal distance). On 2015/05/11 17:32:41, pkotwicz wrote: > Nit: "(horizontal distance)" -> "horizontal pixels" Done. https://codereview.chromium.org/1127133003/diff/40001/ash/wm/workspace/two_st... ash/wm/workspace/two_step_edge_cycler.cc:45: On 2015/05/11 17:35:06, pkotwicz wrote: > Correction: > > I would be tempted to do something like this: > > if ((base::TimeTicks::Now() - time_last_move_).InMilliseconds() > kMaxDelay) { > ... > } > > time_last_move_ = base::TimeTicks::Now(); > > int compare_x = paused_ ? paused_x_ : start_x_; > if (location.x() != compare_x && > (location.x() < compare_x) != (direction == DIRECTION_LEFT) { > return; > } > > ++num_moves_; > > bool moved_in_the_same_direction_after_pause =... > Done.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127133003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nits https://codereview.chromium.org/1127133003/diff/60001/ash/wm/workspace/two_st... File ash/wm/workspace/two_step_edge_cycler.cc (right): https://codereview.chromium.org/1127133003/diff/60001/ash/wm/workspace/two_st... ash/wm/workspace/two_step_edge_cycler.cc:20: // direction. Nit: Can you remove the comments about the movement being in the preferred direction since the movement must be in the preferred direction in all cases due to the early return on line 56 https://codereview.chromium.org/1127133003/diff/60001/ash/wm/workspace/two_st... ash/wm/workspace/two_step_edge_cycler.cc:64: (paused_x_ - location.x()) >= kMaxPixelsAfterPause)); Can't this become simpler now? i.e. std::abs(location.x() - paused_x_) >= kMaxPixelsAfterPause) https://codereview.chromium.org/1127133003/diff/60001/ash/wm/workspace/two_st... File ash/wm/workspace/two_step_edge_cycler.h (right): https://codereview.chromium.org/1127133003/diff/60001/ash/wm/workspace/two_st... ash/wm/workspace/two_step_edge_cycler.h:22: // The direction in which a mouse should travel after a pause to switch mode. Nit: Can you remove the "after a pause" part of the comment because it is no longer true? https://codereview.chromium.org/1127133003/diff/60001/ash/wm/workspace/two_st... ash/wm/workspace/two_step_edge_cycler.h:53: // Determines a movement direction that we are watching after a pause. Can you please update this comment?
Patchset #5 (id:2) has been deleted
Thanks, makes it easier to understand! https://codereview.chromium.org/1127133003/diff/60001/ash/wm/workspace/two_st... File ash/wm/workspace/two_step_edge_cycler.cc (right): https://codereview.chromium.org/1127133003/diff/60001/ash/wm/workspace/two_st... ash/wm/workspace/two_step_edge_cycler.cc:20: // direction. On 2015/05/11 21:35:20, pkotwicz wrote: > Nit: Can you remove the comments about the movement being in the preferred > direction since the movement must be in the preferred direction in all cases due > to the early return on line 56 Done. https://codereview.chromium.org/1127133003/diff/60001/ash/wm/workspace/two_st... ash/wm/workspace/two_step_edge_cycler.cc:64: (paused_x_ - location.x()) >= kMaxPixelsAfterPause)); On 2015/05/11 21:35:20, pkotwicz wrote: > Can't this become simpler now? i.e. std::abs(location.x() - paused_x_) >= > kMaxPixelsAfterPause) Done. https://codereview.chromium.org/1127133003/diff/60001/ash/wm/workspace/two_st... File ash/wm/workspace/two_step_edge_cycler.h (right): https://codereview.chromium.org/1127133003/diff/60001/ash/wm/workspace/two_st... ash/wm/workspace/two_step_edge_cycler.h:22: // The direction in which a mouse should travel after a pause to switch mode. On 2015/05/11 21:35:21, pkotwicz wrote: > Nit: Can you remove the "after a pause" part of the comment because it is no > longer true? Done. https://codereview.chromium.org/1127133003/diff/60001/ash/wm/workspace/two_st... ash/wm/workspace/two_step_edge_cycler.h:53: // Determines a movement direction that we are watching after a pause. On 2015/05/11 21:35:20, pkotwicz wrote: > Can you please update this comment? Done.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/1127133003/#ps90001 (title: "Adjusts docking dragging logic to be more tolerant (nits)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127133003/90001
Message was sent while issue was closed.
Committed patchset #5 (id:90001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/988c697aff35b3fd347dec35f2f2ac159c8328a9 Cr-Commit-Position: refs/heads/master@{#329268}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:90001) has been created in https://codereview.chromium.org/1128933005/ by dpranke@chromium.org. The reason for reverting is: I suspect this is causing a browser_tests failure on the bots: http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2... https://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Te... Sorry!. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
