Dont allow the user to 'resize a window out of the workarea'

Dont allow the user to 'resize a window out of the workarea'


BUG=151255
TEST=unit test & visual


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

[Mr4D (OOO till 08-26), 2012-10-09 22:00:34]

Please have a look!

[sky, 2012-10-09 23:58:53]

If we really want to enforce there is always 10 pixels on screen, then we should
cover the move case too (eg grab a corner and drag to the side). I think this is
better done in CalculateBoundsForDrag. And you should share the constant with
the one in WorkspaceLayoutManager2::AdjustWindowSizeForScreenChange .

http://codereview.chromium.org/11087037/diff/1/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/11087037/diff/1/ash/wm/window_resizer.cc#newco...
ash/wm/window_resizer.cc:180: gfx::Rect work_area =
Move work_area inside if.

[Mr4D (OOO till 08-26), 2012-10-10 23:14:35]

I removed the 10 pixel overhang and commented at your change request.

I have however kept the offset logic/constant in there for the moment since I am
sure that this will be asked for sooner or later.

Please have another look!

http://codereview.chromium.org/11087037/diff/1/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/11087037/diff/1/ash/wm/window_resizer.cc#newco...
ash/wm/window_resizer.cc:180: gfx::Rect work_area =
cannot since it's being used in line 233 as well.

[sky, 2012-10-11 00:48:29]

http://codereview.chromium.org/11087037/diff/3003/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/11087037/diff/3003/ash/wm/window_resizer.cc#ne...
ash/wm/window_resizer.cc:104: const int WindowResizer::kMinimumOnScreenSize = 0;
0?
This should be the same value as in
WorkspaceLayoutManager2::AdjustWindowSizeForScreenChange. Promote a constant
that both places share.

http://codereview.chromium.org/11087037/diff/3003/ash/wm/window_resizer.cc#ne...
ash/wm/window_resizer.cc:213: 
Wouldn't it be better to constrain here? So that we can force at least 10 pixels
on screen for resize and moving?

[Mr4D (OOO till 08-26), 2012-10-11 19:41:13]

Please see the comments.

Note: The bug was to avoid being able to resize a window out of the screen. That
is exactly done here. If we want to change the behavior of the minimal visible
area of a window I would now suggest that we should create a new issue and
address that separately (after discussing with all parties what we want).

http://codereview.chromium.org/11087037/diff/3003/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/11087037/diff/3003/ash/wm/window_resizer.cc#ne...
ash/wm/window_resizer.cc:104: const int WindowResizer::kMinimumOnScreenSize = 0;
Yes - 0 is what moving does - which was requested. It essentially means that the
e.g. left most pixel of a window can land on -1, allowing the user to grab the
shadow / resizer. (Yes, this is a bit odd which is the reason why I thought that
enforcing a bit of visibility might be good, but Alex and you were requesting it
to be the same as move.)

For the 10 pixels from WindowManager: Never seen that constant before, but once
we decide of keeping the window always partially visible we can do that.

http://codereview.chromium.org/11087037/diff/3003/ash/wm/window_resizer.cc#ne...
ash/wm/window_resizer.cc:213: 
I thought the same first when I looked at the problem. However - the size and
the origin are independently changed from each other and as such it is not that
simple to reverse what the other functions have done (if they have).

[sky, 2012-10-11 21:26:48]

The resolution of the thread, and I talked with Alex to verify this,
is that we should force 10 pixels on screen for both resize and move.

  -Scott

On Wed, Oct 10, 2012 at 5:48 PM,  <sky@chromium.org> wrote:
>
> http://codereview.chromium.org/11087037/diff/3003/ash/wm/window_resizer.cc
> File ash/wm/window_resizer.cc (right):
>
>
http://codereview.chromium.org/11087037/diff/3003/ash/wm/window_resizer.cc#ne...
> ash/wm/window_resizer.cc:104: const int
> WindowResizer::kMinimumOnScreenSize = 0;
> 0?
> This should be the same value as in
> WorkspaceLayoutManager2::AdjustWindowSizeForScreenChange. Promote a
> constant that both places share.
>
>
http://codereview.chromium.org/11087037/diff/3003/ash/wm/window_resizer.cc#ne...
> ash/wm/window_resizer.cc:213:
> Wouldn't it be better to constrain here? So that we can force at least
> 10 pixels on screen for resize and moving?
>
> http://codereview.chromium.org/11087037/

[Mr4D (OOO till 08-26), 2012-10-12 00:42:23]

Ahh. Somehow that was not clear from the eMail thread. It was said to have it
the same as the move case (which is 0 pixels). So I will have then another look
at this.

[Mr4D (OOO till 08-26), 2012-10-16 15:27:36]

Size and move operations are now staying within the given bounds.

Please have another look.

[sky, 2012-10-16 22:57:46]

http://codereview.chromium.org/11087037/diff/25001/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/11087037/diff/25001/ash/ash.gyp#newcode572
ash/ash.gyp:572: 'wm/workspace/workspace_layout_manager2.h',
Why are you adding this here? It's listed @ 409.

http://codereview.chromium.org/11087037/diff/25001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/11087037/diff/25001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:194: if (details.bounds_change & kBoundsChange_Resizes)
{
This code is logically similar to that of 245-250, right? Can't we expand the if
there to deal with this case?

http://codereview.chromium.org/11087037/diff/25001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:207: if (details.size_change_direction &
kBoundsChangeDirection_Vertical) {
Similarly should we fold 237-244 into this?

http://codereview.chromium.org/11087037/diff/25001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_layout_manager2.h (right):

http://codereview.chromium.org/11087037/diff/25001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager2.h:31: // Upon user resizing, at least
this many pixels should remain visible on the
resizing, moving or display area changing, right? Or maybe just say the number
of DIPs we force on screen.

[Mr4D (OOO till 08-26), 2012-10-17 00:05:17]

Have another look

http://codereview.chromium.org/11087037/diff/25001/ash/ash.gyp
File ash/ash.gyp (right):

http://codereview.chromium.org/11087037/diff/25001/ash/ash.gyp#newcode572
ash/ash.gyp:572: 'wm/workspace/workspace_layout_manager2.h',
Done.

http://codereview.chromium.org/11087037/diff/25001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

http://codereview.chromium.org/11087037/diff/25001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:194: if (details.bounds_change & kBoundsChange_Resizes)
{
The thing is that a resize operation can change size and pos. If we do this
after the other pos changes below we will introduce an error.

That said, I was moving the sizing code up here before the moving correction
code.

http://codereview.chromium.org/11087037/diff/25001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:207: if (details.size_change_direction &
kBoundsChangeDirection_Vertical) {
On 2012/10/16 22:57:46, sky wrote:
> Similarly should we fold 237-244 into this?

Done.

http://codereview.chromium.org/11087037/diff/25001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_layout_manager2.h (right):

http://codereview.chromium.org/11087037/diff/25001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_layout_manager2.h:31: // Upon user resizing, at least
this many pixels should remain visible on the
On 2012/10/16 22:57:46, sky wrote:
> resizing, moving or display area changing, right? Or maybe just say the number
> of DIPs we force on screen.

Done.

[sky, 2012-10-17 00:23:58]

LGTM

[commit-bot: I haz the power, 2012-10-17 00:33:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/11087037/27002

[commit-bot: I haz the power, 2012-10-17 11:14:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skuhne@chromium.org/11087037/27002

[commit-bot: I haz the power, 2012-10-17 13:31:31]

Change committed as 162390