Implement workspace scrubbing

Implement workspace scrubbing 

BUG=160903
Test=Manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170688

[pkotwicz, 2012-11-23 16:45:03]

Sadrul, can you please take a look?

[sadrul, 2012-11-23 20:02:07]

I haven't looked at the change in workspace-manager regarding hidden/pending
workspaces. But I have some comments for the cycler:

https://codereview.chromium.org/11417150/diff/7002/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_cycler.cc (right):

https://codereview.chromium.org/11417150/diff/7002/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_cycler.cc:108: for (int i = 0; i < num_scrub_steps;
++i) {
Why the loop here?

https://codereview.chromium.org/11417150/diff/7002/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_cycler.cc:116: // before scrubbing was initiated.
Do we not want to go to previously minimized windows/workspaces?

https://codereview.chromium.org/11417150/diff/7002/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_cycler.cc:139: return ui::ER_HANDLED;
The logic in this function seems rather complex. I was expecting something
simpler, like:

    if (delta_y > threshold) {
      SwitchToNextWorkspace();
      last_transition_location_ = event->root_location();
    } else if (delta_y < -threshold) {
      SwitchToPreviousWorkspace();
      last_transition_location_ = event->root_location();
    } else { ... }

https://codereview.chromium.org/11417150/diff/7002/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_manager.h (right):

https://codereview.chromium.org/11417150/diff/7002/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager.h:85: bool
CycleAwayFromCurrentlyActiveWorkspace();
Where does it cycle to?

[pkotwicz, 2012-11-23 22:08:32]

Changes as requested. Sadrul, can you take another look?

https://codereview.chromium.org/11417150/diff/7002/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_cycler.cc (right):

https://codereview.chromium.org/11417150/diff/7002/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_cycler.cc:116: // before scrubbing was initiated.
I am unsure. My personal preference is not to scrub to previously minimized
windows/workspaces.
If we did scrub to previously minimized windows/workspaces, we would have to
somehow handle the desktop workspace which can have several minimized windows.
Would a three finger scrub unminimize them all at once, unminimize them
individually, not do anything at all and unminimize only maximized windows?

[sadrul, 2012-11-26 16:55:50]

I haven't looked at the pending/hidden workspace enough to know if the change is
good. It would be nice to not have to maintain a new list of workspaces though.
In general, this looks reasonable. You should send this to sky@ (and probably
add some tests, although perhaps we want to iterate with UX first to decide on
the specific behaviour, and then add tests for that)

https://codereview.chromium.org/11417150/diff/7002/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_cycler.cc (right):

https://codereview.chromium.org/11417150/diff/7002/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_cycler.cc:116: // before scrubbing was initiated.
On 2012/11/23 22:08:32, pkotwicz wrote:
> I am unsure. My personal preference is not to scrub to previously minimized
> windows/workspaces.
> If we did scrub to previously minimized windows/workspaces, we would have to
> somehow handle the desktop workspace which can have several minimized windows.
> Would a three finger scrub unminimize them all at once, unminimize them
> individually, not do anything at all and unminimize only maximized windows?

We should confirm this with UX

https://codereview.chromium.org/11417150/diff/3015/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_cycler.cc (right):

https://codereview.chromium.org/11417150/diff/3015/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_cycler.cc:40: WorkspaceCycler::~WorkspaceCycler() {
RemovePreTargetHandler here

https://codereview.chromium.org/11417150/diff/3015/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_cycler.cc:124: ui::EventResult
WorkspaceCycler::OnScrollEvent(ui::ScrollEvent* event) {
You don't need to override these since you aren't doing anything here yet.

[pkotwicz, 2012-11-26 21:07:31]

Scott, can you please take a look?

This CL implements workspace scrubbing for immersive mode. Similar to tab
scrubbing, it is done using Ctrl+drag for now.

There is one known issue with my CL (which it may be best to punt for later
given that this is experimental). With the current CL if the user scrubs to an
empty desktop workspace, any workspace they scrub to afterward does not get
activated as part of the "scrub". I am currently using wm::DeactivateWindow() to
kick the activation controller. In the future, when we are more confident that
we want workspace scrubbing, I imagine that we will want to expose
ActivationController::GetTopmostWindowToActivateInContainer() to
workspace_manager

[sky, 2012-11-26 23:20:49]

I got hung up on what we're trying to do. That said, I do have two comments
here. Once you get resolution from Nicholas and Alex ping the issue and I'll
look again. I suspect the right thing it to change the code so that workspaces
with minimized windows still exist in workspaces_. That way you don't lost the
stacking order when a maximized window is minimized. It does mean some changes
to ignore such workspaces when closing a window, but that shouldn't be that big.

https://codereview.chromium.org/11417150/diff/11002/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace.cc (right):

https://codereview.chromium.org/11417150/diff/11002/ash/wm/workspace/workspac...
ash/wm/workspace/workspace.cc:63: if (!active_window)
return window_->Contains(active_window) ? active_window : NULL;

https://codereview.chromium.org/11417150/diff/11002/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_manager.h (right):

https://codereview.chromium.org/11417150/diff/11002/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_manager.h:158: void
MoveWorkspaceToHiddenOrDelete(Workspace* workspace,
Since all but one of the windows in workspaces_ is hidden having something else
named 'hidden' is ambiguous and easily confused.

[pkotwicz, 2012-11-27 20:28:23]

I pinged alcor@ and kuscher@. Their opinion is that we don't want to allow a
user to cycle through minimized windows / workspaces via scrubbing. This means
that for now we can keep the pending windows unordered.
I changed SetActiveWorkspace() such that the active workspace always goes to the
top of the stack. (I imagine that we will want to have different behavior while
cycling through workspaces however I will punt that till I look into which
animations we want to use when cycling)

[sky, 2012-11-28 15:15:34]

I think always moving the active workspace to the front is going to be
confusing. It basically means resetting the stack all the time. Are you sure
Alex and Nicholas want it?

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace.cc (right):

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace.cc:62: aura::Window* active_window =
wm::GetActiveWindow();
Did you see my earlier comment on this? I believe this can be:
return window_->Contains(active_window) ? active_window : NULL;

at which point it might not be worth a method.

Also, this should return a const aura::Window*. If you need no const then the
method shouldn't be const either.

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_cycler.cc (right):

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_cycler.cc:78: event->type() ==
ui::ET_MOUSE_RELEASED)) {
nit: indent to match previous line.
Also, this could is more readable if you carry through the !, eg event->type()
!= DRAGGED && type != PRESSED..
And what about capture changed, shouldn't that cancel scrubbing?
This code seems like it would be easier to follow if you based it off the
current scrub_state_ first, but maybe I'm wrong.

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_cycler.cc:102: scrub_state_ = CANCELLED_SCRUBBING;
What is the rationale for cancelling when you move far enough in the
x-coordinate? Especially since last_transition_position_ is reset on 117.

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_cycler.cc:108:
workspace_manager_->CycleToWorkspace(WorkspaceManager::WORKSPACE_BELOW);
Wow, we really don't let you cycle to the previous?

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_cycler.h (right):

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_cycler.h:32: 
nit: no newline.

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_cycler.h:47: enum ScrubState {
Style guide says enums are typically first in a section.

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_cycler.h:58: gfx::Point last_transition_position_;
Document coordinates, in fact I think you should name this
last_transition_position_in_root_ to make it clear.

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_manager.cc (right):

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_manager.cc:289: wm::DeactivateWindow(active_window);
I don't understand this, why do you need to to do it?

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_manager.h (right):

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_manager.h:56: 
remove newline.

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_manager.h:58: WORKSPACE_ABOVE,
Above and below are not clear in this content. How about next/previous and use
cycle here, eg:
CycleDirection {
  CYCLE_NEXT, CYCLE_PREVIOUS
}

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_manager.h:96: aura::Window*
GetActiveWorkspaceWindow() const;
Should either return const or not be const.

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_manager.h:168: void SelectWorkspaceBelow(SwitchReason
reason);
Keep the old name, its clearer since below doesn't mean anything.

[pkotwicz, 2012-11-29 01:01:46]

Changes as requested

> I think always moving the active workspace to the front is > going to be
> confusing. It basically means resetting the stack all the > time. Are you sure
> Alex and Nicholas want it?
To be honest, I think that the order that workspaces get shown when a maximized
window is minimized is currently confusing. Showing workspaces in the order that
they were activated seems to make more sense.

For cycling through workspaces, depending on what animations we come up with, it
may end up being slightly better to keep the workspaces in a fixed order. I
haven't asked alex and nicholas about this particular aspect but I expect that
they will give me their feedback once they try out the feature

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_cycler.cc (right):

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_cycler.cc:78: event->type() ==
ui::ET_MOUSE_RELEASED)) {
Fixed nit. Added comment to explain behavior.
We don't want to cancel scrubbing if we get a different mouse event because we
don't want to cancel scrubbing upon getting ui::ET_MOUSE_ENTERED

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_cycler.cc:102: scrub_state_ = CANCELLED_SCRUBBING;
The intent of this old code was to allow horizontal scrub events to go to the
tab scrubber (not be consumed by the workspace cycler)

In the CL I just posted:
- I removed CANCELLED_SCRUBBING state.
- reset last_transition_position_in_root_ such that we don't switch workspaces
when a user scrubs through a whole bunch of tabs and over the course of
scrubbing through (eg 10) tabs moves the required amount to scrub through one
workspace
- Don't consume the mouse event unless the event causes a switch in the active
workspace. This is not ideal for mouse events because the control dragging may
trigger other mouse handlers listening for drags. However this state is
temporary because we want to move to triggering scrubbing on three finger swipes
in the near term

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_cycler.cc:108:
workspace_manager_->CycleToWorkspace(WorkspaceManager::WORKSPACE_BELOW);
I believe that there are two cases where this code prevents cycling to a
previous workspace:
- when the initial workspace is the desktop. (This is the correct behavior in my
opinion)
- cycling is disabled to workspaces created while cycling through workspaces.
(This is a bug in my opinion and I plan on fixing it once I look into the proper
animations for cycling between workspaces)

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_manager.cc (right):

https://codereview.chromium.org/11417150/diff/15001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_manager.cc:289: wm::DeactivateWindow(active_window);
This is a hack. It has the effect of activating a window in the workspace cycled
to. This is similar to what is done in
WorkspaceLayoutManager::ShowStateChanged() when a window is minimized.
This is a hack because it does not work if there is no active window as is the
case when the desktop workspace is active and the desktop workspace has no
toplevel windows in it.

[pkotwicz, 2012-11-29 01:02:12]

Just to be explicit, Scott can you take another look?

[pkotwicz, 2012-11-29 21:53:05]

Scott, can you please take another look?

[sky, 2012-11-29 23:09:51]

https://codereview.chromium.org/11417150/diff/6041/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_cycler.cc (right):

https://codereview.chromium.org/11417150/diff/6041/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_cycler.cc:73: scroll_y_ = 0;
Lets say I've scrolled by 100. If you set scroll_y_ to 0 here won't it get reset
to 100+delta on the next scroll event around line 62/64 and you'll try to update
again since the increment is > kWorkspaceStepSize.

I suggest trying for the following:
. Once you cycle the user should have to cycle a certain amount before cycling
again.
. The cycle stops should be relative to the start of the scroll.

I'm thinking something like this. Do away with 60-66 and convert to something
like:

// Only consider moving if the user moves at least kAmountNeededToScroll since
last scroll.
if (std::abs(last_scroll_offset - event->y_offset()) > kAmountNeededToScroll) {
  const int cycle_delta = event->y_offset() / kWorkspaceStepSize -
last_scroll_offset_ / kWorskpaceSteSize;
  if (cycle_delta > 0) {
    // cycle through cycle_delta workspaces.
    last_scroll_offset_ = event->y_offset();
  }
}

I'm sure I'm not handling the directionality (line 60), but you get the idea.

https://codereview.chromium.org/11417150/diff/6041/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_cycler.cc:77: if (std::abs(scroll_y_) >
kWorkspaceStepSize) {
You should never get here, right?

[pkotwicz, 2012-11-30 01:19:45]

https://codereview.chromium.org/11417150/diff/6041/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_cycler.cc (right):

https://codereview.chromium.org/11417150/diff/6041/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_cycler.cc:73: scroll_y_ = 0;
As far as I understand it, event->x_offset() is a delta as opposed to being
absolute. So a subsequent event will not include the offset of the previous
event. I have tested this and the test results seem consistent.

https://codereview.chromium.org/11417150/diff/6041/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_cycler.cc:77: if (std::abs(scroll_y_) >
kWorkspaceStepSize) {
This should be scroll_x_, fixed!

[sky, 2012-11-30 01:39:45]

https://codereview.chromium.org/11417150/diff/1043/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_cycler.cc (right):

https://codereview.chromium.org/11417150/diff/1043/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_cycler.cc:69:
workspace_manager_->CycleToWorkspace(scroll_y_ > 0 ?
You sure you don't want the logic I outlined in the last patch. Your patch makes
it so you always have to move z-pixels, even right after a switch. That doesn't
seem the greatest to me.

[pkotwicz, 2012-11-30 02:12:48]

https://codereview.chromium.org/11417150/diff/1043/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_cycler.cc (right):

https://codereview.chromium.org/11417150/diff/1043/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_cycler.cc:69:
workspace_manager_->CycleToWorkspace(scroll_y_ > 0 ?
I am unsure if I completely understand your comment.
It is true that my CL forces a user to always move z-pixels even after a switch.
In particular, I ignore the fact that a user may have scrolled z+y pixels
causing the workspace switch (I do not take in account (y), the remainder). I do
not implement switching through multiple workspaces as a result of a single
scroll event either.
I can add a TODO to implement this.

Some more background:
I actually had implemented switching through multiple workspaces as the result
of a single scroll event in a previous CL and Sadrul advised me to only switch
through a single workspace per scroll event. In the world of this CL where there
is no/little animation while switching through workspaces, a user would most
likely not know that they have switched through multiple workspaces, making it
confusing.

[sky, 2012-11-30 02:32:16]

LGTM

https://codereview.chromium.org/11417150/diff/1043/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_manager.cc (right):

https://codereview.chromium.org/11417150/diff/1043/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_manager.cc:287: if (active_window)
Add a TODO here, we shouldn't need to do this.

[commit-bot: I haz the power, 2012-12-02 02:18:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/11417150/4018

[commit-bot: I haz the power, 2012-12-02 07:43:54]

Change committed as 170688