Bug 923859 - Wide widget rearranging: derecurse, simplify, improve, r=jaws
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Thu, 10 Oct 2013 13:40:44 +0200
changeset 164875 4afd6ba4678dd3506bbc60e8ef68f9da0640d840
parent 164874 70aba2a2ede7cc277436df06a64edf0b80eb0b9b
child 164876 a5c17d687ccfb8a1d58fde490103394da9942df4
push id4703
push userakeybl@mozilla.com
push dateMon, 09 Dec 2013 20:24:19 +0000
treeherdermozilla-aurora@20af7fbd96c1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjaws
bugs923859
milestone27.0a1
Bug 923859 - Wide widget rearranging: derecurse, simplify, improve, r=jaws
browser/components/customizableui/src/PanelWideWidgetTracker.jsm
browser/components/customizableui/test/browser_880382_drag_wide_widgets_in_panel.js
--- a/browser/components/customizableui/src/PanelWideWidgetTracker.jsm
+++ b/browser/components/customizableui/src/PanelWideWidgetTracker.jsm
@@ -21,36 +21,46 @@ let gPanelPlacements = [];
 // All the wide widgets we know of:
 let gWideWidgets = new Set();
 // All the widgets we know of:
 let gSeenWidgets = new Set();
 
 // The class by which we recognize wide widgets:
 const kWidePanelItemClass = "panel-combined-item";
 
+// TODO(bug 885574): Merge this constant with the one in CustomizeMode.jsm,
+//                   maybe just use a pref for this.
+const kColumnsInMenuPanel = 3;
+
 let PanelWideWidgetTracker = {
   // Listeners used to validate panel contents whenever they change:
   onWidgetAdded: function(aWidgetId, aArea, aPosition) {
     if (aArea == gPanel) {
+      gPanelPlacements = CustomizableUI.getWidgetIdsInArea(gPanel);
       let moveForward = this.shouldMoveForward(aWidgetId, aPosition);
-      this.adjustWidgets(aWidgetId, aPosition, moveForward);
+      this.adjustWidgets(aWidgetId, moveForward);
     }
   },
   onWidgetMoved: function(aWidgetId, aArea, aOldPosition, aNewPosition) {
     if (aArea == gPanel) {
+      gPanelPlacements = CustomizableUI.getWidgetIdsInArea(gPanel);
       let moveForward = this.shouldMoveForward(aWidgetId, aNewPosition);
-      this.adjustWidgets(aWidgetId, Math.min(aOldPosition, aNewPosition), moveForward);
+      this.adjustWidgets(aWidgetId, moveForward);
     }
   },
   onWidgetRemoved: function(aWidgetId, aPrevArea) {
     if (aPrevArea == gPanel) {
+      gPanelPlacements = CustomizableUI.getWidgetIdsInArea(gPanel);
       let pos = gPanelPlacements.indexOf(aWidgetId);
-      this.adjustWidgets(aWidgetId, pos);
+      this.adjustWidgets(aWidgetId, false);
     }
   },
+  onWidgetReset: function(aWidgetId) {
+    gPanelPlacements = CustomizableUI.getWidgetIdsInArea(gPanel);
+  },
   // Listener to keep abreast of any new nodes. We use the DOM one because
   // we need access to the actual node's classlist, so we can't use the ones above.
   // Furthermore, onWidgetCreated only fires for API-based widgets, not for XUL ones.
   onWidgetAfterDOMChange: function(aNode, aNextNode, aContainer) {
     if (!gSeenWidgets.has(aNode.id)) {
       if (aNode.classList.contains(kWidePanelItemClass)) {
         gWideWidgets.add(aNode.id);
       }
@@ -58,56 +68,41 @@ let PanelWideWidgetTracker = {
     }
   },
   // When widgets get destroyed, we remove them from our sets of stuff we care about:
   onWidgetDestroyed: function(aWidgetId) {
     gSeenWidgets.delete(aWidgetId);
     gWideWidgets.delete(aWidgetId);
   },
   shouldMoveForward: function(aWidgetId, aPosition) {
-    let currentWidgetAtPosition = gPanelPlacements[aPosition];
+    let currentWidgetAtPosition = gPanelPlacements[aPosition + 1];
     return gWideWidgets.has(currentWidgetAtPosition) && !gWideWidgets.has(aWidgetId);
   },
-  adjustWidgets: function(aWidgetId, aPosition, aMoveForwards) {
-    if (this.adjustmentStack == 0) {
-      this.movingForward = aMoveForwards;
-    }
-    gPanelPlacements = CustomizableUI.getWidgetIdsInArea(gPanel);
-    // First, make a list of all the widgets that are *after* the insertion/moving point.
-    let widgetsAffected = [];
-    for (let widget of gWideWidgets) {
-      let wideWidgetPos = gPanelPlacements.indexOf(widget);
-      // This would just be wideWidgetPos >= aPosition, except that if we start re-arranging
-      // widgets, we would re-enter here because obviously the wide widget ends up in its
-      // own position, and we'd never stop trying to rearrange things.
-      // So instead, we check the wide widget is after the insertion point *or*
-      // this is the first move, and the widget is exactly on the insertion point
-      if (wideWidgetPos > aPosition || (!this.adjustmentStack && wideWidgetPos == aPosition)) {
-        widgetsAffected.push(widget);
-      }
-    }
-    if (!widgetsAffected.length) {
+  adjustWidgets: function(aWidgetId, aMoveForwards) {
+    if (this.adjusting) {
       return;
     }
-    widgetsAffected.sort(function(a, b) gPanelPlacements.indexOf(a) < gPanelPlacements.indexOf(b));
-    this.adjustmentStack++;
-    this.adjustPosition(widgetsAffected[0]);
-    this.adjustmentStack--;
-    if (this.adjustmentStack == 0) {
-      delete this.movingForward;
+    this.adjusting = true;
+    let widgetsAffected = [w for (w of gPanelPlacements) if (gWideWidgets.has(w))];
+    // If we're moving the wide widgets forwards (down/to the right in the panel)
+    // we want to start with the last widgets. Otherwise we move widgets over other wide
+    // widgets, which might mess up their order. Likewise, if moving backwards we should start with
+    // the first widget and work our way down/right from there.
+    let compareFn = aMoveForwards ? (function(a, b) a < b) : (function(a, b) a > b)
+    widgetsAffected.sort(function(a, b) compareFn(gPanelPlacements.indexOf(a),
+                                                  gPanelPlacements.indexOf(b)));
+    for (let widget of widgetsAffected) {
+      this.adjustPosition(widget, aMoveForwards);
     }
+    this.adjusting = false;
   },
   // This function is called whenever an item gets moved in the menu panel. It
-  // adjusts the position of widgets within the panel to reduce single-column
-  // buttons from being placed in a row by themselves.
-  adjustPosition: function(aWidgetId) {
-    // TODO(bug 885574): Merge this constant with the one in CustomizeMode.jsm,
-    //                   maybe just use a pref for this.
-    const kColumnsInMenuPanel = 3;
-
+  // adjusts the position of widgets within the panel to prevent "gaps" between
+  // wide widgets that could be filled up with single column widgets
+  adjustPosition: function(aWidgetId, aMoveForwards) {
     // Make sure that there are n % columns = 0 narrow buttons before the widget.
     let placementIndex = gPanelPlacements.indexOf(aWidgetId);
     let prevSiblingCount = 0;
     let fixedPos = null;
     while (placementIndex--) {
       let thisWidgetId = gPanelPlacements[placementIndex];
       if (gWideWidgets.has(thisWidgetId)) {
         continue;
@@ -135,28 +130,23 @@ let PanelWideWidgetTracker = {
         prevSiblingCount = 0;
       } else {
         prevSiblingCount++;
       }
     }
 
     if (fixedPos !== null || prevSiblingCount % kColumnsInMenuPanel) {
       let desiredPos = (fixedPos !== null) ? fixedPos : gPanelPlacements.indexOf(aWidgetId);
-      if (this.movingForward) {
-        // Add 1 because we're moving forward, and we would otherwise count the widget itself.
-        desiredPos += (kColumnsInMenuPanel - (prevSiblingCount % kColumnsInMenuPanel)) + 1;
-      } else {
-        desiredPos -= prevSiblingCount % kColumnsInMenuPanel;
+      let desiredChange = -(prevSiblingCount % kColumnsInMenuPanel);
+      if (aMoveForwards && fixedPos == null) {
+        // +1 because otherwise we'd count ourselves:
+        desiredChange = kColumnsInMenuPanel + desiredChange + 1;
       }
-      // We don't need to move all of the items in this pass, because
-      // this move will trigger adjustPosition to get called again. The
-      // function will stop recursing when it finds that there is no
-      // more work that is needed.
+      desiredPos += desiredChange;
       CustomizableUI.moveWidgetWithinArea(aWidgetId, desiredPos);
     }
   },
-  adjustmentStack: 0,
   init: function() {
     // Initialize our local placements copy and register the listener
     gPanelPlacements = CustomizableUI.getWidgetIdsInArea(gPanel);
     CustomizableUI.addListener(this);
   },
 };
--- a/browser/components/customizableui/test/browser_880382_drag_wide_widgets_in_panel.js
+++ b/browser/components/customizableui/test/browser_880382_drag_wide_widgets_in_panel.js
@@ -144,18 +144,16 @@ let gTests = [
                                    "save-page-button",
                                    "print-button",
                                    "history-panelmenu",
                                    "fullscreen-button",
                                    "find-button",
                                    "preferences-button",
                                    "add-ons-button"];
       simulateItemDrag(developerButton, zoomControls);
-      // Currently, the developer-button is placed after the zoom-controls, but it should be
-      // placed like expectedPlacementsAfterInsert describes.
       assertAreaPlacements(CustomizableUI.AREA_PANEL, placementsAfterInsert);
       ok(!CustomizableUI.inDefaultState, "Should no longer be in default state.");
       let palette = document.getElementById("customization-palette");
       // Check that the palette items are re-wrapped correctly.
       let feedWrapper = document.getElementById("wrapper-feed-button");
       let feedButton = document.getElementById("feed-button");
       is(feedButton.parentNode, feedWrapper,
          "feed-button should be a child of wrapper-feed-button");
@@ -165,16 +163,52 @@ let gTests = [
       is(developerButton.parentNode.tagName, "toolbarpaletteitem",
          "The developer-button should be wrapped by a toolbarpaletteitem");
       let newWindowButton = document.getElementById("new-window-button");
       simulateItemDrag(zoomControls, newWindowButton);
       ok(CustomizableUI.inDefaultState, "Should be in default state again.");
     },
   },
   {
+    desc: "Dragging an item from the palette to before the edit-controls " +
+          "should move it and two other buttons before the edit and zoom controls.",
+    setup: startCustomizing,
+    run: function() {
+      let developerButton = document.getElementById("developer-button");
+      let editControls = document.getElementById("edit-controls");
+      let placementsAfterInsert = ["developer-button",
+                                   "new-window-button",
+                                   "privatebrowsing-button",
+                                   "edit-controls",
+                                   "zoom-controls",
+                                   "save-page-button",
+                                   "print-button",
+                                   "history-panelmenu",
+                                   "fullscreen-button",
+                                   "find-button",
+                                   "preferences-button",
+                                   "add-ons-button"];
+      simulateItemDrag(developerButton, editControls);
+      assertAreaPlacements(CustomizableUI.AREA_PANEL, placementsAfterInsert);
+      ok(!CustomizableUI.inDefaultState, "Should no longer be in default state.");
+      let palette = document.getElementById("customization-palette");
+      // Check that the palette items are re-wrapped correctly.
+      let feedWrapper = document.getElementById("wrapper-feed-button");
+      let feedButton = document.getElementById("feed-button");
+      is(feedButton.parentNode, feedWrapper,
+         "feed-button should be a child of wrapper-feed-button");
+      is(feedWrapper.getAttribute("place"), "palette",
+         "The feed-button wrapper should have it's place set to 'palette'");
+      simulateItemDrag(developerButton, palette);
+      is(developerButton.parentNode.tagName, "toolbarpaletteitem",
+         "The developer-button should be wrapped by a toolbarpaletteitem");
+      ok(CustomizableUI.inDefaultState, "Should be in default state again.");
+    },
+  },
+  {
     desc: "Dragging the edit-controls to be before the zoom-controls button " +
           "should not move any widgets.",
     setup: startCustomizing,
     run: function() {
       let editControls = document.getElementById("edit-controls");
       let zoomControls = document.getElementById("zoom-controls");
       let placementsAfterMove = ["edit-controls",
                                  "zoom-controls",