MD Settings: Display: Reparent children of dragged displays

chrome/browser/resources/settings/device_page/layout_behavior.js

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview Behavior for handling display layout, specifically
  *     edge snapping and collisions.
  */
 
 /** @polymerBehavior */
@@ skipping 62 matching lines @@
    *     bounds for the display.
    * @return {!chrome.system.display.Bounds}
    */
   updateDisplayBounds(id, newBounds) {
     this.dragLayoutId = id;
 
     // Find the closest parent.
     var closestId = this.findClosest_(id, newBounds);
 
     // Find the closest edge.
-    var layoutPosition = this.getlayoutPositionForBounds_(newBounds, closestId);
+    var closestBounds = this.getCalculatedDisplayBounds(closestId);
+    var layoutPosition =
+        this.getLayoutPositionForBounds_(newBounds, closestBounds);
 
     // Snap to the closet edge
-    this.snapBounds_(closestId, layoutPosition, newBounds);
+    var snapPos = this.snapBounds_(newBounds, closestId, layoutPosition);
+    newBounds.left = snapPos.x;
+    newBounds.top = snapPos.y;
 
     // Calculate the new bounds and delta.
     var oldBounds = this.dragBounds_ || this.getCalculatedDisplayBounds(id);
     var deltaPos = {
       x: newBounds.left - oldBounds.left,
       y: newBounds.top - oldBounds.top
     };
 
     // Check for collisions.
     this.collideAndModifyDelta_(id, oldBounds, deltaPos);
@@ skipping 15 matching lines @@
   },
 
   /**
    * Called when dragging ends. Sends the updated layout to chrome.
    * @param {string} id
    */
   finishUpdateDisplayBounds(id) {
     this.highlightEdge_('', undefined);  // Remove any highlights.
     if (id != this.dragLayoutId || !this.dragBounds_)
       return;
+
     var layout = this.displayLayoutMap_.get(id);
-    if (!layout)
-      return;
-    // Note: This updates layout in this.displayLayoutMap_ which is also the
-    // entry in this.layouts.
-    this.updateOffsetAndPosition_(
-        this.dragBounds_, this.draglayoutPosition_, layout);
+
+    var orphanIds;
+    if (!layout || layout.parentId == '') {
+      // Primary display. Set the calculated position to |dragBounds_|.
+      this.setCalculatedDisplayBounds_(id, this.dragBounds_);
+
+      // We cannot re-parent the primary display, so instead make all other
+      // displays orphans and clear their calculated bounds.
+      orphanIds = this.findChildren_(id, true /* recurse */);
+      for (let o in orphanIds)
+        this.calculatedBoundsMap_.delete(o);
+
+      // Re-parent |dragParentId_|. It will be forced to parent to the dragged
+      // display since it is the only non-orphan.
+      this.reparentOrphan_(this.dragParentId_, orphanIds);
+      orphanIds.splice(orphanIds.indexOf(this.dragParentId_), 1);
+    } else {
+      // All immediate children of |layout| will need to be re-parented.
+      orphanIds = this.findChildren_(id, false /* do not recurse */);
+      for (let o in orphanIds)
+        this.calculatedBoundsMap_.delete(o);
+
+      // When re-parenting to a descendant, also parent any immediate child to
+      // drag display's current parent.
+      var topLayout = this.displayLayoutMap_.get(this.dragParentId_);
+      while (topLayout && topLayout.parentId != '') {
+        if (topLayout.parentId == id) {
+          topLayout.parentId = layout.parentId;
+          break;
+        }
+        topLayout = this.displayLayoutMap_.get(topLayout.parentId);
+      }
+
+      // Re-parent the dragged display

[michaelpg, 2016/06/29 20:16:50]

nit: period

[stevenjb, 2016/06/29 23:25:30]

Done.

+      layout.parentId = this.dragParentId_;
+      this.updateOffsetAndPosition_(
+          this.dragBounds_, this.draglayoutPosition_, layout);
+    }
+
+    // Update any orphaned children. This may cause the dragged display to
+    // be re-attached if it was attached to a child.
+    this.updateOrphans_(orphanIds);
 
     // Send the updated layouts.
     chrome.system.display.setDisplayLayout(this.layouts, function() {
       if (chrome.runtime.lastError) {
         console.error(
             'setDisplayLayout Error: ' + chrome.runtime.lastError.message);
       }
     });
   },
 
@@ skipping 14 matching lines @@
    */
   setCalculatedDisplayBounds_: function(displayId, bounds) {
     assert(bounds);
     this.calculatedBoundsMap_.set(
         displayId,
         /** @type {!chrome.system.display.Bounds} */ (
             Object.assign({}, bounds)));
   },
 
   /**
+   * Re-parent all entries in |orphanIds| and any children.

[michaelpg, 2016/06/29 20:16:51]

nit: indicative ("Re-parents") here & below

[stevenjb, 2016/06/29 23:25:29]

Done.

+   * @param {Array<string>} orphanIds The list of ids affected by the move.

[michaelpg, 2016/06/29 20:16:51]

nit: !Array<string>

[stevenjb, 2016/06/29 23:25:29]

Done.

+   * @private
+   */
+  updateOrphans_: function(orphanIds) {
+    var orphans = orphanIds.slice();
+    for (let orphan of orphanIds) {
+      var newOrphans = this.findChildren_(orphan, true /* recurse */);
+      // If the dragged display was re-parented to one of its children,
+      // there may be duplicates so merge the lists.
+      for (let o of newOrphans) {
+        if (orphans.indexOf(o) == -1)

[michaelpg, 2016/06/29 20:16:51]

!orphans.includes(o)

also, consider using a Set if order doesn't matter

[stevenjb, 2016/06/29 23:25:30]

Done. (Order matters, see findChildren).

+          orphans.push(o);
+      }
+    }
+
+    // Remove each orphan from the list as it is re-parented so that
+    // subsequent orphans can be parented to it.
+    while (orphans.length) {
+      var orphanId = orphans.shift();
+      this.reparentOrphan_(orphanId, orphans);
+    }
+  },
+
+  /**
+   * Re-parent the orphan to a layout that is not a member of
+   * |otherOrphanIds|.
+   * @param {string} orphanId The id of the orphan to re-parent.
+   * @param {Array<string>} otherOrphanIds The list of ids of other orphans
+   *     to ignore when re-parenting.
+   * @private
+   */
+  reparentOrphan_: function(orphanId, otherOrphanIds) {
+    var layout = this.displayLayoutMap_.get(orphanId);

[michaelpg, 2016/06/29 20:16:51]

opt nit: var layout = assert(...);

[stevenjb, 2016/06/29 23:25:30]

I'm not a fan of that pattern outside of tests.

[Dan Beam, 2016/06/29 23:35:10]

why?

[stevenjb, 2016/06/29 23:38:33]

In C++ I've seen bugs where someone puts code with side effects in a CHECK then
later someone changes that toa DCHECK. I realize we don't have that issue with
assert, but even so I avoid putting code with side effects inside asserts.

[michaelpg, 2016/07/02 00:53:27]

Acknowledged.

+    assert(layout);
+    if (orphanId == this.dragId_ && layout.parentId != '') {
+      this.setCalculatedDisplayBounds_(orphanId, this.dragBounds_);
+      return;
+    }
+    var bounds = this.getCalculatedDisplayBounds(orphanId);
+
+    // Find the closest parent.
+    var newParentId = this.findClosest_(orphanId, bounds, otherOrphanIds);
+    assert(newParentId != '');
+    layout.parentId = newParentId;
+
+    // Find the closest edge.
+    var parentBounds = this.getCalculatedDisplayBounds(newParentId);
+    var layoutPosition = this.getLayoutPositionForBounds_(bounds, parentBounds);
+
+    // Move from the nearest corner to the desired locaiton and get the delta.

[michaelpg, 2016/06/29 20:16:51]

location

[stevenjb, 2016/06/29 23:25:30]

Done.

+    var cornerBounds = this.getCornerBounds_(bounds, parentBounds);
+    var desiredPos = this.snapBounds_(bounds, newParentId, layoutPosition);
+    var deltaPos = {
+      x: desiredPos.x - cornerBounds.left,
+      y: desiredPos.y - cornerBounds.top
+    };
+
+    // Check for collisions.
+    this.collideAndModifyDelta_(orphanId, cornerBounds, deltaPos);
+    var desiredBounds = {
+      left: cornerBounds.left + deltaPos.x,
+      top: cornerBounds.top + deltaPos.y,
+      width: bounds.width,
+      height: bounds.height
+    };
+
+    this.updateOffsetAndPosition_(desiredBounds, layoutPosition, layout);
+  },
+
+  /**
+   * @param {string} parentId
+   * @param {boolean} recurse Include descendants of children.
+   * @return {!Array<string>}
+   * @private
+   */
+  findChildren_: function(parentId, recurse) {
+    var children = [];
+    for (let childId of this.displayLayoutMap_.keys()) {
+      if (childId == parentId)
+        continue;
+      if (this.displayLayoutMap_.get(childId).parentId == parentId) {
+        // Insert immediate children at the front of the array.
+        children.unshift(childId);
+        if (recurse) {
+          // Descendants get added to the end of the list.
+          children = children.concat(this.findChildren_(childId, true));
+        }
+      }
+    }
+    return children;
+  },
+
+  /**
    * Recursively calculate the absolute bounds of a display.
    * Caches the display bounds so that parent bounds are only calculated once.
    * @param {string} id
    * @param {number} width
    * @param {number} height
    * @private
    */
   calculateBounds_: function(id, width, height) {
     var left, top;
     var layout = this.displayLayoutMap_.get(id);
@@ skipping 79 matching lines @@
         closestId = otherId;
         closestDelta2 = delta2;
       }
     }
     return closestId;
   },
 
   /**
    * Calculates the LayoutPosition for |bounds| relative to |parentId|.
    * @param {!chrome.system.display.Bounds} bounds
-   * @param {string} parentId
+   * @param {!chrome.system.display.Bounds} parentBounds
    * @return {!chrome.system.display.LayoutPosition}
    */
-  getlayoutPositionForBounds_: function(bounds, parentId) {
+  getLayoutPositionForBounds_: function(bounds, parentBounds) {
     // Translate bounds from top-left to center.
     var x = bounds.left + bounds.width / 2;
     var y = bounds.top + bounds.height / 2;
 
     // Determine the distance from the new bounds to both of the near edges.
-    var parentBounds = this.getCalculatedDisplayBounds(parentId);
     var left = parentBounds.left;
     var top = parentBounds.top;
     var width = parentBounds.width;
     var height = parentBounds.height;
 
-    // Signed deltas to the center of the div.
+    // Signed deltas to the center.
     var dx = x - (left + width / 2);
     var dy = y - (top + height / 2);
 
     // Unsigned distance to each edge.
     var distx = Math.abs(dx) - width / 2;
     var disty = Math.abs(dy) - height / 2;
 
     if (distx > disty) {
       if (dx < 0)
         return chrome.system.display.LayoutPosition.LEFT;
       else
         return chrome.system.display.LayoutPosition.RIGHT;
     } else {
       if (dy < 0)
         return chrome.system.display.LayoutPosition.TOP;
       else
         return chrome.system.display.LayoutPosition.BOTTOM;
     }
   },
 
   /**
    * Modifes |bounds| to the position closest to it along the edge of |parentId|
    * specified by |layoutPosition|.
+   * @param {!chrome.system.display.Bounds} bounds
    * @param {string} parentId
    * @param {!chrome.system.display.LayoutPosition} layoutPosition
-   * @param {!chrome.system.display.Bounds} bounds
+   * @return {!{x: number, y: number}}
    */
-  snapBounds_: function(parentId, layoutPosition, bounds) {
+  snapBounds_: function(bounds, parentId, layoutPosition) {
     var parentBounds = this.getCalculatedDisplayBounds(parentId);
 
     var x;
     if (layoutPosition == chrome.system.display.LayoutPosition.LEFT) {
       x = parentBounds.left - bounds.width;
     } else if (layoutPosition == chrome.system.display.LayoutPosition.RIGHT) {
       x = parentBounds.left + parentBounds.width;
     } else {
       x = this.snapToX_(bounds, parentBounds);
     }
 
     var y;
     if (layoutPosition == chrome.system.display.LayoutPosition.TOP) {
       y = parentBounds.top - bounds.height;
     } else if (layoutPosition == chrome.system.display.LayoutPosition.BOTTOM) {
       y = parentBounds.top + parentBounds.height;
     } else {
       y = this.snapToY_(bounds, parentBounds);
     }
 
-    bounds.left = x;
-    bounds.top = y;
+    return {x: x, y: y};
   },
 
   /**
    * Snaps a horizontal value, see SnapToEdge.
    * @param {!chrome.system.display.Bounds} newBounds
    * @param {!chrome.system.display.Bounds} parentBounds
    * @param {number=} opt_snapDistance Provide to override the snap distance.
    *     0 means snap from any distance.
    * @return {number}
    */
@@ skipping 161 matching lines @@
     /** @const */ var MIN_OFFSET_OVERLAP = 50;
     minOffset += MIN_OFFSET_OVERLAP;
     maxOffset -= MIN_OFFSET_OVERLAP;
     layout.offset = Math.max(minOffset, Math.min(offset, maxOffset));
 
     // Update the calculated bounds to match the new offset.
     this.calculateBounds_(layout.id, bounds.width, bounds.height);
   },
 
   /**
+   * Return the position of the corner of the div closest to |pos|.

[michaelpg, 2016/06/29 20:16:51]

nits: Return => Returns, position => bounds, pos => somethingelse

[stevenjb, 2016/06/29 23:25:30]

Rewrote.

+   * @param {!chrome.system.display.Bounds} bounds
+   * @param {!chrome.system.display.Bounds} parentBounds
+   * @return {!chrome.system.display.Bounds}
+   * @private
+   */
+  getCornerBounds_: function(bounds, parentBounds) {
+    var x;
+    if (bounds.left > parentBounds.left + parentBounds.width / 2)
+      x = parentBounds.left + parentBounds.width;
+    else
+      x = parentBounds.left - bounds.width;
+    var y;
+    if (bounds.top > parentBounds.top + parentBounds.height / 2)
+      y = parentBounds.top + parentBounds.height;
+    else
+      y = parentBounds.top - bounds.height;
+    return {
+      left: x,
+      top: y,
+      width: bounds.width,
+      height: bounds.height,
+    };
+  },
+
+  /**
    * Highlights the edge of the div associated with |id| based on
    * |layoutPosition| and removes any other highlights. If |layoutPosition| is
    * undefined, removes all highlights.
    * @param {string} id
    * @param {chrome.system.display.LayoutPosition|undefined} layoutPosition
    * @private
    */
   highlightEdge_: function(id, layoutPosition) {
     for (let layout of this.layouts) {
       var highlight = (layout.id == id) ? layoutPosition : undefined;
       var div = this.$$('#_' + layout.id);
       div.classList.toggle(
           'highlight-right',
           highlight == chrome.system.display.LayoutPosition.RIGHT);
       div.classList.toggle(
           'highlight-left',
           highlight == chrome.system.display.LayoutPosition.LEFT);
       div.classList.toggle(
           'highlight-top',
           highlight == chrome.system.display.LayoutPosition.TOP);
       div.classList.toggle(
           'highlight-bottom',
           highlight == chrome.system.display.LayoutPosition.BOTTOM);
     }
   },
 };