Bug 394353 (Part 2) - Tag list is not updated when manually adding, renaming, or deleting tags from within the tags field.

r+a=dietrich

browser/components/places/content/editBookmarkOverlay.js

@@ -138,7 +138,7 @@ var gEditItemOverlay = {
       this.uninitPanel(false);
 
     var aItemIdList;
-    if (aFor.length) {
+    if (Array.isArray(aFor)) {
       aItemIdList = aFor;
       aFor = aItemIdList[0];
     }
@@ -211,7 +211,6 @@ var gEditItemOverlay = {
         this._multiEdit = true;
         this._allTags = [];
         this._itemIds = aItemIdList;
-        var nodeToCheck = 0;
         for (var i = 0; i < aItemIdList.length; i++) {
           if (aItemIdList[i] instanceof Ci.nsIURI) {
             this._uris[i] = aItemIdList[i];
@@ -220,10 +219,8 @@ var gEditItemOverlay = {
           else
             this._uris[i] = PlacesUtils.bookmarks.getBookmarkURI(this._itemIds[i]);
           this._tags[i] = PlacesUtils.tagging.getTagsForURI(this._uris[i]);
-          if (this._tags[i].length < this._tags[nodeToCheck].length)
-            nodeToCheck =  i;
         }
-        this._getCommonTags(nodeToCheck);
+        this._allTags = this._getCommonTags();
         this._initTextField("tagsField", this._allTags.join(", "), false);
         this._element("itemsCountText").value =
           PlacesUIUtils.getFormattedString("detailsPane.multipleItems",
@@ -241,7 +238,9 @@ var gEditItemOverlay = {
 
     // observe changes
     if (!this._observersAdded) {
-      if (this._itemId != -1)
+      // Single bookmarks observe any change.  History entries and multiEdit
+      // observe only tags changes, through bookmarks.
+      if (this._itemId != -1 || this._uri || this._multiEdit)
         PlacesUtils.bookmarks.addObserver(this, false);
       window.addEventListener("unload", this, false);
       this._observersAdded = true;
@@ -250,22 +249,19 @@ var gEditItemOverlay = {
     this._initialized = true;
   },
 
-  _getCommonTags: function(aArrIndex) {
-    var tempArray = this._tags[aArrIndex];
-    var isAllTag;
-    for (var k = 0; k < tempArray.length; k++) {
-      isAllTag = true;
-      for (var j = 0; j < this._tags.length; j++) {
-        if (j == aArrIndex)
-          continue;
-        if (this._tags[j].indexOf(tempArray[k]) == -1) {
-          isAllTag = false;
-          break;
-        }
-      }
-      if (isAllTag)
-        this._allTags.push(tempArray[k]);
-    }
+  /**
+   * Finds tags that are in common among this._tags entries that track tags
+   * for each selected uri.
+   * The tags arrays should be kept up-to-date for this to work properly.
+   *
+   * @return array of common tags for the selected uris.
+   */
+  _getCommonTags: function() {
+    return this._tags[0].filter(
+      function (aTag) this._tags.every(
+        function (aTags) aTags.indexOf(aTag) != -1
+      ), this
+    );
   },
 
   _initTextField: function(aTextFieldId, aValue, aReadOnly) {
@@ -542,7 +538,7 @@ var gEditItemOverlay = {
     }
 
     if (this._observersAdded) {
-      if (this._itemId != -1)
+      if (this._itemId != -1 || this._uri || this._multiEdit)
         PlacesUtils.bookmarks.removeObserver(this);
 
       this._observersAdded = false;
@@ -1069,6 +1065,42 @@ var gEditItemOverlay = {
   onItemChanged: function EIO_onItemChanged(aItemId, aProperty,
                                             aIsAnnotationProperty, aValue,
                                             aLastModified, aItemType) {
+    if (aProperty == "tags") {
+      // Tags case is special, since they should be updated if either:
+      // - the notification is for the edited bookmark
+      // - the notification is for the edited history entry
+      // - the notification is for one of edited uris
+      let shouldUpdateTagsField = this._itemId == aItemId;
+      if (this._itemId == -1 || this._multiEdit) {
+        // Check if the changed uri is part of the modified ones.
+        let changedURI = PlacesUtils.bookmarks.getBookmarkURI(aItemId);
+        let uris = this._multiEdit ? this._uris : [this._uri];
+        uris.forEach(function (aURI, aIndex) {
+          if (aURI.equals(changedURI)) {
+            shouldUpdateTagsField = true;
+            if (this._multiEdit) {
+              this._tags[aIndex] = PlacesUtils.tagging.getTagsForURI(this._uris[aIndex]);
+            }
+          }
+        }, this);
+      }
+
+      if (shouldUpdateTagsField) {
+        if (this._multiEdit) {
+          this._allTags = this._getCommonTags();
+          this._initTextField("tagsField", this._allTags.join(", "), false);
+        }
+        else {
+          let tags = PlacesUtils.tagging.getTagsForURI(this._uri).join(", ");
+          this._initTextField("tagsField", tags, false);
+        }
+      }
+
+      // Any tags change should be reflected in the tags selector.
+      this._rebuildTagsSelectorList();
+      return;
+    }
+
     if (this._itemId != aItemId) {
       if (aProperty == "title") {
         // If the title of a folder which is listed within the folders
@@ -1164,25 +1196,9 @@ var gEditItemOverlay = {
   onItemAdded: function EIO_onItemAdded(aItemId, aParentId, aIndex, aItemType,
                                         aURI) {
     this._lastNewItem = aItemId;
-
-    if (this._uri && aItemType == PlacesUtils.bookmarks.TYPE_BOOKMARK &&
-        PlacesUtils.bookmarks.getFolderIdForItem(aParentId) ==
-          PlacesUtils.tagsFolderId) {
-      // Ensure the tagsField is in sync.
-      let tags = PlacesUtils.tagging.getTagsForURI(this._uri).join(", ");
-      this._initTextField("tagsField", tags, false);
-    }
-  },
-  onItemRemoved: function(aItemId, aParentId, aIndex, aItemType) {
-    if (this._uri && aItemType == PlacesUtils.bookmarks.TYPE_BOOKMARK &&
-        PlacesUtils.bookmarks.getFolderIdForItem(aParentId) ==
-          PlacesUtils.tagsFolderId) {
-      // Ensure the tagsField is in sync.
-      let tags = PlacesUtils.tagging.getTagsForURI(this._uri).join(", ");
-      this._initTextField("tagsField", tags, false);
-    }
   },
 
+  onItemRemoved: function() { },
   onBeginUpdateBatch: function() { },
   onEndUpdateBatch: function() { },
   onBeforeItemRemoved: function() { },

browser/components/places/tests/chrome/test_editBookmarkOverlay_tags_liveUpdate.xul

@@ -36,43 +36,163 @@
 
   <script type="application/javascript">
   <![CDATA[
-    function runTest() {
+    function runTest()
+    {
       Components.utils.import("resource://gre/modules/NetUtil.jsm");
 
-      const TEST_URI = NetUtil.newURI("http://www.test.me");
+      const TEST_URI = NetUtil.newURI("http://www.test.me/");
+      const TEST_URI2 = NetUtil.newURI("http://www.test.again.me/");
       const TEST_TAG = "test-tag";
 
-      // Add a bookmark, init the panel then add a tag to the bookmark
-      // through the Places API.  The editBookmarkPanel should live update.
+      ok(gEditItemOverlay, "Sanity check: gEditItemOverlay is in context");
+
+      // Open the tags selector.
+      document.getElementById("editBMPanel_tagsSelectorRow").collapsed = false;
+
+      function checkTagsSelector(aAvailableTags, aCheckedTags)
+      {
+        is(PlacesUtils.tagging.allTags.length, aAvailableTags.length,
+           "tagging service is in sync.");
+        let tagsSelector = document.getElementById("editBMPanel_tagsSelector");
+        let children = tagsSelector.childNodes;
+        is(children.length, aAvailableTags.length,
+            "Found expected number of tags in the tags selector");
+        Array.forEach(children, function (aChild) {
+          let tag = aChild.getAttribute("label");
+          ok(true, "Found tag '" + tag + "' in the selector");
+          ok(aAvailableTags.indexOf(tag) != -1, "Found expected tag");
+          let checked = aChild.getAttribute("checked") == "true";
+          is(checked, aCheckedTags.indexOf(tag) != -1,
+             "Tag is correctly marked");
+        });
+      }
+
+      // Add a bookmark.
       let itemId = PlacesUtils.bookmarks.insertBookmark(
-        PlacesUtils.toolbarFolderId, TEST_URI,
+        PlacesUtils.unfiledBookmarksFolderId, TEST_URI,
         PlacesUtils.bookmarks.DEFAULT_INDEX, "test.me"
       );
 
       // Init panel.
-      ok(gEditItemOverlay, "gEditItemOverlay is in context");
       gEditItemOverlay.initPanel(itemId);
 
       // Add a tag.
       PlacesUtils.tagging.tagURI(TEST_URI, [TEST_TAG]);
-      // Test that the tag has been added.
+
       is(PlacesUtils.tagging.getTagsForURI(TEST_URI)[0], TEST_TAG,
-         "Tag correctly added.");
-      // Check the panel.
-      is (document.getElementById("editBMPanel_tagsField").value, TEST_TAG,
-          "Tags match.");
+         "Correctly added tag to a single bookmark");
+      is(document.getElementById("editBMPanel_tagsField").value, TEST_TAG,
+         "Editing a single bookmark shows the added tag");
+      checkTagsSelector([TEST_TAG], [TEST_TAG]);
 
       // Remove tag.
       PlacesUtils.tagging.untagURI(TEST_URI, [TEST_TAG]);
-      // Test that the tag has been removed.
       is(PlacesUtils.tagging.getTagsForURI(TEST_URI)[0], undefined,
-         "Tag correctly removed.");
-      // Check the panel.
-      is (document.getElementById("editBMPanel_tagsField").value, "",
-          "Tags match.");
+         "The tag has been removed");
+      is(document.getElementById("editBMPanel_tagsField").value, "",
+         "Editing a single bookmark should not show any tag");
+      checkTagsSelector([], []);
+
+      // Add a second bookmark.
+      let itemId2 = PlacesUtils.bookmarks.insertBookmark(
+        PlacesUtils.unfiledBookmarksFolderId, TEST_URI2,
+        PlacesUtils.bookmarks.DEFAULT_INDEX, "test.again.me"
+      );
+
+      // Init panel with multiple bookmarks.
+      gEditItemOverlay.initPanel([itemId, itemId2]);
+
+      // Add a tag to the first bookmark.
+      PlacesUtils.tagging.tagURI(TEST_URI, [TEST_TAG]);
+      is(PlacesUtils.tagging.getTagsForURI(TEST_URI)[0], TEST_TAG,
+         "Correctly added a tag to the first bookmark.");
+      is(document.getElementById("editBMPanel_tagsField").value, "",
+         "Editing multiple bookmarks without matching tags should not show any tag.");
+      checkTagsSelector([TEST_TAG], []);
+      
+      // Add a tag to the second bookmark.
+      PlacesUtils.tagging.tagURI(TEST_URI2, [TEST_TAG]);
+      is(PlacesUtils.tagging.getTagsForURI(TEST_URI2)[0], TEST_TAG,
+         "Correctly added a tag to the second bookmark.");
+      is(document.getElementById("editBMPanel_tagsField").value, TEST_TAG,
+         "Editing multiple bookmarks should show matching tags.");
+      checkTagsSelector([TEST_TAG], [TEST_TAG]);
+
+      // Remove tag from the first bookmark.
+      PlacesUtils.tagging.untagURI(TEST_URI, [TEST_TAG]);
+      is(PlacesUtils.tagging.getTagsForURI(TEST_URI)[0], undefined,
+         "Correctly removed tag from the first bookmark.");
+      is(document.getElementById("editBMPanel_tagsField").value, "",
+         "Editing multiple bookmarks without matching tags should not show any tag.");
+      checkTagsSelector([TEST_TAG], []);
+
+      // Remove tag from the second bookmark.
+      PlacesUtils.tagging.untagURI(TEST_URI2, [TEST_TAG]);
+      is(PlacesUtils.tagging.getTagsForURI(TEST_URI2)[0], undefined,
+         "Correctly removed tag from the second bookmark.");
+      is(document.getElementById("editBMPanel_tagsField").value, "",
+         "Editing multiple bookmarks without matching tags should not show any tag.");
+      checkTagsSelector([], []);
+
+      // Init panel with a nsIURI entry.
+      gEditItemOverlay.initPanel(TEST_URI);
+
+      // Add a tag.
+      PlacesUtils.tagging.tagURI(TEST_URI, [TEST_TAG]);
+      is(PlacesUtils.tagging.getTagsForURI(TEST_URI)[0], TEST_TAG,
+         "Correctly added tag to the first entry.");
+      is(document.getElementById("editBMPanel_tagsField").value, TEST_TAG,
+         "Editing a single nsIURI entry shows the added tag");
+      checkTagsSelector([TEST_TAG], [TEST_TAG]);
+
+      // Remove tag.
+      PlacesUtils.tagging.untagURI(TEST_URI, [TEST_TAG]);
+      is(PlacesUtils.tagging.getTagsForURI(TEST_URI)[0], undefined,
+         "Correctly removed tag from the nsIURI entry.");
+      is(document.getElementById("editBMPanel_tagsField").value, "",
+         "Editing a single nsIURI entry should not show any tag");
+      checkTagsSelector([], []);
+
+      // Init panel with multiple nsIURI entries.
+      gEditItemOverlay.initPanel([TEST_URI, TEST_URI2]);
+
+      // Add a tag to the first entry.
+      PlacesUtils.tagging.tagURI(TEST_URI, [TEST_TAG]);
+      is(PlacesUtils.tagging.getTagsForURI(TEST_URI)[0], TEST_TAG,
+         "Tag correctly added.");
+      is(document.getElementById("editBMPanel_tagsField").value, "",
+         "Editing multiple nsIURIs without matching tags should not show any tag.");
+      checkTagsSelector([TEST_TAG], []);
+
+      // Add a tag to the second entry.
+      PlacesUtils.tagging.tagURI(TEST_URI2, [TEST_TAG]);
+      is(PlacesUtils.tagging.getTagsForURI(TEST_URI2)[0], TEST_TAG,
+         "Tag correctly added.");
+      is(document.getElementById("editBMPanel_tagsField").value, TEST_TAG,
+         "Editing multiple nsIURIs should show matching tags");
+      checkTagsSelector([TEST_TAG], [TEST_TAG]);
+
+      // Remove tag from the first entry.
+      PlacesUtils.tagging.untagURI(TEST_URI, [TEST_TAG]);
+      is(PlacesUtils.tagging.getTagsForURI(TEST_URI)[0], undefined,
+         "Correctly removed tag from the first entry.");
+      is(document.getElementById("editBMPanel_tagsField").value, "",
+         "Editing multiple nsIURIs without matching tags should not show any tag.");
+      checkTagsSelector([TEST_TAG], []);
+
+      // Remove tag from the second entry.
+      PlacesUtils.tagging.untagURI(TEST_URI2, [TEST_TAG]);
+      is(PlacesUtils.tagging.getTagsForURI(TEST_URI2)[0], undefined,
+         "Correctly removed tag from the second entry.");
+      is(document.getElementById("editBMPanel_tagsField").value, "",
+         "Editing multiple nsIURIs without matching tags should not show any tag.");
+      checkTagsSelector([], []);
 
       // Cleanup.
-      PlacesUtils.bookmarks.removeItem(itemId);
+      PlacesUtils.bookmarks.removeFolderChildren(
+        PlacesUtils.unfiledBookmarksFolderId
+      );
+
     }
   ]]>
   </script>

toolkit/components/places/src/nsNavBookmarks.cpp

@@ -941,6 +941,11 @@ nsNavBookmarks::InsertBookmark(PRInt64 aFolder,
 
     if (bookmarks.Length()) {
       for (PRUint32 i = 0; i < bookmarks.Length(); i++) {
+        // Don't notify to the same tag entry we just added.
+        if (bookmarks[i] == *aNewBookmarkId) {
+          continue;
+        }
+
         NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                          nsINavBookmarkObserver,
                          OnItemChanged(bookmarks[i], NS_LITERAL_CSTRING("tags"),
@@ -1042,37 +1047,40 @@ nsNavBookmarks::RemoveItem(PRInt64 aItemId)
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
+  bool isTagEntry = false;
+  if (itemType == TYPE_BOOKMARK) {
+    // Check if the removed bookmark was child of a tag container.
+    // This is done before notifying since during the notification the parent
+    // could be removed as well.
+    PRInt64 grandParentId;
+    rv = GetFolderIdForItem(folderId, &grandParentId);
+    NS_ENSURE_SUCCESS(rv, rv);
+    isTagEntry = grandParentId == mTagsRoot;
+  }
+
   NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
                    nsINavBookmarkObserver,
                    OnItemRemoved(aItemId, folderId, childIndex, itemType));
 
-  if (itemType == TYPE_BOOKMARK) {
-    // If the removed bookmark was a child of a tag container, notify all
-    // bookmark-folder result nodes which contain a bookmark for the removed
-    // bookmark's url.
-    PRInt64 grandParentId;
-    rv = GetFolderIdForItem(folderId, &grandParentId);
+  if (isTagEntry) {
+    // Get all bookmarks pointing to the same uri as this tag entry and
+    // notify them that tags changed.
+    nsCOMPtr<nsIURI> uri;
+    rv = NS_NewURI(getter_AddRefs(uri), spec);
+    NS_ENSURE_SUCCESS(rv, rv);
+    nsTArray<PRInt64> bookmarks;
+    rv = GetBookmarkIdsForURITArray(uri, bookmarks);
     NS_ENSURE_SUCCESS(rv, rv);
-    if (grandParentId == mTagsRoot) {
-      nsCOMPtr<nsIURI> uri;
-      rv = NS_NewURI(getter_AddRefs(uri), spec);
-      NS_ENSURE_SUCCESS(rv, rv);
-      nsTArray<PRInt64> bookmarks;
 
-      rv = GetBookmarkIdsForURITArray(uri, bookmarks);
-      NS_ENSURE_SUCCESS(rv, rv);
-
-      if (bookmarks.Length()) {
-        for (PRUint32 i = 0; i < bookmarks.Length(); i++) {
-          NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
-                           nsINavBookmarkObserver,
-                           OnItemChanged(bookmarks[i],
-                                         NS_LITERAL_CSTRING("tags"), PR_FALSE,
-                                         EmptyCString(), 0, TYPE_BOOKMARK));
-        }
-      }
+    for (PRUint32 i = 0; i < bookmarks.Length(); i++) {
+      NOTIFY_OBSERVERS(mCanNotify, mCacheObservers, mObservers,
+                       nsINavBookmarkObserver,
+                       OnItemChanged(bookmarks[i],
+                                     NS_LITERAL_CSTRING("tags"), PR_FALSE,
+                                     EmptyCString(), 0, TYPE_BOOKMARK));
     }
   }
+
   return NS_OK;
 }
 

toolkit/components/places/src/nsTaggingService.js

@@ -226,13 +226,13 @@ TaggingService.prototype = {
     var result = this._getTagResult(aTagId);
     if (!result)
       return;
-    var node = result.root;
-    node.QueryInterface(Ci.nsINavHistoryContainerResultNode);
+    var node = PlacesUtils.asContainer(result.root);
     node.containerOpen = true;
     var cc = node.childCount;
     node.containerOpen = false;
-    if (cc == 0)
-      PlacesUtils.bookmarks.removeItem(node.itemId);
+    if (cc == 0) {
+      PlacesUtils.bookmarks.removeItem(aTagId);
+    }
   },
 
   // nsITaggingService
@@ -263,7 +263,6 @@ TaggingService.prototype = {
             if (itemId != -1) {
               // There is a tagged item.
               PlacesUtils.bookmarks.removeItem(itemId);
-              this._removeTagIfEmpty(tag.id);
             }
           }
         }, taggingService);
@@ -432,6 +431,11 @@ TaggingService.prototype = {
       if (tagIds)
         this.untagURI(itemURI, tagIds);
     }
+
+    // Item is a tag entry.  If this was the last entry for this tag, remove it.
+    else if (itemURI && this._tagFolders[aFolderId]) {
+      this._removeTagIfEmpty(aFolderId);
+    }
   },
 
   onItemChanged: function(aItemId, aProperty, aIsAnnotationProperty, aNewValue,