Drag and rotate windows

Drag and rotate windows between/within workspaces

BUG=none
TEST=new tests in workspace_manager_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107643

[oshima, 2011-10-26 22:48:03]

I'm adding more tests, but the rest is ready for review. PTL

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
File ui/aura_shell/default_container_event_filter.cc (right):

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
ui/aura_shell/default_container_event_filter.cc:30: // incorporate the logic
below and intorduce DragObserver to decouple DCLM.
I'll work on refactoring in separate CL

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
File ui/aura_shell/workspace/workspace.cc (right):

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.cc:103: std::swap(windows_[drag_index],
windows_[target_index]);
I feel there is better way but i somehow found find. Do you have suggestion?

[oshima, 2011-10-27 00:49:10]

uploaded new patch with tests. PTAL

[sky, 2011-10-27 00:53:03]

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
File ui/aura_shell/default_container_event_filter.cc (right):

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
ui/aura_shell/default_container_event_filter.cc:43: switch(drag_state_) {
nit: 'switch(' -> 'switch ('

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
ui/aura_shell/default_container_event_filter.cc:45: if(!UpdateDragState())
nit: 'if(' -> 'if ('

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
ui/aura_shell/default_container_event_filter.cc:52: break;
default: NOTREACHED

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
ui/aura_shell/default_container_event_filter.cc:53: };
no;

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
File ui/aura_shell/default_container_layout_manager.cc (right):

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
ui/aura_shell/default_container_layout_manager.cc:67:
workspace_manager_->SwapWindow(drag, swap_target);
Don't we want to move rather than swapping?

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
File ui/aura_shell/workspace/workspace.cc (right):

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.cc:70: // If all windows fits to the width of
workspace, it returns the
Split this comment up and move it into the appropriate section.

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.cc:77: if (total_width < bounds_.width()) {
nit: make this if (GetTotalWidth() < bounds_.width())

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.cc:103: std::swap(windows_[drag_index],
windows_[target_index]);
On 2011/10/26 22:48:03, oshima wrote:
> I feel there is better way but i somehow found find. Do you have suggestion?

I can't think of a better way.

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
File ui/aura_shell/workspace/workspace.h (right):

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.h:58: // Returns a window to swap based on the
|position|. See implementation
I would nuke the second sentence and leave it at the first.

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.h:64: void SwapWindow(aura::Window* drag,
aura::Window* target);
nit: rename 'drag' to 'source' and name this SwapWindows.

[oshima, 2011-10-27 16:16:26]

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
File ui/aura_shell/default_container_event_filter.cc (right):

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
ui/aura_shell/default_container_event_filter.cc:43: switch(drag_state_) {
On 2011/10/27 00:53:03, sky wrote:
> nit: 'switch(' -> 'switch ('

Done.

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
ui/aura_shell/default_container_event_filter.cc:45: if(!UpdateDragState())
On 2011/10/27 00:53:03, sky wrote:
> nit: 'if(' -> 'if ('

Done.

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
ui/aura_shell/default_container_event_filter.cc:52: break;
On 2011/10/27 00:53:03, sky wrote:
> default: NOTREACHED

drag_state_ is enum and compilation fails if I don't handle other cases.

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
ui/aura_shell/default_container_event_filter.cc:53: };
On 2011/10/27 00:53:03, sky wrote:
> no;

Done.

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
File ui/aura_shell/default_container_layout_manager.cc (right):

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/default_contai...
ui/aura_shell/default_container_layout_manager.cc:67:
workspace_manager_->SwapWindow(drag, swap_target);
On 2011/10/27 00:53:03, sky wrote:
> Don't we want to move rather than swapping?

You're right. we want move (or rotate). I'll fix asap.

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
File ui/aura_shell/workspace/workspace.cc (right):

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.cc:70: // If all windows fits to the width of
workspace, it returns the
On 2011/10/27 00:53:03, sky wrote:
> Split this comment up and move it into the appropriate section.

Done.

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.cc:77: if (total_width < bounds_.width()) {
On 2011/10/27 00:53:03, sky wrote:
> nit: make this if (GetTotalWidth() < bounds_.width())

Done.

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
File ui/aura_shell/workspace/workspace.h (right):

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.h:58: // Returns a window to swap based on the
|position|. See implementation
On 2011/10/27 00:53:03, sky wrote:
> I would nuke the second sentence and leave it at the first.

Done.

http://codereview.chromium.org/8391035/diff/6003/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.h:64: void SwapWindow(aura::Window* drag,
aura::Window* target);
On 2011/10/27 00:53:03, sky wrote:
> nit: rename 'drag' to 'source' and name this SwapWindows.

Done.

[sky, 2011-10-27 16:21:18]

> You're right. we want move (or rotate). I'll fix asap.

Let me know when I should take another look.

  -Scott

[oshima, 2011-10-27 18:53:10]

Uploaded new patch that rotates window. Added new tests.

[sky, 2011-10-27 19:46:41]

LGTM

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
File ui/aura_shell/workspace/workspace.cc (right):

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.cc:229: 
nit: remove on of these lines.

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
File ui/aura_shell/workspace/workspace.h (right):

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.h:64: // Returns a window to rotate to based
on the |position|.
remove 'the'

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.h:83: // Layout windows. The workspace doesn't
set bounds on the |ignore| if it's
'the ignore' -> 'ignore'
same with next line

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.h:111: // Test only: Changes how may windows
workspace can haven.
haven -> have

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
File ui/aura_shell/workspace/workspace_manager_unittest.cc (right):

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace_manager_unittest.cc:38: };
DISALLOW_COPY_AND_ASSIGN

[oshima, 2011-10-27 20:25:43]

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
File ui/aura_shell/workspace/workspace.cc (right):

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.cc:229: 
On 2011/10/27 19:46:41, sky wrote:
> nit: remove on of these lines.

Done.

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
File ui/aura_shell/workspace/workspace.h (right):

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.h:64: // Returns a window to rotate to based
on the |position|.
On 2011/10/27 19:46:41, sky wrote:
> remove 'the'

Done.

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.h:83: // Layout windows. The workspace doesn't
set bounds on the |ignore| if it's
On 2011/10/27 19:46:41, sky wrote:
> 'the ignore' -> 'ignore'
> same with next line

removed all "the" before ||

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.h:111: // Test only: Changes how may windows
workspace can haven.
On 2011/10/27 19:46:41, sky wrote:
> haven -> have

Done.

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
File ui/aura_shell/workspace/workspace_manager_unittest.cc (right):

http://codereview.chromium.org/8391035/diff/3049/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace_manager_unittest.cc:38: };
On 2011/10/27 19:46:41, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[oshima, 2011-10-27 22:23:29]

Found a bug where I wasn't handle dropping a source in the middle. Fix the bug
and added test. PTAL.

Thanks,

[sky, 2011-10-27 22:28:53]

LGTM

http://codereview.chromium.org/8391035/diff/3050/ui/aura_shell/workspace/work...
File ui/aura_shell/workspace/workspace.cc (right):

http://codereview.chromium.org/8391035/diff/3050/ui/aura_shell/workspace/work...
ui/aura_shell/workspace/workspace.cc:142: iter++;
++iter.

[oshima, 2011-10-27 22:31:45]

On 2011/10/27 22:28:53, sky wrote:
> LGTM
> 
>
http://codereview.chromium.org/8391035/diff/3050/ui/aura_shell/workspace/work...
> File ui/aura_shell/workspace/workspace.cc (right):
> 
>
http://codereview.chromium.org/8391035/diff/3050/ui/aura_shell/workspace/work...
> ui/aura_shell/workspace/workspace.cc:142: iter++;
> ++iter.
done.