444849 - Removing a bookmark doesn't clean up tags

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Todd Agulnick]

If you use nsINavBookmarksService.remove(At) to delete a bookmark and that bookmark was the last item with one or more tags, those tags should be purged but they are not.

Note that the Places Organizer implements this functionality in http://mxr.mozilla.org/mozilla-central/source/browser/components/places/src/nsPlacesTransactionsService.js#517, but clients of nsINavBookmarksService would be better served by having this functionality baked into that API -- otherwise every extension that wants to delete a bookmark has to implement the same logic.

Comment 1

[Marco Bonardo [:mak]]

the way tags are managed is not pretty and makes this a mess. I agree that is should be like that, but at the same time tag management should be done through the tagging service for clarity. So, probably future, when we will change tags implementation, for now we could base on preventive maintanance to remove childless tags

Comment 2

[Shawn Wilsher :sdwilsh]

Doesn't the tag service already have a bookmark listener?  We could just listen for item removed, and see if anything that has the same url has a tag?

Comment 3

[Shawn Wilsher :sdwilsh]

Do we even want to do that anymore?  With tag autocomplete, past used tags could be useful to have around still...

Comment 4

[Dietrich Ayala (:dietrich)]

Given our current model, I consider leftover tags a privacy exposure. We should either enforce the "tags are properties of bookmarks" model, or ditch it altogether. I think the former is the only realistic option.

Should add tag removal to the bookmark apis, as well as expiration.

Comment 5

[Marco Bonardo [:mak]]

if we go for tags only for bookmarks, changing the db schema could probably also solve this issue, and probably with a trigger.

Comment 6

[Brian Carpenter [:geeknik]]

I've noticed this in all of the 3.1b3pre builds and now the b4pre build. I'm sure it exists in 3.0.x as well but I haven't used those versions in a long time.  If you delete bookmarks that are tagged and you click on the Smart Bookmarks option in the bookmarks toolbar and open recent tags, there are a lot of tags that are "empty" because the associated bookmark has been deleted. I don't see a need to hang on to tags that are empty because what if you never use that tag again? It's just there taking up space. I say if the bookmark is removed, all associated data should be removed with it and that includes tags.

Comment 7

[Michael Kohler [:mkohler]]

How about checking after the deletion of a bookmark if the tags used for this bookmark are empty/childless? If so, we just delete these tags. Would this be the right approach?

Comment 8

[Christopher-J Carlson]

I had never used the tagging system before it was referenced during the release of a new build of Firefox 3. If I remember correctly, the reference seemed quite laudatory, which was the reason I finally tried tagging. (Sorry for not being able to remember which specific build this was.)

So, I tagged all my bookmarks. Then I did some editing; removing tags from some bookmarks, creating new tags for others, deleting some bookmarks in the process. Then I discovered all the childless tags. WTH?! *sigh*

Now, I had wrongly assumed a few things that seemed like common sense to me, but I'm not well-versed in program creation and such. I just use these programs. However, not being able to *delete any tags* (even blank ones)... Well, that just seemed wrong; *morally* as well as technically.

In the end, I re-installed Firefox. But, since my tagged bookmarks had been auto-synced to Delicious, I had to manually delete (on my Delicious page) all tags attached to my bookmarks before importing them back to Firefox. (Thank Jebus I tend to be frugal when it comes to creating bookmarks.)

Haven't gone near Firefox tags since, and I don't plan to until I get official and detailed confirmation that they won't take over Firefox like tribbles next time around.

Anyhoo...

Comment 9

[Drew Willcoxon :adw]

Attachment: WIP v1

Takes the approach of comment 2.  Observes bookmark removals in the tagging service's bookmarks observer, checks the parents of the item being removed, if no more non-tag parents, untags the URI.

Comment 10

[Drew Willcoxon :adw]

Attachment: for review v1

I've tried to make sure that as little code and queries as possible are run in the remove callbacks, since those callbacks are run every time a bookmark item is removed.  This patch adds one call to nsINavBookmarksService.getBookmarkURI() every time any item is removed and one call to getBookmarkIdsForURI() and potentially many calls to getFolderIdForItem() every time a bookmark is removed.  I ran the test browser/components/places/tests/perf/perf_large_delete.xul several times with the patch and without and there was no difference.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 373961 [details] [diff] [review]
for review v1

>diff --git a/toolkit/components/places/src/nsTaggingService.js b/toolkit/components/places/src/nsTaggingService.js

i see why you did not notice me and dietrich already worked on this service, please add all of three us to the contributors header, we did quite some change.

>--- a/toolkit/components/places/src/nsTaggingService.js
>+++ b/toolkit/components/places/src/nsTaggingService.js
>@@ -339,43 +339,88 @@ TaggingService.prototype = {
>   // nsIObserver
>   observe: function TS_observe(aSubject, aTopic, aData) {
>     if (aTopic == "xpcom-shutdown") {
>       this._bms.removeObserver(this);
>       this._obss.removeObserver(this, "xpcom-shutdown");
>     }
>   },
> 
>+  /**
>+   * If aURI is contained only in tag folders, returns the IDs of those
>+   * folders.  Returns null otherwise.
>+   *
>+   * @returns null or an array of tag IDs
>+   */

missing @param

>+  _getTagsForOrphanedBookmark: function TS__getTagsForOrphanedBookmark(aURI) {

i don't like much the name of this method, what about getTagsIfUnbookmarkedURI or something like that?
This is not real an orphan bookmark, we want to get tags for an history-only uri.
Please better expand the method comment above with more informations.

>+  onItemRemoved: function(aItemId, aFolderId, aIndex) {
>+    var itemURI = this._itemsInRemoval[aItemId];
>+    delete this._itemsInRemoval[aItemId];
>+
>+    // Item is a tag folder.
>     if (aFolderId == this._bms.tagsFolder && this._tagFolders[aItemId])
>       delete this._tagFolders[aItemId];
>+
>+    // Item is a bookmark that was removed from a non-tag folder.
>+    else if (itemURI && !this._tagFolders[aFolderId]) {
>+      var tagIds = this._getTagsForOrphanedBookmark(itemURI);
>+
>+      // If bookmark is contained in only tag folders, untag it.

could you expand this comment a bit? looks like a bit cryptic, or send to the method documentation and expand that.

> 
> // Implements nsIAutoCompleteResult
> function TagAutoCompleteResult(searchString, searchResult,
>diff --git a/toolkit/components/places/tests/unit/test_childlessTags.js b/toolkit/components/places/tests/unit/test_childlessTags.js
>+      print("  Remove the bookmark.  The tags should no longer exist.");
>+      bmsvc.removeItem(bmId);
>+      tags.forEach(function (tag) {
>+        var tagId = bmsvc.getChildFolder(bmsvc.tagsFolder, tag);

I don't like this because relies on current tags implementation (being bookmarks folders), that is not carved in stone.
What about building a RESULTS_AS_TAG_QUERY query and check if the tag appear there? Should allow you to obtain the same result without relying on tags implementation, then if we change the implementation or the query this test would still work.

Comment 12

[Drew Willcoxon :adw]

Attachment: for checkin

Addressed comment 11

Comment 13

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/ccae2424e1c9

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 374289 [details] [diff] [review]
for checkin

This is an useful change for 1.9.1 and UI coherence, minor risk, has test.

Comment 15

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090505 Minefield/3.6a1pre

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 374289 [details] [diff] [review]
for checkin

a191=beltzner

Comment 17

[Marco Bonardo [:mak]]

Attachment: 1.9.1 patch

1.9.1 needs special QI to nsINavBookmarksObserver_1.9.1_ADDITIONS

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2645c8243404

Comment 19

[Aakash Desai [:aakashd]]

verified FIXED on Shiretoko:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090526 Shiretoko/3.5pre ID:20090526031155

Comment 20

[Gervase Markham [:gerv]]

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