Make touch-resizing windows to screen edge possible

Make touch-resizing windows to screen edge possible

When resizing a window using touch, the window border is often several
pixels behind the finger. So, the window border cannot be moved to the
edge of the screen. To fix it:
 - upon start of resizing with touch, the appropriate border immediately
   catches up the finger and then follows the finger;
 - the snapping distance for touch resizing is doubled.

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

[mohsen, 2013-05-06 20:18:33]

Please take a look...

[sadrul, 2013-05-08 17:10:56]

https://codereview.chromium.org/15008002/diff/1001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/1001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:132: ui::ET_GESTURE_SCROLL_BEGIN) {
Should this check current_event()->IsGestureEvent()?

https://codereview.chromium.org/15008002/diff/1001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:279: if (details.is_touch && (details.bounds_change &
kBoundsChange_Resizes)) {
Return early instead of nesting too deep.

https://codereview.chromium.org/15008002/diff/1001/ash/wm/window_resizer.h
File ash/wm/window_resizer.h (right):

https://codereview.chromium.org/15008002/diff/1001/ash/wm/window_resizer.h#ne...
ash/wm/window_resizer.h:107: static void AdjustDeltas(const Details& details,
int* delta_x, int* delta_y);
How about:

static gfx::Vector2d AdjustDeltaForTouchDrag(const Details& details, const
gfx::Vector2d& delta);

https://codereview.chromium.org/15008002/diff/1001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_window_resizer.h (right):

https://codereview.chromium.org/15008002/diff/1001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_window_resizer.h:41: // window gets this close to an
edge of the screen it snaps to the edge.
'... as the window gets close the an edge ...'

https://codereview.chromium.org/15008002/diff/1001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_window_resizer.h:45: // the window gets this close to
an edge of the screen it snaps to the edge.
This can just be a local const in workspace_window_resizer.cc?

[mohsen, 2013-05-28 23:27:39]

https://codereview.chromium.org/15008002/diff/1001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/1001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:132: ui::ET_GESTURE_SCROLL_BEGIN) {
On 2013/05/08 17:10:56, sadrul wrote:
> Should this check current_event()->IsGestureEvent()?

Done.

https://codereview.chromium.org/15008002/diff/1001/ash/wm/window_resizer.cc#n...
ash/wm/window_resizer.cc:279: if (details.is_touch && (details.bounds_change &
kBoundsChange_Resizes)) {
On 2013/05/08 17:10:56, sadrul wrote:
> Return early instead of nesting too deep.

Done.

https://codereview.chromium.org/15008002/diff/1001/ash/wm/window_resizer.h
File ash/wm/window_resizer.h (right):

https://codereview.chromium.org/15008002/diff/1001/ash/wm/window_resizer.h#ne...
ash/wm/window_resizer.h:107: static void AdjustDeltas(const Details& details,
int* delta_x, int* delta_y);
On 2013/05/08 17:10:56, sadrul wrote:
> How about:
> 
> static gfx::Vector2d AdjustDeltaForTouchDrag(const Details& details, const
> gfx::Vector2d& delta);

delta_x and delta_y are defined as two separate integers and other methods
working with these two values take them separately. Changing this method to get
Vector2d would make it inconsistent with the rest of the code, IMO. Also, I have
to make a Vector2d just to feed this method. Unless, I switch other methods to
Vector2d, too. What do you think? (By the way, I changed the name).

https://codereview.chromium.org/15008002/diff/1001/ash/wm/workspace/workspace...
File ash/wm/workspace/workspace_window_resizer.h (right):

https://codereview.chromium.org/15008002/diff/1001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_window_resizer.h:41: // window gets this close to an
edge of the screen it snaps to the edge.
On 2013/05/08 17:10:56, sadrul wrote:
> '... as the window gets close the an edge ...'

This is the original wording and I haven't changed it. Also, I don't see what
the problem is with it! Maybe you are (or I am?) misreading it? (... as the
window gets "this close" to the edge ...)

https://codereview.chromium.org/15008002/diff/1001/ash/wm/workspace/workspace...
ash/wm/workspace/workspace_window_resizer.h:45: // the window gets this close to
an edge of the screen it snaps to the edge.
On 2013/05/08 17:10:56, sadrul wrote:
> This can just be a local const in workspace_window_resizer.cc?

Done.

[sadrul, 2013-05-29 04:01:51]

LGTM

https://codereview.chromium.org/15008002/diff/21001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/21001/ash/wm/window_resizer.cc#...
ash/wm/window_resizer.cc:189: AdjustDeltaForTouch(details, &delta_x, &delta_y);
My suggestion was to make AdjustDeltaForTouch more like the other static methods
here (e.g. AdjustBoundsToGrid). So you could do:

 gfx::Vector2d delta = location - detials.initial_location;
 delta = AdjustDeltaForTouch(details, delta);
 ..

But looking at how GetSizeForDrag takes the deltas as int*s, that wouldn't be an
improvement.

[mohsen, 2013-05-29 15:38:21]

sky@: for OWNERS in /ash/, please.

[sky, 2013-05-29 19:53:03]

Please add test coverage.

[sadrul, 2013-06-04 19:17:24]

https://codereview.chromium.org/15008002/diff/30001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/15008002/diff/30001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer_unittest.cc:1637: EXPECT_EQ("100,100
600x" + base::IntToString(expected_height),
This is better done using gfx::Rect(100, 100, 600, expected_height).ToString()

https://codereview.chromium.org/15008002/diff/30001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer_unittest.cc:1657: EXPECT_EQ("100,100
666x" + base::IntToString(expected_height),
Why should the height change when dragging towards the right?

[mohsen, 2013-06-04 19:30:08]

https://codereview.chromium.org/15008002/diff/30001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/15008002/diff/30001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer_unittest.cc:1637: EXPECT_EQ("100,100
600x" + base::IntToString(expected_height),
On 2013/06/04 19:17:24, sadrul wrote:
> This is better done using gfx::Rect(100, 100, 600, expected_height).ToString()

What about directly comparing the gfx::Rects, instead of comparing ToString()
results? (Though other tests in this file used the style I originally used).

https://codereview.chromium.org/15008002/diff/30001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer_unittest.cc:1657: EXPECT_EQ("100,100
666x" + base::IntToString(expected_height),
On 2013/06/04 19:17:24, sadrul wrote:
> Why should the height change when dragging towards the right?

The height is not changing. It just depends on kRootHeight constant. That's why
it is calculated.

[sadrul, 2013-06-04 19:42:46]

https://codereview.chromium.org/15008002/diff/30001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/15008002/diff/30001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer_unittest.cc:1637: EXPECT_EQ("100,100
600x" + base::IntToString(expected_height),
On 2013/06/04 19:30:08, mohsen wrote:
> On 2013/06/04 19:17:24, sadrul wrote:
> > This is better done using gfx::Rect(100, 100, 600,
expected_height).ToString()
> 
> What about directly comparing the gfx::Rects, instead of comparing ToString()
> results? (Though other tests in this file used the style I originally used).

Direct Rect-comparisons don't show human-readable values on mismatch. So we
convert from Rect to strings for EXPECTs. But we should avoid writing code that
can break if/when Rect::ToString() changes.

https://codereview.chromium.org/15008002/diff/30001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer_unittest.cc:1657: EXPECT_EQ("100,100
666x" + base::IntToString(expected_height),
On 2013/06/04 19:30:08, mohsen wrote:
> On 2013/06/04 19:17:24, sadrul wrote:
> > Why should the height change when dragging towards the right?
> 
> The height is not changing. It just depends on kRootHeight constant. That's
why
> it is calculated.

I mean in line 1656

[mohsen, 2013-06-04 20:13:33]

https://codereview.chromium.org/15008002/diff/30001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_window_resizer_unittest.cc (right):

https://codereview.chromium.org/15008002/diff/30001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer_unittest.cc:1637: EXPECT_EQ("100,100
600x" + base::IntToString(expected_height),
On 2013/06/04 19:42:46, sadrul wrote:
> On 2013/06/04 19:30:08, mohsen wrote:
> > On 2013/06/04 19:17:24, sadrul wrote:
> > > This is better done using gfx::Rect(100, 100, 600,
> expected_height).ToString()
> > 
> > What about directly comparing the gfx::Rects, instead of comparing
ToString()
> > results? (Though other tests in this file used the style I originally used).
> 
> Direct Rect-comparisons don't show human-readable values on mismatch. So we
> convert from Rect to strings for EXPECTs. But we should avoid writing code
that
> can break if/when Rect::ToString() changes.

I see. Done.

https://codereview.chromium.org/15008002/diff/30001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer_unittest.cc:1657: EXPECT_EQ("100,100
666x" + base::IntToString(expected_height),
On 2013/06/04 19:42:46, sadrul wrote:
> On 2013/06/04 19:30:08, mohsen wrote:
> > On 2013/06/04 19:17:24, sadrul wrote:
> > > Why should the height change when dragging towards the right?
> > 
> > The height is not changing. It just depends on kRootHeight constant. That's
> why
> > it is calculated.
> 
> I mean in line 1656

I'm talking about the same line! The height of the window is (kRootHeight - 200)
in the beginning and it remains the same after dragging towards the right.

[sadrul, 2013-06-04 20:19:02]

>
https://codereview.chromium.org/15008002/diff/30001/ash/wm/workspace/workspac...
> ash/wm/workspace/workspace_window_resizer_unittest.cc:1657: EXPECT_EQ("100,100
> 666x" + base::IntToString(expected_height),
> On 2013/06/04 19:42:46, sadrul wrote:
> > On 2013/06/04 19:30:08, mohsen wrote:
> > > On 2013/06/04 19:17:24, sadrul wrote:
> > > > Why should the height change when dragging towards the right?
> > > 
> > > The height is not changing. It just depends on kRootHeight constant.
That's
> > why
> > > it is calculated.
> > 
> > I mean in line 1656
> 
> I'm talking about the same line! The height of the window is (kRootHeight -
200)
> in the beginning and it remains the same after dragging towards the right.

Oh, ha! The value does not actually change. The extra line there confused me.
LGTM

[mohsen, 2013-06-04 21:04:23]

sky@: Unit tests are added. Please take a look.

[sky, 2013-06-04 21:18:55]

https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc#...
ash/wm/window_resizer.cc:135: if (window->GetRootWindow()->current_event())
I don't like grabbing the current event, it's too error prone. Please pass the
state in you need.

https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc#...
ash/wm/window_resizer.cc:280: void WindowResizer::AdjustDeltaForTouch(const
Details& details,
Is all of this to snap the edge to the edge of the screen? Do we only care about
workspace windows or all windows? It seems like we should be able to share some
of the code in WorkspaceWindowResizer that snaps resizes. Something like
MagneticallySnapResizeToOtherWindows but that snaps to the screen edges.

[mohsen, 2013-06-05 16:19:59]

https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc#...
ash/wm/window_resizer.cc:280: void WindowResizer::AdjustDeltaForTouch(const
Details& details,
On 2013/06/04 21:18:55, sky wrote:
> Is all of this to snap the edge to the edge of the screen? Do we only care
about
> workspace windows or all windows? It seems like we should be able to share
some
> of the code in WorkspaceWindowResizer that snaps resizes. Something like
> MagneticallySnapResizeToOtherWindows but that snaps to the screen edges.

This code is not for snapping. Its purpose is to move window border faster than
the finger (when necessary) so that the border catches up the finger at the edge
of the screen (see:
https://code.google.com/p/chromium/issues/detail?id=151168#c6). For the snapping
part, I have just increased the snapping distance (for touch-resizing) used by
existing snapping code.

[sky, 2013-06-06 15:27:42]

MagneticallySnapResizeToOtherWindows handles a resize. Isn't that what
you care about?

On Wed, Jun 5, 2013 at 9:20 AM,  <mohsen@chromium.org> wrote:
>
> https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc
> File ash/wm/window_resizer.cc (right):
>
>
https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc#...
> ash/wm/window_resizer.cc:280: void
> WindowResizer::AdjustDeltaForTouch(const Details& details,
> On 2013/06/04 21:18:55, sky wrote:
>>
>> Is all of this to snap the edge to the edge of the screen? Do we only
>
> care about
>>
>> workspace windows or all windows? It seems like we should be able to
>
> share some
>>
>> of the code in WorkspaceWindowResizer that snaps resizes. Something
>
> like
>>
>> MagneticallySnapResizeToOtherWindows but that snaps to the screen
>
> edges.
>
> This code is not for snapping. Its purpose is to move window border
> faster than the finger (when necessary) so that the border catches up
> the finger at the edge of the screen (see:
> https://code.google.com/p/chromium/issues/detail?id=151168#c6). For the
> snapping part, I have just increased the snapping distance (for
> touch-resizing) used by existing snapping code.
>
> https://codereview.chromium.org/15008002/

[mohsen, 2013-06-06 19:12:44]

As far as I understand, MagneticallySnapResizeToOtherWindows() performs (only)
the magnetic snapping during a resize. That's not what I want to do! I want the
border to catch up the finger (as I described earlier) and have found
WindowResizer::CalculateBoundsForDrag() the most appropriate place to do that.
Do you have any other opinion?

On 2013/06/06 15:27:42, sky wrote:
> MagneticallySnapResizeToOtherWindows handles a resize. Isn't that what
> you care about?
> 
> On Wed, Jun 5, 2013 at 9:20 AM,  <mailto:mohsen@chromium.org> wrote:
> >
> > https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc
> > File ash/wm/window_resizer.cc (right):
> >
> >
>
https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc#...
> > ash/wm/window_resizer.cc:280: void
> > WindowResizer::AdjustDeltaForTouch(const Details& details,
> > On 2013/06/04 21:18:55, sky wrote:
> >>
> >> Is all of this to snap the edge to the edge of the screen? Do we only
> >
> > care about
> >>
> >> workspace windows or all windows? It seems like we should be able to
> >
> > share some
> >>
> >> of the code in WorkspaceWindowResizer that snaps resizes. Something
> >
> > like
> >>
> >> MagneticallySnapResizeToOtherWindows but that snaps to the screen
> >
> > edges.
> >
> > This code is not for snapping. Its purpose is to move window border
> > faster than the finger (when necessary) so that the border catches up
> > the finger at the edge of the screen (see:
> > https://code.google.com/p/chromium/issues/detail?id=151168#c6). For the
> > snapping part, I have just increased the snapping distance (for
> > touch-resizing) used by existing snapping code.
> >
> > https://codereview.chromium.org/15008002/

[sky, 2013-06-07 16:22:28]

On Thu, Jun 6, 2013 at 12:12 PM,  <mohsen@chromium.org> wrote:
> As far as I understand, MagneticallySnapResizeToOtherWindows() performs
> (only)
> the magnetic snapping during a resize.

It does a resize. Say you drag the left edge of a window close to the
right edge of another window MagneticallySnapResizeToOtherWindows()
then resizes the window so the two edges are aligned.

> That's not what I want to do! I want
> the
> border to catch up the finger (as I described earlier) and have found
> WindowResizer::CalculateBoundsForDrag() the most appropriate place to do
> that.
> Do you have any other opinion?

It's my understanding this bug about resizing. The title says "Unable
to touch-resize". Maybe I'm missing something?

  -Scott

>
>
> On 2013/06/06 15:27:42, sky wrote:
>>
>> MagneticallySnapResizeToOtherWindows handles a resize. Isn't that what
>> you care about?
>
>
>> On Wed, Jun 5, 2013 at 9:20 AM,  <mailto:mohsen@chromium.org> wrote:
>> >
>> >
>> >
https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc
>> > File ash/wm/window_resizer.cc (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc#...
>>
>> > ash/wm/window_resizer.cc:280: void
>> > WindowResizer::AdjustDeltaForTouch(const Details& details,
>> > On 2013/06/04 21:18:55, sky wrote:
>> >>
>> >> Is all of this to snap the edge to the edge of the screen? Do we only
>> >
>> > care about
>> >>
>> >> workspace windows or all windows? It seems like we should be able to
>> >
>> > share some
>> >>
>> >> of the code in WorkspaceWindowResizer that snaps resizes. Something
>> >
>> > like
>> >>
>> >> MagneticallySnapResizeToOtherWindows but that snaps to the screen
>> >
>> > edges.
>> >
>> > This code is not for snapping. Its purpose is to move window border
>> > faster than the finger (when necessary) so that the border catches up
>> > the finger at the edge of the screen (see:
>> > https://code.google.com/p/chromium/issues/detail?id=151168#c6). For the
>> > snapping part, I have just increased the snapping distance (for
>> > touch-resizing) used by existing snapping code.
>> >
>> > https://codereview.chromium.org/15008002/
>
>
>
>
> https://codereview.chromium.org/15008002/

[mohsen, 2013-06-07 17:09:19]

On 2013/06/07 16:22:28, sky wrote:
> On Thu, Jun 6, 2013 at 12:12 PM,  <mailto:mohsen@chromium.org> wrote:
> > As far as I understand, MagneticallySnapResizeToOtherWindows() performs
> > (only)
> > the magnetic snapping during a resize.
> 
> It does a resize. Say you drag the left edge of a window close to the
> right edge of another window MagneticallySnapResizeToOtherWindows()
> then resizes the window so the two edges are aligned.

It does resizing, but only the part related to the snapping. The rest of
resizing is done in other parts of code (e.g. CalculateBoudnsForDrag() I
mentioned earlier).

> > That's not what I want to do! I want
> > the
> > border to catch up the finger (as I described earlier) and have found
> > WindowResizer::CalculateBoundsForDrag() the most appropriate place to do
> > that.
> > Do you have any other opinion?
> 
> It's my understanding this bug about resizing. The title says "Unable
> to touch-resize". Maybe I'm missing something?
The bug is about resizing and all I said is in the context of resizing: when
resizing using touch, normally the finger is a bit ahead of window border, so
when the finger reaches the edge of the screen, the window border is several
pixels behind. I'm trying to correct this behavior as described in the CL
description.

> 
>   -Scott
> 
> >
> >
> > On 2013/06/06 15:27:42, sky wrote:
> >>
> >> MagneticallySnapResizeToOtherWindows handles a resize. Isn't that what
> >> you care about?
> >
> >
> >> On Wed, Jun 5, 2013 at 9:20 AM,  <mailto:mohsen@chromium.org> wrote:
> >> >
> >> >
> >> >
> https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc
> >> > File ash/wm/window_resizer.cc (right):
> >> >
> >> >
> >
> >
> >
>
https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc#...
> >>
> >> > ash/wm/window_resizer.cc:280: void
> >> > WindowResizer::AdjustDeltaForTouch(const Details& details,
> >> > On 2013/06/04 21:18:55, sky wrote:
> >> >>
> >> >> Is all of this to snap the edge to the edge of the screen? Do we only
> >> >
> >> > care about
> >> >>
> >> >> workspace windows or all windows? It seems like we should be able to
> >> >
> >> > share some
> >> >>
> >> >> of the code in WorkspaceWindowResizer that snaps resizes. Something
> >> >
> >> > like
> >> >>
> >> >> MagneticallySnapResizeToOtherWindows but that snaps to the screen
> >> >
> >> > edges.
> >> >
> >> > This code is not for snapping. Its purpose is to move window border
> >> > faster than the finger (when necessary) so that the border catches up
> >> > the finger at the edge of the screen (see:
> >> > https://code.google.com/p/chromium/issues/detail?id=151168#c6). For the
> >> > snapping part, I have just increased the snapping distance (for
> >> > touch-resizing) used by existing snapping code.
> >> >
> >> > https://codereview.chromium.org/15008002/
> >
> >
> >
> >
> > https://codereview.chromium.org/15008002/

[sky, 2013-06-07 19:26:30]

https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc#...
ash/wm/window_resizer.cc:280: void WindowResizer::AdjustDeltaForTouch(const
Details& details,
On 2013/06/05 16:20:00, mohsen wrote:
> On 2013/06/04 21:18:55, sky wrote:
> > Is all of this to snap the edge to the edge of the screen? Do we only care
> about
> > workspace windows or all windows? It seems like we should be able to share
> some
> > of the code in WorkspaceWindowResizer that snaps resizes. Something like
> > MagneticallySnapResizeToOtherWindows but that snaps to the screen edges.
> 
> This code is not for snapping. Its purpose is to move window border faster
than
> the finger (when necessary) so that the border catches up the finger at the
edge
> of the screen (see:
> https://code.google.com/p/chromium/issues/detail?id=151168#c6). For the
snapping
> part, I have just increased the snapping distance (for touch-resizing) used by
> existing snapping code.

This seems disconcerting and while I get what Dana is suggesting I don't see the
point of it. Aren't the changes in worskpacewindowresize all that is necessary
to fix the bug? If you prefer, I can bring this discussion back to the bug, or
email Alex.

https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc#...
ash/wm/window_resizer.cc:280: void WindowResizer::AdjustDeltaForTouch(const
Details& details,
On 2013/06/05 16:20:00, mohsen wrote:
> On 2013/06/04 21:18:55, sky wrote:
> > Is all of this to snap the edge to the edge of the screen? Do we only care
> about
> > workspace windows or all windows? It seems like we should be able to share
> some
> > of the code in WorkspaceWindowResizer that snaps resizes. Something like
> > MagneticallySnapResizeToOtherWindows but that snaps to the screen edges.
> 
> This code is not for snapping. Its purpose is to move window border faster
than
> the finger (when necessary) so that the border catches up the finger at the
edge
> of the screen (see:
> https://code.google.com/p/chromium/issues/detail?id=151168#c6). For the
snapping
> part, I have just increased the snapping distance (for touch-resizing) used by
> existing snapping code.

Aren't the changes in WorkspaceWindowResize enough to fix this bug? I'm a bit
skeptical of slowly shifting the position as you drag, that seems a bit odd. In
particular I'm worried the slight shift as dragging is going to make it so it
feels as though the border is shifting under the finger. I'm happy to continue
this discussion in the bug or email Alex if you prefer.

[mohsen, 2013-06-07 20:06:57]

https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc#...
ash/wm/window_resizer.cc:280: void WindowResizer::AdjustDeltaForTouch(const
Details& details,
On 2013/06/07 19:26:30, sky wrote:
> On 2013/06/05 16:20:00, mohsen wrote:
> > On 2013/06/04 21:18:55, sky wrote:
> > > Is all of this to snap the edge to the edge of the screen? Do we only care
> > about
> > > workspace windows or all windows? It seems like we should be able to share
> > some
> > > of the code in WorkspaceWindowResizer that snaps resizes. Something like
> > > MagneticallySnapResizeToOtherWindows but that snaps to the screen edges.
> > 
> > This code is not for snapping. Its purpose is to move window border faster
> than
> > the finger (when necessary) so that the border catches up the finger at the
> edge
> > of the screen (see:
> > https://code.google.com/p/chromium/issues/detail?id=151168#c6). For the
> snapping
> > part, I have just increased the snapping distance (for touch-resizing) used
by
> > existing snapping code.
> 
> Aren't the changes in WorkspaceWindowResize enough to fix this bug? I'm a bit
> skeptical of slowly shifting the position as you drag, that seems a bit odd.
In
> particular I'm worried the slight shift as dragging is going to make it so it
> feels as though the border is shifting under the finger. I'm happy to continue
> this discussion in the bug or email Alex if you prefer.

The change in WorkspaceWindowResizer increases snapping distance from 8 to 16
for touch. Even with this increase we should try hard to drag as close to the
window border as possible to move the border close enough to the edge to trigger
the snapping. That's where the slight shift helps a lot. Anyway, I'm fine with
either discussing under the bug or emailing Alex, whichever you think is more
appropriate.

[sky, 2013-06-07 20:43:08]

I'll email Alex.

How about making this patch just the change to increase size to 16
until we resolve the other issue?

On Fri, Jun 7, 2013 at 1:06 PM,  <mohsen@chromium.org> wrote:
>
> https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc
> File ash/wm/window_resizer.cc (right):
>
>
https://codereview.chromium.org/15008002/diff/43001/ash/wm/window_resizer.cc#...
> ash/wm/window_resizer.cc:280: void
> WindowResizer::AdjustDeltaForTouch(const Details& details,
> On 2013/06/07 19:26:30, sky wrote:
>>
>> On 2013/06/05 16:20:00, mohsen wrote:
>> > On 2013/06/04 21:18:55, sky wrote:
>> > > Is all of this to snap the edge to the edge of the screen? Do we
>
> only care
>>
>> > about
>> > > workspace windows or all windows? It seems like we should be able
>
> to share
>>
>> > some
>> > > of the code in WorkspaceWindowResizer that snaps resizes.
>
> Something like
>>
>> > > MagneticallySnapResizeToOtherWindows but that snaps to the screen
>
> edges.
>>
>> >
>> > This code is not for snapping. Its purpose is to move window border
>
> faster
>>
>> than
>> > the finger (when necessary) so that the border catches up the finger
>
> at the
>>
>> edge
>> > of the screen (see:
>> > https://code.google.com/p/chromium/issues/detail?id=151168#c6). For
>
> the
>>
>> snapping
>> > part, I have just increased the snapping distance (for
>
> touch-resizing) used by
>>
>> > existing snapping code.
>
>
>> Aren't the changes in WorkspaceWindowResize enough to fix this bug?
>
> I'm a bit
>>
>> skeptical of slowly shifting the position as you drag, that seems a
>
> bit odd. In
>>
>> particular I'm worried the slight shift as dragging is going to make
>
> it so it
>>
>> feels as though the border is shifting under the finger. I'm happy to
>
> continue
>>
>> this discussion in the bug or email Alex if you prefer.
>
>
> The change in WorkspaceWindowResizer increases snapping distance from 8
> to 16 for touch. Even with this increase we should try hard to drag as
> close to the window border as possible to move the border close enough
> to the edge to trigger the snapping. That's where the slight shift helps
> a lot. Anyway, I'm fine with either discussing under the bug or emailing
> Alex, whichever you think is more appropriate.
>
> https://codereview.chromium.org/15008002/

[mohsen, 2013-06-11 18:00:04]

sky@: Please take another look; I have:
 1. used only snapping (32 pixels) to solve the problem, no more extra shift of
border to catch up the finger
 2. Passed the source of the resize (touch or mouse) instead of fetching
current_event(). This caused a lot of changes in different places.

[mohsen, 2013-06-12 21:39:53]

Updated as described in the description. Please take a look....

[sky, 2013-06-14 15:57:27]

https://codereview.chromium.org/15008002/diff/84001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/84001/ash/wm/window_resizer.cc#...
ash/wm/window_resizer.cc:283: details.initial_bounds_in_parent.right();
nit: indent this 2 more (same on 286, 292 and 295).

https://codereview.chromium.org/15008002/diff/84001/ash/wm/window_resizer.h
File ash/wm/window_resizer.h (right):

https://codereview.chromium.org/15008002/diff/84001/ash/wm/window_resizer.h#n...
ash/wm/window_resizer.h:107: static void AdjustDeltaForTouchResize(const
Details& details, int* delta_x,
nit: when you wrap put each param on its own line.

https://codereview.chromium.org/15008002/diff/84001/ash/wm/window_resizer.h#n...
ash/wm/window_resizer.h:107: static void AdjustDeltaForTouchResize(const
Details& details, int* delta_x,
Can this be private?

https://codereview.chromium.org/15008002/diff/84001/ash/wm/window_resizer.h#n...
ash/wm/window_resizer.h:107: static void AdjustDeltaForTouchResize(const
Details& details, int* delta_x,
Can this be private?

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:88: const int
kScreenEdgeInsetForTouchResize = 16;
I thought we were going to make this 32?

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:339: sticky_size =
kScreenEdgeInsetForTouchResize;
Do you want this only effect snapping to screen bounds?

[mohsen, 2013-06-14 16:21:48]

https://codereview.chromium.org/15008002/diff/84001/ash/wm/window_resizer.cc
File ash/wm/window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/84001/ash/wm/window_resizer.cc#...
ash/wm/window_resizer.cc:283: details.initial_bounds_in_parent.right();
On 2013/06/14 15:57:27, sky wrote:
> nit: indent this 2 more (same on 286, 292 and 295).

Done.

https://codereview.chromium.org/15008002/diff/84001/ash/wm/window_resizer.h
File ash/wm/window_resizer.h (right):

https://codereview.chromium.org/15008002/diff/84001/ash/wm/window_resizer.h#n...
ash/wm/window_resizer.h:107: static void AdjustDeltaForTouchResize(const
Details& details, int* delta_x,
On 2013/06/14 15:57:27, sky wrote:
> Can this be private?

Yes. It should be. Done.

https://codereview.chromium.org/15008002/diff/84001/ash/wm/window_resizer.h#n...
ash/wm/window_resizer.h:107: static void AdjustDeltaForTouchResize(const
Details& details, int* delta_x,
On 2013/06/14 15:57:27, sky wrote:
> nit: when you wrap put each param on its own line.

Done. I thought that only if no parameter fits in the first line then all should
be on their own line.

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:88: const int
kScreenEdgeInsetForTouchResize = 16;
On 2013/06/14 15:57:27, sky wrote:
> I thought we were going to make this 32?

32 was for the case that we only wanted to use snapping. But now that we align
border immediately with touch, 16 is enough. But, we can change it to 32 if you
think that's better?

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:339: sticky_size =
kScreenEdgeInsetForTouchResize;
On 2013/06/14 15:57:27, sky wrote:
> Do you want this only effect snapping to screen bounds?

The problem only happens for resizing to the screen edge. Snapping to other
windows works with the current configuration. Do you think we have better be
consistent in both cases?

[sky, 2013-06-14 16:25:07]

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:88: const int
kScreenEdgeInsetForTouchResize = 16;
On 2013/06/14 16:21:48, mohsen wrote:
> On 2013/06/14 15:57:27, sky wrote:
> > I thought we were going to make this 32?
> 
> 32 was for the case that we only wanted to use snapping. But now that we align
> border immediately with touch, 16 is enough. But, we can change it to 32 if
you
> think that's better?

I only mention it since I thought we agreed on 32 in the email thread. Maybe I'm
wrong?

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:339: sticky_size =
kScreenEdgeInsetForTouchResize;
On 2013/06/14 16:21:48, mohsen wrote:
> On 2013/06/14 15:57:27, sky wrote:
> > Do you want this only effect snapping to screen bounds?
> 
> The problem only happens for resizing to the screen edge. Snapping to other
> windows works with the current configuration. Do you think we have better be
> consistent in both cases?

My point is that doing the change here effects *all* cases, not just the one you
care about. I don't necessarily know if this is a problem. I'm just pointing it
out. It seems reasonable to increase the size for everything during touch, which
matches what you're doing here. But I don't know how that feels in practice. I
raise the issue to make sure you understand the impact of your change.

[mohsen, 2013-06-14 18:17:17]

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:88: const int
kScreenEdgeInsetForTouchResize = 16;
On 2013/06/14 16:25:07, sky wrote:
> On 2013/06/14 16:21:48, mohsen wrote:
> > On 2013/06/14 15:57:27, sky wrote:
> > > I thought we were going to make this 32?
> > 
> > 32 was for the case that we only wanted to use snapping. But now that we
align
> > border immediately with touch, 16 is enough. But, we can change it to 32 if
> you
> > think that's better?
> 
> I only mention it since I thought we agreed on 32 in the email thread. Maybe
I'm
> wrong?

Well, I thought that was for when we only have snapping. Anyway, I changed it
back to 32.

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:339: sticky_size =
kScreenEdgeInsetForTouchResize;
On 2013/06/14 16:25:07, sky wrote:
> On 2013/06/14 16:21:48, mohsen wrote:
> > On 2013/06/14 15:57:27, sky wrote:
> > > Do you want this only effect snapping to screen bounds?
> > 
> > The problem only happens for resizing to the screen edge. Snapping to other
> > windows works with the current configuration. Do you think we have better be
> > consistent in both cases?
> 
> My point is that doing the change here effects *all* cases, not just the one
you
> care about. I don't necessarily know if this is a problem. I'm just pointing
it
> out. It seems reasonable to increase the size for everything during touch,
which
> matches what you're doing here. But I don't know how that feels in practice. I
> raise the issue to make sure you understand the impact of your change.

I think this sticy_size is only used for snapping to screen edges. The one used
for other windows is MagnetismMathcer::kMagneticDistance
(https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/workspace/m...)
used here:
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/workspace/m....
Also my tests show that this only affects snapping to screen edge.

[sky, 2013-06-14 19:22:21]

LGTM

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_window_resizer.cc (right):

https://codereview.chromium.org/15008002/diff/84001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_window_resizer.cc:339: sticky_size =
kScreenEdgeInsetForTouchResize;
On 2013/06/14 18:17:17, mohsen wrote:
> On 2013/06/14 16:25:07, sky wrote:
> > On 2013/06/14 16:21:48, mohsen wrote:
> > > On 2013/06/14 15:57:27, sky wrote:
> > > > Do you want this only effect snapping to screen bounds?
> > > 
> > > The problem only happens for resizing to the screen edge. Snapping to
other
> > > windows works with the current configuration. Do you think we have better
be
> > > consistent in both cases?
> > 
> > My point is that doing the change here effects *all* cases, not just the one
> you
> > care about. I don't necessarily know if this is a problem. I'm just pointing
> it
> > out. It seems reasonable to increase the size for everything during touch,
> which
> > matches what you're doing here. But I don't know how that feels in practice.
I
> > raise the issue to make sure you understand the impact of your change.
> 
> I think this sticy_size is only used for snapping to screen edges. The one
used
> for other windows is MagnetismMathcer::kMagneticDistance
>
(https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/workspace/m...)
> used here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/workspace/m....
> Also my tests show that this only affects snapping to screen edge.

Ah yes, you are right. Sorry about that.

[commit-bot: I haz the power, 2013-06-17 18:02:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mohsen@chromium.org/15008002/109001

[commit-bot: I haz the power, 2013-06-17 19:43:46]

Change committed as 206776