If a user goes to the Library (organizer) in Places, they can go to their "Smart Bookmarks" folder, select the "Recent Tags" folder and then a tag. When they select a tag, the "Name" field in the details below will be populated with the name of the tag. This is an editable field. If the user renames the tag to the name of an existing tag, the tag is not merged into the existing tag (it's bookmarks are not merged into that tag). Instead, you will now have two tags with the same name with bookmarks associated with them. If the user looks at the "Tags" folder in the Library, they will see both tags listed there with the same name and their items. We should be merging tags. Otherwise, if people do have a typo in a tag, the only way to merge entries into the correct tag is to remove the typo tag and then add the other tag. This seems like a basic feature for tagging support.

Found in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008011204 Minefield/3.0b3pre

Comment on attachment 330613 [details] [diff] [review] api for merging (and renaming) tags + fix First-pass review.

Comment on attachment 330613 [details] [diff] [review] api for merging (and renaming) tags + fix >diff --git a/browser/components/places/src/nsPlacesTransactionsService.js b/browser/components/places/src/nsPlacesTransactionsService.js >--- a/browser/components/places/src/nsPlacesTransactionsService.js >+++ b/browser/components/places/src/nsPlacesTransactionsService.js >@@ -115,16 +115,20 @@ placesTransactionsService.prototype = { > var uri = PlacesUtils.bookmarks.getBookmarkURI(id); > return this.untagURI(uri, [parent]); > } > > return new placesRemoveItemTransaction(id); > }, > > editItemTitle: function placesEditItmTitle(id, newTitle) { please s/Itm/Item/ while you're there >+ var parent = PlacesUtils.bookmarks.getFolderIdForItem(id) >+ if (parent == PlacesUtils.tagsFolderId) >+ return new placesRenameOrMergeTag(PlacesUtils.bookmarks.getItemTitle(id), newTitle); >+ > return new placesEditItemTitleTransactions(id, newTitle); > }, > > editBookmarkURI: function placesEditBkmkURI(aBookmarkId, aNewURI) { > return new placesEditBookmarkURITransactions(aBookmarkId, aNewURI); > }, > > setLoadInSidebar: function placesSetLdInSdbar(aBookmarkId, aLoadInSidebar) { >@@ -580,16 +584,46 @@ placesEditItemTitleTransactions.prototyp > PlacesUtils.bookmarks.setItemTitle(this._id, this._newTitle); > }, > > undoTransaction: function PEITT_undoTransaction() { > PlacesUtils.bookmarks.setItemTitle(this._id, this._oldTitle); > } > }; > >+function placesRenameOrMergeTag(sourceTag, targetTag) { >+ this._sourceTag = sourceTag; >+ this._targetTag = targetTag; >+} please append "Transaction" on the end of the name, for consistency w/ the others. >+placesRenameOrMergeTag.prototype = { >+ __proto__: placesBaseTransaction.prototype, >+ >+ doTransaction: function PROMT_doTransaction() { >+ if (PlacesUtils.tagging.tagExists(this._targetTag)) { >+ // complex case - undoing this transaction should separate back the tags! >+ this._movedURIs = PlacesUtils.tagging.moveURIs([this._sourceTag], this._targetTag); >+ } >+ else >+ PlacesUtils.tagging.moveURIs([this._sourceTag], this._targetTag); >+ }, >+ >+ undoTransaction: function PROMT_undoTransaction() { >+ if (!this._movedURIs) // just renamed it back nit: s/renamed/rename/ >+ PlacesUtils.tagging.moveURIs([this._targetTag], this._sourceTag); >+ else { >+ for (var i=0; i < this._movedURIs.length; i++) { >+ PlacesUtils.tagging.untagURI(this._movedURIs[i], [this._targetTag]); >+ PlacesUtils.tagging.tagURI(this._movedURIs[i], [this._sourceTag]); >+ } >+ this._movedURIs = null; >+ } >+ } >+}; don't we always want to store the list of affected URIs, and only apply the change to that set for undo/redo? for undo, the list of tagged URIs for a new tag could be different than when the transaction was first committed. >+ /** >+ * Moves all the URIs tagged with one or more tags to another tag. >+ * >+ * @param aSourceTags >+ * Array of tags from which URIs may be moved. >+ * @param aTargetTag >+ * the target tag, with which URIs might be tagged. >+ * @returns an array of URIs which were tagged with aTargetTag as a result >+ * of calling this method (Note this opereation never results >+ * in duplicated URIs under aTargetTag). >+ */ >+ nsIVariant moveURIs(in nsIVariant aSourceTags, in AString aTargetTag); > }; moveURIs describes the back-end implementation, not the action. maybe mergeTags, or migrateTags? > > // nsITaggingService > getURIsForTag: function TS_getURIsForTag(aTag) { > if (aTag.length == 0) > throw Cr.NS_ERROR_INVALID_ARG; > >- var uris = []; > var tagNode = this._getTagNode(aTag); >- if (tagNode) { >- tagNode.QueryInterface(Ci.nsINavHistoryContainerResultNode); >- tagNode.containerOpen = true; >- var cc = tagNode.childCount; >- for (var i=0; i < cc; i++) >- uris.push(gIoService.newURI(tagNode.getChild(i).uri, null, null)); >- tagNode.containerOpen = false; >- } >- return uris; >+ return tagNode ? this._getURIforTagNode(tagNode) : []; > }, s/_getURIfor/_getURIsFor/ >+ // nsITaggingService >+ moveURIs: function TS_moveURIs(aSourceTags, aTargetTag) { >+ var targetTagNode = this._getTagNode(aTargetTag); >+ >+ if (!targetTagNode) { >+ if (typeof(aTargetTag) != "string") >+ throw Cr.NS_ERROR_INVALID_ARG; >+ >+ // Most common and simple case: the tag to which the urls should be >+ // moved doesn't exist already and the there's only one tag from which nit: s/the// >+ // uris needs to be "moved". In this case, the old-tag container can be >+ // simply renamed. >+ if (aSourceTags.length == 1) { >+ var oldTagNode = this._getTagNode(aSourceTags[0]); >+ if (!oldTagNode) // Nothing to do >+ return; >+ >+ this._bms.setItemTitle(oldTagNode.itemId, aTargetTag); >+ return this._getURIsForTagNode(oldTagNode); >+ } >+ } >+ >+ // Otherwise, move the items (but make sure not to create dupes!) >+ // >+ // urisUnderTargetTag has a couple of purposes: >+ // - dynamically keeping track of all uris under the target-tag container, >+ // to avoid dupes. >+ // - starting after initialTargetArraySize (which is the number of uris >+ // tagged with the target-tag _before_ calling this method), it stores >+ // the return value of this method, which is an array of all uris >+ // which were actually moved from the source tags. >+ var urisUnderTargetTag = targetTagNode ? >+ this._getURIsForTagNode(targetTagNode) : []; >+ var initialTargetArraySize = urisUnderTargetTag.length; >+ >+ var itemsToMove = []; >+ var tagsToRemove = []; >+ for (var i=0; i < aSourceTags.length; i++) { >+ var oldTagNode = this._getTagNode(aSourceTags[i]); >+ oldTagNode.containerOpen = true; >+ var cc = oldTagNode.childCount; >+ for (var j=0; j < cc; j++) { >+ var uriNode = oldTagNode.getChild(j); >+ var uriObject = gIoService.newURI(uriNode.uri, null, null); >+ >+ // only move the item if the uri isn't already set under the target >+ // tag >+ if (urisUnderTargetTag.every(function(uriElt) { return !uriElt.equals(uriObject); })) { wouldn't >+ itemsToMove.push(uriNode.itemId); >+ urisUnderTargetTag.push(uriObject); >+ } >+ } >+ tagsToRemove.push(oldTagNode.itemId); >+ } >+ >+ >+ // Nothing to move >+ if (itemsToMove.length == 0) >+ return []; >+ >+ // move the items, create a container for the tag if there's not >+ // one already >+ var targetTagId = targetTagNode ? >+ targetTagNode.itemId : this._createTag(aTargetTag); >+ for (i=0; i < itemsToMove.length; i++) >+ this._bms.moveItem(itemsToMove[i], targetTagId, this._bms.DEFAULT_INDEX); >+ >+ // remove the source tags >+ for (i=0; i < tagsToRemove.length; i++) >+ this._bms.removeFolder(tagsToRemove[i]); >+ >+ // return all the uris which were tagged >+ return urisUnderTargetTag.slice(initialTargetArraySize); >+ }, a couple of things: 1. i don't like how this avoids using the tag/untag methods of the service, in favor of using the bookmarks service directly. this makes future changes to how tagging is done more difficult, as the code is less centralized. 2. given the potential for a large number of database writes, should batch this.

(In reply to comment #5) > >+ PlacesUtils.tagging.moveURIs([this._targetTag], this._sourceTag); > >+ else { > >+ for (var i=0; i < this._movedURIs.length; i++) { > >+ PlacesUtils.tagging.untagURI(this._movedURIs[i], [this._targetTag]); > >+ PlacesUtils.tagging.tagURI(this._movedURIs[i], [this._sourceTag]); > >+ } > >+ this._movedURIs = null; > >+ } > >+ } > >+}; > > don't we always want to store the list of affected URIs, and only apply the > change to that set for undo/redo? for undo, the list of tagged URIs for a new > tag could be different than when the transaction was first committed. Not if you've used this service to commit this transactions (it's a stack). I don't think we should take care of direct calls to the tagging or bookmarking services within the PTM service, other than its own calls. > >+ /** > >+ * Moves all the URIs tagged with one or more tags to another tag. > >+ * > >+ * @param aSourceTags > >+ * Array of tags from which URIs may be moved. > >+ * @param aTargetTag > >+ * the target tag, with which URIs might be tagged. > >+ * @returns an array of URIs which were tagged with aTargetTag as a result > >+ * of calling this method (Note this opereation never results > >+ * in duplicated URIs under aTargetTag). > >+ */ > >+ nsIVariant moveURIs(in nsIVariant aSourceTags, in AString aTargetTag); > > }; > > moveURIs describes the back-end implementation, not the action. maybe > mergeTags, or migrateTags? I disagree on that, but migrateTags works too. > >+ // uris needs to be "moved". In this case, the old-tag container can be > >+ // simply renamed. > >+ if (aSourceTags.length == 1) { > >+ var oldTagNode = this._getTagNode(aSourceTags[0]); > >+ if (!oldTagNode) // Nothing to do > >+ return; > >+ > >+ this._bms.setItemTitle(oldTagNode.itemId, aTargetTag); > >+ return this._getURIsForTagNode(oldTagNode); > >+ } > >+ } > >+ > >+ // Otherwise, move the items (but make sure not to create dupes!) > >+ // > >+ // urisUnderTargetTag has a couple of purposes: > >+ // - dynamically keeping track of all uris under the target-tag container, > >+ // to avoid dupes. > >+ // - starting after initialTargetArraySize (which is the number of uris > >+ // tagged with the target-tag _before_ calling this method), it stores > >+ // the return value of this method, which is an array of all uris > >+ // which were actually moved from the source tags. > >+ var urisUnderTargetTag = targetTagNode ? > >+ this._getURIsForTagNode(targetTagNode) : []; > >+ var initialTargetArraySize = urisUnderTargetTag.length; > >+ > >+ var itemsToMove = []; > >+ var tagsToRemove = []; > >+ for (var i=0; i < aSourceTags.length; i++) { > >+ var oldTagNode = this._getTagNode(aSourceTags[i]); > >+ oldTagNode.containerOpen = true; > >+ var cc = oldTagNode.childCount; > >+ for (var j=0; j < cc; j++) { > >+ var uriNode = oldTagNode.getChild(j); > >+ var uriObject = gIoService.newURI(uriNode.uri, null, null); > >+ > >+ // only move the item if the uri isn't already set under the target > >+ // tag > >+ if (urisUnderTargetTag.every(function(uriElt) { return !uriElt.equals(uriObject); })) { > > wouldn't hrn? > >+ > >+ // remove the source tags > >+ for (i=0; i < tagsToRemove.length; i++) > >+ this._bms.removeFolder(tagsToRemove[i]); > >+ > >+ // return all the uris which were tagged > >+ return urisUnderTargetTag.slice(initialTargetArraySize); > >+ }, > > a couple of things: > > 1. i don't like how this avoids using the tag/untag methods of the service, in > favor of using the bookmarks service directly. this makes future changes to how > tagging is done more difficult, as the code is less centralized. > Using the public methods is expensive as that would result in locating the tag node over and over again. > 2. given the potential for a large number of database writes, should batch > this. ok.

(In reply to comment #6) > > 1. i don't like how this avoids using the tag/untag methods of the service, in > > favor of using the bookmarks service directly. this makes future changes to how > > tagging is done more difficult, as the code is less centralized. > > > > Using the public methods is expensive as that would result in locating the tag > node over and over again. > Plus I would really like not to loose dateAdded/lastModifieid data.

Comment on attachment 330800 [details] [diff] [review] v2 >- editItemTitle: function placesEditItmTitle(id, newTitle) { >+ editItemTitle: function placesEditItemTitle(id, newTitle) { >+ var parent = PlacesUtils.bookmarks.getFolderIdForItem(id) >+ if (parent == PlacesUtils.tagsFolderId) >+ return new placesRenameOrMergeTagTransactions(PlacesUtils.bookmarks.getItemTitle(id), newTitle); >+ nit: line-length >+placesRenameOrMergeTagTransactions.prototype = { >+ __proto__: placesBaseTransaction.prototype, >+ >+ doTransaction: function PROMTT_doTransaction() { >+ if (PlacesUtils.tagging.tagExists(this._targetTag)) { >+ // complex case - undoing this transaction should separate back the tags! >+ this._movedURIs = PlacesUtils.tagging.migrateTags([this._sourceTag], >+ this._targetTag); >+ } >+ else >+ PlacesUtils.tagging.moveURIs([this._sourceTag], this._targetTag); >+ }, s/moveURIs/migrateTags/ also, please add a test to test_placesTxn.js. it would've caught this. >+ >+ /** >+ * Returns whether or any uri is tagged with the given tag name. s/or any/or not any/ >+ /** >+ * Migrates all the URIs tagged with one or more tags to another tag. >+ * >+ * @param aSourceTags >+ * Array of tags from which URIs may be moved. >+ * @param aTargetTag >+ * the target tag, with which URIs might be tagged. s/might/will >+ * @returns an array of URIs which were tagged with aTargetTag as a result >+ * of calling this method (Note this opereation never results s/opereation/operation r=me with tests added

Asaf, Dietrich, what is the current state of this bug? The patch is nearly ready to land but the bug is lingering for more then half a year.

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv

(In reply to Henrik Skupin (:whimboo) from comment #10) > Asaf, Dietrich, what is the current state of this bug? The patch is nearly > ready to land but the bug is lingering for more then half a year. I've set bug 913832 as duplicate of this one even if I'm not really sure of the current state of this bug.

Hi Marco, is this patch even remotely usable anymore?

not this patch, Mano is rewriting the transactions manager at this time, so it will require something different.