MD Settings: Display: Add snapping and collisions

MD Settings: Display: Add snapping and collisions

Note: This does not correctly resolve children of dragged displays yet,
it just carries them along which can result in overlapping.

This also:
* Moves the layout code into a separate behavior
* Uses Map instead of Object for bounds and layout maps
* Uses the id instead of the index when selecting a display (more robust)
* Fixes the calculateVisualScale behavior

BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/c7f3b6cec255c73f4f3b1473bc14c4da547c0399
Cr-Commit-Position: refs/heads/master@{#403500}

[stevenjb, 2016-06-24 21:02:14]

Description was changed from

==========
MD Settings: Display: Add snapping and collisions

Note: This does not correctly resolve children of dragged displays yet,
it just carries them along which can result in overlapping.

BUG=547080
==========

to

==========
MD Settings: Display: Add snapping and collisions

Note: This does not correctly resolve children of dragged displays yet,
it just carries them along which can result in overlapping.

BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[stevenjb, 2016-06-24 21:24:53]

stevenjb@chromium.org changed reviewers:
+ michaelpg@chromium.org

[stevenjb, 2016-06-24 21:24:53]

Display layout 2/3

This is the biggest piece. The logic is *mostly* copied from
display_layout_manager_multi.js, but the structure is different enough that the
diff is unfortunately unhelpful.

FWIW, the overall logic is basically the same as what we are currently shipping
on Options.

[stevenjb, 2016-06-24 21:26:33]

Description was changed from

==========
MD Settings: Display: Add snapping and collisions

Note: This does not correctly resolve children of dragged displays yet,
it just carries them along which can result in overlapping.

BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Display: Add snapping and collisions

Note: This does not correctly resolve children of dragged displays yet,
it just carries them along which can result in overlapping.

This also:
* Moves the layout code into a separate behavior
* Uses Map instead of Object for bounds and layout maps
* Uses the id instead of the index when selecting a display (more robust)
* Fixes the calculateVisualScale behavior


BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[michaelpg, 2016-06-27 22:09:32]

Awesome. Some nits, and I'm a little suspicious of the collision logic (but if
it's already been reviewed elsewhere, no point spending time on it again).

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display_layout.js (right):

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.js:47: /** @type
{!{left: number, top: number}} */
still @private, right?

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/layout_behavior.js (right):

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:85: // Snap to
the closet edge
"closest edge."

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:228: if
(opt_ignoreIds && opt_ignoreIds.indexOf(otherId) != -1)
replace "indexOf(otherId) != -1" with "includes(otherId)"
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:265:
getlayoutPositionForBounds_: function(bounds, parentId) {
capital L in getLayout

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:332: * @param
{!chrome.system.display.Bounds} newBounds
lowercase snapToEdge

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:452: deltaPos.y
= 0;
Is this logic copied from Options? It's odd that there are cases where, instead
of snapping to an edge, the delta is reset to 0.

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:452: deltaPos.y
= 0;
opt nit: put this at the top of the block, like line 454 is (makes it clearer
that only one point is being calculated, IMO)

[stevenjb, 2016-06-27 23:25:47]

So I did some extra testing to verify my cleanup of the snap logic and found an
unrelated bug in the translation so I fixed that. So thanks! :)

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/display_layout.js (right):

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/display_layout.js:47: /** @type
{!{left: number, top: number}} */
On 2016/06/27 22:09:32, michaelpg wrote:
> still @private, right?

Yeah, Merge. Done.

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/layout_behavior.js (right):

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:85: // Snap to
the closet edge
On 2016/06/27 22:09:32, michaelpg wrote:
> "closest edge."

Out of the closet, edge...
Done

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:228: if
(opt_ignoreIds && opt_ignoreIds.indexOf(otherId) != -1)
On 2016/06/27 22:09:32, michaelpg wrote:
> replace "indexOf(otherId) != -1" with "includes(otherId)"
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

Nice. Done.

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:265:
getlayoutPositionForBounds_: function(bounds, parentId) {
On 2016/06/27 22:09:32, michaelpg wrote:
> capital L in getLayout

Done.

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:332: * @param
{!chrome.system.display.Bounds} newBounds
On 2016/06/27 22:09:32, michaelpg wrote:
> lowercase snapToEdge

Done.

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:452: deltaPos.y
= 0;
On 2016/06/27 22:09:32, michaelpg wrote:
> Is this logic copied from Options? It's odd that there are cases where,
instead
> of snapping to an edge, the delta is reset to 0.

It was copied, but that doesn't mean it's entirely correct. I'm not sure what
you mean though. We set the delta to 0 when we would otherwise invert the sign
of the delta, i.e. we don't want to turn a drag right into a drag left, we just
want to limit the drag. I'll try making that more clear with an extra
intermediate delta.

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:452: deltaPos.y
= 0;
On 2016/06/27 22:09:32, michaelpg wrote:
> opt nit: put this at the top of the block, like line 454 is (makes it clearer
> that only one point is being calculated, IMO)

It was thinking that x then y was more clear, but sure, I'm fine either way.
Done.

[michaelpg, 2016-06-29 16:42:08]

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/layout_behavior.js (right):

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:85: // Snap to
the closet edge
On 2016/06/27 23:25:47, stevenjb wrote:
> On 2016/06/27 22:09:32, michaelpg wrote:
> > "closest edge."
> 
> Out of the closet, edge...
> Done

(also the period.)

https://codereview.chromium.org/2097763004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/layout_behavior.js (right):

https://codereview.chromium.org/2097763004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:420: *
|deltaPos| to limit movement to a single axis and avoid the collision
add a comment that this happens after snapping, ie is intended more to prevent
collision with other displays besides the one it's already attached to

https://codereview.chromium.org/2097763004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:459: let y =
otherBounds.top + otherBounds.top;
height?

[michaelpg, 2016-06-29 19:58:46]

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/layout_behavior.js (right):

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:99: if
(layoutPosition != this.draglayoutPosition_ ||
dragLayoutPosition_ (as in properties) here & elsewhere

[stevenjb, 2016-06-29 22:34:41]

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/layout_behavior.js (right):

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:85: // Snap to
the closet edge
On 2016/06/29 16:42:07, michaelpg wrote:
> On 2016/06/27 23:25:47, stevenjb wrote:
> > On 2016/06/27 22:09:32, michaelpg wrote:
> > > "closest edge."
> > 
> > Out of the closet, edge...
> > Done
> 
> (also the period.)

Done.

https://codereview.chromium.org/2097763004/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:99: if
(layoutPosition != this.draglayoutPosition_ ||
On 2016/06/29 19:58:46, michaelpg wrote:
> dragLayoutPosition_ (as in properties) here & elsewhere

Doh. I think I fixed this before but lost it in the rebase since the code was
moved. Done.

https://codereview.chromium.org/2097763004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/device_page/layout_behavior.js (right):

https://codereview.chromium.org/2097763004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:420: *
|deltaPos| to limit movement to a single axis and avoid the collision
On 2016/06/29 16:42:08, michaelpg wrote:
> add a comment that this happens after snapping, ie is intended more to prevent
> collision with other displays besides the one it's already attached to

Done.

https://codereview.chromium.org/2097763004/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/device_page/layout_behavior.js:459: let y =
otherBounds.top + otherBounds.top;
On 2016/06/29 16:42:08, michaelpg wrote:
> height?

Doh! Thanks. Done.

[michaelpg, 2016-07-01 18:11:52]

lgtm

[stevenjb, 2016-07-01 18:35:53]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2016-07-01 18:36:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-01 19:24:28]

Description was changed from

==========
MD Settings: Display: Add snapping and collisions

Note: This does not correctly resolve children of dragged displays yet,
it just carries them along which can result in overlapping.

This also:
* Moves the layout code into a separate behavior
* Uses Map instead of Object for bounds and layout maps
* Uses the id instead of the index when selecting a display (more robust)
* Fixes the calculateVisualScale behavior


BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Display: Add snapping and collisions

Note: This does not correctly resolve children of dragged displays yet,
it just carries them along which can result in overlapping.

This also:
* Moves the layout code into a separate behavior
* Uses Map instead of Object for bounds and layout maps
* Uses the id instead of the index when selecting a display (more robust)
* Fixes the calculateVisualScale behavior


BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-07-01 19:24:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-01 19:24:32]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-01 19:26:42]

Description was changed from

==========
MD Settings: Display: Add snapping and collisions

Note: This does not correctly resolve children of dragged displays yet,
it just carries them along which can result in overlapping.

This also:
* Moves the layout code into a separate behavior
* Uses Map instead of Object for bounds and layout maps
* Uses the id instead of the index when selecting a display (more robust)
* Fixes the calculateVisualScale behavior


BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Display: Add snapping and collisions

Note: This does not correctly resolve children of dragged displays yet,
it just carries them along which can result in overlapping.

This also:
* Moves the layout code into a separate behavior
* Uses Map instead of Object for bounds and layout maps
* Uses the id instead of the index when selecting a display (more robust)
* Fixes the calculateVisualScale behavior

BUG=547080
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/c7f3b6cec255c73f4f3b1473bc14c4da547c0399
Cr-Commit-Position: refs/heads/master@{#403500}
==========

[commit-bot: I haz the power, 2016-07-01 19:26:43]

Patchset 5 (id:??) landed as
https://crrev.com/c7f3b6cec255c73f4f3b1473bc14c4da547c0399
Cr-Commit-Position: refs/heads/master@{#403500}