ash: Add accelerator for snapping windows left/right.

ash: Add accelerator for snapping windows left/right.

BUG=none
TEST=none

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

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

[sadrul, 2012-04-02 06:12:41]

A feature request (happy to add only when !defined(NDEBUG))

[sky, 2012-04-02 15:25:20]

I like it, but I tend to think we should put these in the context menu when you
click a window title bar and include other things like minimize and maximize.
WDYT?

[Daniel Erat, 2012-04-17 16:51:19]

http://codereview.chromium.org/9956056/diff/2001/ash/accelerators/accelerator...
File ash/accelerators/accelerator_table.cc (right):

http://codereview.chromium.org/9956056/diff/2001/ash/accelerators/accelerator...
ash/accelerators/accelerator_table.cc:74: { ui::ET_TRANSLATED_KEY_PRESS,
ui::VKEY_LEFT, false, true, true,
it has been asserted in the past (by pkasting, i think) that we should stay away
from ctrl-alt accelerators.  i think they conflict with certain accelerators for
entering non-latin characters, although perhaps only on other OSes.

also, these are weird since i believe that we remap alt-up/down to page up/down
and ctrl-alt-up/down to home/end on chromebooks.  it's best to stay as far away
from that mess as possible.

i used alt-comma and alt-period for similar actions in the old X window manager.
 any reason that those, or their ctrl- equivalents, couldn't be used here
instead?

http://codereview.chromium.org/9956056/diff/2001/ash/accelerators/accelerator...
File ash/accelerators/accelerator_table.h (right):

http://codereview.chromium.org/9956056/diff/2001/ash/accelerators/accelerator...
ash/accelerators/accelerator_table.h:35: SNAP_WINDOW_LEFT,
nit: mind re-alphabetizing this list?

[sadrul, 2012-04-25 16:04:20]

Updated the accelerators and added support for minimize/maximize/center.
Awaiting response from afk@ about adding these in the window-menu.

http://codereview.chromium.org/9956056/diff/2001/ash/accelerators/accelerator...
File ash/accelerators/accelerator_table.cc (right):

http://codereview.chromium.org/9956056/diff/2001/ash/accelerators/accelerator...
ash/accelerators/accelerator_table.cc:74: { ui::ET_TRANSLATED_KEY_PRESS,
ui::VKEY_LEFT, false, true, true,
On 2012/04/17 16:51:19, Daniel Erat wrote:
> it has been asserted in the past (by pkasting, i think) that we should stay
away
> from ctrl-alt accelerators.  i think they conflict with certain accelerators
for
> entering non-latin characters, although perhaps only on other OSes.
> 
> also, these are weird since i believe that we remap alt-up/down to page
up/down
> and ctrl-alt-up/down to home/end on chromebooks.  it's best to stay as far
away
> from that mess as possible.
> 
> i used alt-comma and alt-period for similar actions in the old X window
manager.
>  any reason that those, or their ctrl- equivalents, couldn't be used here
> instead?

I changed the bindings to the ones suggested in the bug (alt- []-=+)

http://codereview.chromium.org/9956056/diff/2001/ash/accelerators/accelerator...
File ash/accelerators/accelerator_table.h (right):

http://codereview.chromium.org/9956056/diff/2001/ash/accelerators/accelerator...
ash/accelerators/accelerator_table.h:35: SNAP_WINDOW_LEFT,
On 2012/04/17 16:51:19, Daniel Erat wrote:
> nit: mind re-alphabetizing this list?

Done.

[Daniel Erat, 2012-04-25 16:11:42]

http://codereview.chromium.org/9956056/diff/6001/ash/accelerators/accelerator...
File ash/accelerators/accelerator_table.h (right):

http://codereview.chromium.org/9956056/diff/6001/ash/accelerators/accelerator...
ash/accelerators/accelerator_table.h:53: WINDOW_POSITION_CENTER,
nit: alphabetize these

http://codereview.chromium.org/9956056/diff/6001/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

http://codereview.chromium.org/9956056/diff/6001/ash/wm/window_util.cc#newcod...
ash/wm/window_util.cc:107: window->SetBounds(center);
should this be taking the grid into account?

[sadrul, 2012-04-25 16:15:38]

http://codereview.chromium.org/9956056/diff/6001/ash/accelerators/accelerator...
File ash/accelerators/accelerator_table.h (right):

http://codereview.chromium.org/9956056/diff/6001/ash/accelerators/accelerator...
ash/accelerators/accelerator_table.h:53: WINDOW_POSITION_CENTER,
On 2012/04/25 16:11:42, Daniel Erat wrote:
> nit: alphabetize these

Whoops. Done.

http://codereview.chromium.org/9956056/diff/6001/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

http://codereview.chromium.org/9956056/diff/6001/ash/wm/window_util.cc#newcod...
ash/wm/window_util.cc:107: window->SetBounds(center);
On 2012/04/25 16:11:42, Daniel Erat wrote:
> should this be taking the grid into account?

[don't have an opinion]

[Daniel Erat, 2012-04-25 16:17:28]

lgtm

[sky, 2012-04-25 17:26:01]

http://codereview.chromium.org/9956056/diff/10001/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/9956056/diff/10001/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:462: internal::SnapSizer
sizer(window,
Can you make it so that this iterates over the possible snap sizes? For example,
if the window isn't already snapped first invocation snaps to first snapped
bounds, second invocation snaps to second bounds...

http://codereview.chromium.org/9956056/diff/10001/ash/wm/window_util.cc
File ash/wm/window_util.cc (right):

http://codereview.chromium.org/9956056/diff/10001/ash/wm/window_util.cc#newco...
ash/wm/window_util.cc:105: gfx::Rect center =
monitor->GetWorkAreaBounds().Center(
Use one of GetUnmaximizedWorkAreaBounds or GetMaximizedWindowBounds.

[sadrul, 2012-04-25 18:05:34]

http://codereview.chromium.org/9956056/diff/10001/ash/accelerators/accelerato...
File ash/accelerators/accelerator_controller.cc (right):

http://codereview.chromium.org/9956056/diff/10001/ash/accelerators/accelerato...
ash/accelerators/accelerator_controller.cc:462: internal::SnapSizer
sizer(window,
On 2012/04/25 17:26:01, sky wrote:
> Can you make it so that this iterates over the possible snap sizes? For
example,
> if the window isn't already snapped first invocation snaps to first snapped
> bounds, second invocation snaps to second bounds...

ooo, nice! Done.

[sky, 2012-04-25 19:23:20]

http://codereview.chromium.org/9956056/diff/8006/ash/wm/workspace/snap_sizer.cc
File ash/wm/workspace/snap_sizer.cc (right):

http://codereview.chromium.org/9956056/diff/8006/ash/wm/workspace/snap_sizer....
ash/wm/workspace/snap_sizer.cc:72: if (target != bounds && target.width() >
bounds.width())
Can you use rect == to find the current snap bounds?

[sadrul, 2012-04-25 19:37:54]

http://codereview.chromium.org/9956056/diff/8006/ash/wm/workspace/snap_sizer.cc
File ash/wm/workspace/snap_sizer.cc (right):

http://codereview.chromium.org/9956056/diff/8006/ash/wm/workspace/snap_sizer....
ash/wm/workspace/snap_sizer.cc:72: if (target != bounds && target.width() >
bounds.width())
On 2012/04/25 19:23:20, sky wrote:
> Can you use rect == to find the current snap bounds?

Done.

[sky, 2012-04-25 19:46:13]

LGTM