Bug 408659 - Tree selection shouldn't go away when an item is resorted or moved. r=dietrich.

2007-12-28 18:59:22 -08:00 · 2007-12-28 18:59:22 -08:00 · b0f92d8d56
--- a/browser/components/places/content/menu.xml
+++ b/browser/components/places/content/menu.xml
@ -288,6 +288,26 @@
          }
        },

+        itemMoved:
+        function PMV_itemMoved(aItem, aOldParent, aOldIndex, aNewParent,
+                               aNewIndex) {
+          // This cannot actually happen yet (see IDL)
+          if (aNewParent != aOldParent);
+            return;
+
+          var popup = this._getPopupForContainer(aNewParent);
+          var index = popup._startMarker + 1 + aNewIndex;
+          var children = popup.childNodes;
+          for (var i = 0; i < children.length; i++) {
+            var menuItem = children[i];
+            if (menuItem.node == aItem) {
+              popup.removeChild(menuItem);
+              popup.insertBefore(menuItem, children[index]);
+              return;
+            }
+          }
+        },
+
        itemChanged: function PMV_itemChanged(aNode) {
          // this check can be removed once we fix bug #382397
          var parentNode = aNode.parent;
--- a/browser/components/places/content/toolbar.xml
+++ b/browser/components/places/content/toolbar.xml
@ -496,6 +496,37 @@
          }
        },

+        itemMoved:
+        function TV_V_itemMoved(aItem, aOldParent, aOldIndex, aNewParent,
+                                aNewIndex) {
+          // This cannot actually happen yet (see IDL)
+          if (aNewParent != aOldParent);
+            return;
+
+          if (aNewParent == this._self.getResultNode()) {
+            var children = this._self.childNodes;
+            for (var i = 0; i < children.length; i++) {
+              var button = children[i];
+              if (button.node == aItem) {
+                this._self.removeChild(button);
+                this._self.insertBefore(button, children[aNewIndex]);
+                this._self.updateChevron();
+                return;
+              }
+            }
+          }
+          var popup = this._getPopupForContainer(aNewParent);
+          var children = popup.childNodes;
+          for (var i = 0; i < children.length; i++) {
+            var menuItem = children[i];
+            if (menuItem.node == aItem) {
+              popup.removeChild(menuItem);
+              popup.insertBefore(menuItem, children[aNewIndex]);
+              return;
+            }
+          }
+        },
+
        itemChanged: function TV_V_itemChanged(aNode) {
          // this check can be removed once we fix bug #382397
          var parentNode = aNode.parent;
--- a/browser/components/places/content/treeView.js
+++ b/browser/components/places/content/treeView.js
@ -298,6 +298,13 @@ PlacesTreeView.prototype = {
    var startReplacement = aContainer.viewIndex + 1;
    var replaceCount = this._countVisibleRowsForItem(aContainer);

+    // We don't replace the container item itself so we decrease the
+    // replaceCount by 1. We don't do so though if there is no visible item
+    // for the container. This happens when aContainer is the root node and
+    // showRoot is not set.
+    if (aContainer.viewIndex != -1)
+      replaceCount-=1;
+
    // Persist selection state
    var nodesToSelect = [];
    var selection = this.selection;
@ -305,20 +312,14 @@ PlacesTreeView.prototype = {
    for (var rangeIndex = 0; rangeIndex < rc; rangeIndex++) {
      var min = { }, max = { };
      selection.getRangeAt(rangeIndex, min, max);
-      if (min.value > startReplacement + replaceCount)
+      var lastIndex = Math.min(max.value, startReplacement + replaceCount -1);
+      if (min.value < startReplacement || min.value > lastIndex)
        continue;

-      for (var nodeIndex = min.value; nodeIndex <= max.value; nodeIndex++)
+      for (var nodeIndex = min.value; nodeIndex <= lastIndex; nodeIndex++)
        nodesToSelect.push(this._visibleElements[nodeIndex]);
    }

-    // We don't replace the container item itself so we decrease the
-    // replaceCount by 1. We don't do so though if there is no visible item
-    // for the container. This happens when aContainer is the root node and
-    // showRoot is not set.
-    if (aContainer.viewIndex != -1)
-      replaceCount-=1;
-
    // Mark the removes as invisible
    for (var i = 0; i < replaceCount; i++)
      this._visibleElements[startReplacement + i].viewIndex = -1;
@ -660,7 +661,7 @@ PlacesTreeView.prototype = {
  itemRemoved: function PTV_itemRemoved(aParent, aItem, aOldIndex) {
    NS_ASSERT(this._result, "Got a notification but have no result!");
    if (!this._tree)
-        return; // nothing to do
+      return; // nothing to do

    var oldViewIndex = aItem.viewIndex;
    if (oldViewIndex < 0)
@ -711,6 +712,55 @@ PlacesTreeView.prototype = {
      this.itemChanged(aParent);
  },

+  /**
+   * Be careful, aOldIndex and aNewIndex specify the index in the
+   * corresponding parent nodes, not the visible indexes.
+   */
+  itemMoved:
+  function PTV_itemMoved(aItem, aOldParent, aOldIndex, aNewParent, aNewIndex) {
+    NS_ASSERT(this._result, "Got a notification but have no result!");
+    if (!this._tree)
+      return; // nothing to do
+
+    var oldViewIndex = aItem.viewIndex;
+    if (oldViewIndex < 0)
+      return; // item was already invisible, nothing to do
+
+    // this may have been a container, in which case it has a lot of rows
+    var count = this._countVisibleRowsForItem(aItem);
+
+    // Persist selection state
+    var nodesToSelect = [];
+    var selection = this.selection;
+    var rc = selection.getRangeCount();
+    for (var rangeIndex = 0; rangeIndex < rc; rangeIndex++) {
+      var min = { }, max = { };
+      selection.getRangeAt(rangeIndex, min, max);
+      var lastIndex = Math.min(max.value, oldViewIndex + count -1);
+      if (min.value < oldViewIndex || min.value > lastIndex)
+        continue;
+
+      for (var nodeIndex = min.value; nodeIndex <= lastIndex; nodeIndex++)
+        nodesToSelect.push(this._visibleElements[nodeIndex]);
+    }
+    if (nodesToSelect.length > 0)
+      selection.selectEventsSuppressed = true;
+
+    // remove the nodes, let itemInserted restore all of its contents
+    this._visibleElements.splice(oldViewIndex, count);
+    this._tree.rowCountChanged(oldViewIndex, -count);
+    this.itemInserted(aNewParent, aItem, aNewIndex);
+
+    // restore selection
+    if (nodesToSelect.length > 0) {
+      for each (var node in nodesToSelect) {
+        var index = node.viewIndex;
+        selection.rangedSelect(index, index, true);
+      }
+      selection.selectEventsSuppressed = false;
+    }
+  },
+
  /**
   * Be careful, the parameter 'aIndex' here specifies the index in the parent
   * node of the item, not the visible index.
--- a/toolkit/components/places/public/nsINavHistoryService.idl
+++ b/toolkit/components/places/public/nsINavHistoryService.idl
@ -485,27 +485,42 @@ interface nsINavHistoryResultViewObserver : nsISupports
 *
 * @see nsINavHistoryResult for where this fits in
 */
-[scriptable, uuid(f208e54c-834f-4a6c-bd4d-a476015bc139)]
+[scriptable, uuid(76cad155-21d5-427e-9740-01386cc8b923)]
 interface nsINavHistoryResultViewer : nsISupports
 {
  /**
-   * Called when 'item' is inserted into 'parent' at index 'newIndex'. The item
-   * previously at index (if any) and everything below it will have been
-   * shifted down by one. The item may be a container or a leaf.
+   * Called when 'aItem' is inserted into 'aParent' at index 'aNewIndex'.
+   * The item previously at index (if any) and everything below it will have
+   * been shifted down by one. The item may be a container or a leaf.
   */
-  void itemInserted(in nsINavHistoryContainerResultNode parent,
-                    in nsINavHistoryResultNode item,
-                    in unsigned long newIndex);
+  void itemInserted(in nsINavHistoryContainerResultNode aParent,
+                    in nsINavHistoryResultNode aItem,
+                    in unsigned long aNewIndex);

  /**
-   * Called whan 'item' is removed from 'parent' at 'oldIndex'. The item may be
-   * a container or a leaf. This function will be called after the item has
-   * been removed from its parent list, but before anything else (including
+   * Called whan 'aItem' is removed from 'aParent' at 'aOldIndex'. The item
+   * may be a container or a leaf. This function will be called after the item
+   * has been removed from its parent list, but before anything else (including
   * NULLing out the item's parent) has happened.
   */
-  void itemRemoved(in nsINavHistoryContainerResultNode parent,
-                   in nsINavHistoryResultNode item,
-                   in unsigned long oldIndex);
+  void itemRemoved(in nsINavHistoryContainerResultNode aParent,
+                   in nsINavHistoryResultNode aItem,
+                   in unsigned long aOldIndex);
+
+  /**
+   * Called whan 'aItem' is moved from 'aOldParent' at 'aOldIndex' to
+   * aNewParent at aNewIndex. The item may be a container or a leaf.
+   *
+   * XXX: at the moment, this method is called only when an item is moved
+   * within the same container. When an item is moved between containers,
+   * a new node is created for the item, and the itemRemoved/itemAdded methods
+   * are used.
+   */
+  void itemMoved(in nsINavHistoryResultNode aItem,
+                 in nsINavHistoryContainerResultNode aOldParent,
+                 in unsigned long aOldIndex,
+                 in nsINavHistoryContainerResultNode aNewParent,
+                 in unsigned long aNewIndex);

  /**
   * Called when an item has been changed and should be repainted. This only
--- a/toolkit/components/places/src/nsNavHistoryResult.cpp
+++ b/toolkit/components/places/src/nsNavHistoryResult.cpp
@ -588,22 +588,10 @@ nsNavHistoryContainerResultNode::ReverseUpdateStats(PRInt32 aAccessCountChange)
      SortComparator comparator = GetSortingComparator(sortMode);
      nsCAutoString sortingAnnotation;
      GetSortingAnnotation(sortingAnnotation);
-      int ourIndex = mParent->FindChild(this);
-      if (mParent->DoesChildNeedResorting(ourIndex, comparator, sortingAnnotation.get())) {
-        // prevent us from being destroyed when removed from the parent
-        nsRefPtr<nsNavHistoryContainerResultNode> ourLock = this;
-        nsNavHistoryContainerResultNode* ourParent = mParent;
-
-        // Performance: moving items by removing and re-inserting is not very
-        // efficient because there may be a lot of unnecessary renumbering of
-        // items. I don't think the overhead is worth the extra complexity in
-        // this case.
-        ourParent->RemoveChildAt(ourIndex, PR_TRUE);
-        ourParent->InsertSortedChild(this, PR_TRUE);
-        resorted = PR_TRUE;
-      }
+      PRUint32 ourIndex = mParent->FindChild(this);
+      resorted = EnsureItemPosition(ourIndex);
    }
-    if (! resorted) {
+    if (!resorted) {
      // repaint visible rows
      nsNavHistoryResult* result = GetResult();
      if (result && result->GetView() && mParent->AreChildrenVisible()) {
@ -1414,6 +1402,42 @@ nsNavHistoryContainerResultNode::InsertSortedChild(
  return InsertChildAt(aNode, mChildren.Count(), aIsTemporary);
 }

+// nsNavHistoryContainerResultNode::EnsureItemPosition
+//
+//  This checks if the item at aIndex is located correctly given the sorting
+//  move. If it's not, the item is moved, and the result view are notified.
+//
+//  Returns true if the item position has been changed, false otherwise.
+
+PRBool
+nsNavHistoryContainerResultNode::EnsureItemPosition(PRUint32 aIndex) {
+  NS_ASSERTION(aIndex >= 0 && aIndex < mChildren.Count(), "Invalid index");
+  if (aIndex < 0 || aIndex >= mChildren.Count())
+    return PR_FALSE;
+
+  SortComparator comparator = GetSortingComparator(GetSortType());
+  if (!comparator)
+    return PR_FALSE;
+
+  nsCAutoString sortAnno;
+  GetSortingAnnotation(sortAnno);
+  if (!DoesChildNeedResorting(aIndex, comparator, sortAnno.get()))
+    return PR_FALSE;
+
+  nsRefPtr<nsNavHistoryResultNode> node(mChildren[aIndex]);
+  mChildren.RemoveObjectAt(aIndex);
+
+  PRUint32 newIndex = FindInsertionPoint(node, comparator,sortAnno.get());
+  mChildren.InsertObjectAt(node.get(), newIndex);
+
+  nsNavHistoryResult* result = GetResult();
+  NS_ENSURE_TRUE(result, PR_TRUE);
+
+  if (result->GetView() && AreChildrenVisible())
+    result->GetView()->ItemMoved(node, this, aIndex, this, newIndex);
+
+  return PR_TRUE;
+}

 // nsNavHistoryContainerResultNode::MergeResults
 //
@ -1694,14 +1718,7 @@ nsNavHistoryContainerResultNode::UpdateURIs(PRBool aRecursive, PRBool aOnlyOne,

    if (aUpdateSort) {
      PRInt32 childIndex = parent->FindChild(node);
-      if (childIndex >= 0 && parent->DoesChildNeedResorting(childIndex, comparator,
-                                                            sortingAnnotation.get())) {
-        // child position changed
-        parent->RemoveChildAt(childIndex, PR_TRUE);
-        parent->InsertChildAt(node, parent->FindInsertionPoint(node, comparator,
-                                                               sortingAnnotation.get()),
-                              PR_TRUE);
-      } else if (childrenVisible) {
+      if ((childIndex < 0 || !parent->EnsureItemPosition(childIndex) && childrenVisible)) {
        result->GetView()->ItemChanged(node);
      }
    } else if (childrenVisible) {
@ -3379,18 +3396,8 @@ nsNavHistoryResultNode::OnItemChanged(PRInt64 aItemId,
  // DO NOT OPTIMIZE THIS TO CHECK aProperty
  // the sorting methods fall back to each other so we need to re-sort the
  // result even if it's not set to sort by the given property
-  nsNavHistoryContainerResultNode::SortComparator comparator =
-    mParent->GetSortingComparator(mParent->GetSortType());
  PRInt32 ourIndex = mParent->FindChild(this);
-  nsCAutoString sortAnno;
-  mParent->GetSortingAnnotation(sortAnno);
-  if (mParent->DoesChildNeedResorting(ourIndex, comparator, sortAnno.get())) {
-    nsCOMPtr<nsINavHistoryResultNode> nodeLock(this);
-    mParent->RemoveChildAt(ourIndex, PR_TRUE);
-    mParent->InsertChildAt(this,
-                           mParent->FindInsertionPoint(this, comparator, sortAnno.get()),
-                           PR_TRUE);
-  }
+  mParent->EnsureItemPosition(ourIndex);

  return NS_OK;
 }
@ -3446,14 +3453,7 @@ nsNavHistoryFolderResultNode::OnItemVisited(PRInt64 aItemId,
    PRInt32 childIndex = FindChild(node);
    NS_ASSERTION(childIndex >= 0, "Could not find child we just got a reference to");
    if (childIndex >= 0) {
-      SortComparator comparator = GetSortingComparator(GetSortType());
-      nsCAutoString sortingAnnotation;
-      GetSortingAnnotation(sortingAnnotation);
-      nsCOMPtr<nsINavHistoryResultNode> nodeLock(node);
-      RemoveChildAt(childIndex, PR_TRUE);
-      InsertChildAt(node,
-                    FindInsertionPoint(node, comparator, sortingAnnotation.get()),
-                    PR_TRUE);
+      EnsureItemPosition(childIndex);
    }
  } else if (result->GetView() && AreChildrenVisible()) {
    // no sorting changed, just redraw the row if visible
@ -3493,21 +3493,8 @@ nsNavHistoryFolderResultNode::OnItemMoved(PRInt64 aItemId, PRInt64 aOldParent,
    node->mBookmarkIndex = aNewIndex;

    // adjust position
-    PRInt32 sortType = GetSortType();
-    SortComparator comparator = GetSortingComparator(sortType);
-    nsCAutoString sortingAnnotation;
-    GetSortingAnnotation(sortingAnnotation);
-    if (DoesChildNeedResorting(index, comparator, sortingAnnotation.get())) {
-      // needs resorting, this will cause everything to be redrawn, so we
-      // don't need to do that explicitly later.
-      nsRefPtr<nsNavHistoryResultNode> lock(node);
-      RemoveChildAt(index, PR_TRUE);
-      InsertChildAt(node,
-                    FindInsertionPoint(node, comparator, sortingAnnotation.get()),
-                    PR_TRUE);
-      return NS_OK;
-    }
-
+    EnsureItemPosition(index);
+    return NS_OK;
  } else {
    // moving between two different folders, just do a remove and an add
    if (aOldParent == mItemId)
--- a/toolkit/components/places/src/nsNavHistoryResult.h
+++ b/toolkit/components/places/src/nsNavHistoryResult.h
@ -653,6 +653,7 @@ public:
                         PRBool aIsTemporary = PR_FALSE);
  nsresult InsertSortedChild(nsNavHistoryResultNode* aNode,
                             PRBool aIsTemporary = PR_FALSE);
+  PRBool EnsureItemPosition(PRUint32 aIndex);
  void MergeResults(nsCOMArray<nsNavHistoryResultNode>* aNodes);
  nsresult ReplaceChildURIAt(PRUint32 aIndex, nsNavHistoryResultNode* aNode);
  nsresult RemoveChildAt(PRInt32 aIndex, PRBool aIsTemporary = PR_FALSE);
--- a/toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js
+++ b/toolkit/components/places/tests/unit/test_nsINavHistoryViewer.js
@ -82,6 +82,10 @@ var viewer = {
    dump("itemReplaced: " + newItem.uri + "\n");
    this.replacedItem = item;
  },
+  movedItem: null,
+  itemMoved: function(item, oldParent, oldIndex, newParent, newIndex) {
+    this.movedItem = item;
+  },
  openedContainer: null,
  containerOpened: function(item) {
    this.openedContainer = item;
@ -112,6 +116,7 @@ var viewer = {
    this.removedItem = null;
    this.changedItem = null;
    this.replacedItem = null;
+    this.movedItem = null;
    this.openedContainer = null;
    this.closedContainer = null;
    this.invalidatedContainer = null;
@ -210,10 +215,13 @@ function run_test() {
  bmsvc.setItemTitle(testBookmark, "baz");
  do_check_eq(viewer.changedItem.title, "baz");

+  var testBookmark2 = bmsvc.insertBookmark(bmsvc.bookmarksMenuFolder, uri("http://google.com"), bmsvc.DEFAULT_INDEX, "foo");
+  bmsvc.moveItem(testBookmark2, bmsvc.bookmarksMenuFolder, 0);
+  do_check_eq(viewer.movedItem.itemId, testBookmark2);
+
  // nsINavHistoryResultViewer.itemRemoved
-  var removedBookmark = bmsvc.insertBookmark(bmsvc.bookmarksMenuFolder, uri("http://google.com"), bmsvc.DEFAULT_INDEX, "foo");
-  bmsvc.removeItem(removedBookmark);
-  do_check_eq(removedBookmark, viewer.removedItem.itemId);
+  bmsvc.removeItem(testBookmark2);
+  do_check_eq(testBookmark2, viewer.removedItem.itemId);

  // XXX nsINavHistoryResultViewer.itemReplaced
  // NHQRN.onVisit()->NHCRN.MergeResults()->NHCRN.ReplaceChildURIAt()->NHRV.ItemReplaced()