diff --git a/browser/components/places/content/editBookmarkOverlay.js b/browser/components/places/content/editBookmarkOverlay.js index 4a0e2d7eb57..4714bf06239 100644 --- a/browser/components/places/content/editBookmarkOverlay.js +++ b/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() { }, diff --git a/browser/components/places/tests/chrome/test_editBookmarkOverlay_tags_liveUpdate.xul b/browser/components/places/tests/chrome/test_editBookmarkOverlay_tags_liveUpdate.xul index 80859a578d5..d4af4f5c3aa 100644 --- a/browser/components/places/tests/chrome/test_editBookmarkOverlay_tags_liveUpdate.xul +++ b/browser/components/places/tests/chrome/test_editBookmarkOverlay_tags_liveUpdate.xul @@ -36,43 +36,163 @@ diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp index ba3d8166271..145e625f05a 100644 --- a/toolkit/components/places/src/nsNavBookmarks.cpp +++ b/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 uri; + rv = NS_NewURI(getter_AddRefs(uri), spec); + NS_ENSURE_SUCCESS(rv, rv); + nsTArray bookmarks; + rv = GetBookmarkIdsForURITArray(uri, bookmarks); NS_ENSURE_SUCCESS(rv, rv); - if (grandParentId == mTagsRoot) { - nsCOMPtr uri; - rv = NS_NewURI(getter_AddRefs(uri), spec); - NS_ENSURE_SUCCESS(rv, rv); - nsTArray 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; } diff --git a/toolkit/components/places/src/nsTaggingService.js b/toolkit/components/places/src/nsTaggingService.js index dfecf64db53..e653a29436b 100644 --- a/toolkit/components/places/src/nsTaggingService.js +++ b/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,