411088 - when deleting a tagged bookmark from the places organizer, the tag remains

Status: VERIFIED FIXED

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

Description

[(not reading, please use [e-mail] instead)]

when deleting a tagged bookmark from the places organizer, the tag remains

thanks to henrik for catching this.  (see bug #408780)

steps to reproduce:

1)  create a new profile
2)  visit http://www.google.com
3)  click the star button to bookmark that page
4)  click the star button again to tag the page with foo
5)  open the places organizer and delete the bookmark

in the "recent tags" smart bookmarks query, "foo" and "http://www.google.com" will remain.  you can also see "foo" and  "http://www.google.com" in the places organizer under "Tags".

note, the same things appears to happen if you create / delete the bookmark from the personal toolbar folder.

note, if you use the delete button from the "star" popup, this does not happen.

Comment 1

[(not reading, please use [e-mail] instead)]

dietrich wrote:  "Tags are not deleted when deleting a page from history."

That explains what I'm seeing in comment #0. 

It appears the reason that deleting from "star" popup doesn't act the same way is because of this code:

http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser-places.js#346

342   deleteButtonOnCommand: function PCH_deleteButtonCommand() {
343     PlacesUtils.bookmarks.removeItem(gEditItemOverlay.itemId);
344 
345     // remove all tags for the associated url
346     PlacesUtils.tagging.untagURI(gEditItemOverlay._uri, null);
347 
348     this.panel.hidePopup();
349   }

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

This is potentially a privacy problem, if users are deleting embarrassing bookmarks that contain embarrassing tags, the are going to expect the tags to be removed as well.

Comment 3

[Asaf Romano (gone)]

My plan is to make the delete command unset tags for the uri if the removed bookmark is the most recent bookmark for its uri.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #2)
> This is potentially a privacy problem, if users are deleting embarrassing
> bookmarks that contain embarrassing tags, the are going to expect the tags to
> be removed as well.

Definitely. I set the privacy keyword.

(In reply to comment #3)
> My plan is to make the delete command unset tags for the uri if the removed
> bookmark is the most recent bookmark for its uri.

Will that also handle the deleting of the last bookmark for this uri, e.g. like a ref counter? Afterwards the uri shouldn't appear anymore and be removed.

Comment 5

[Asaf Romano (gone)]

The most recent bookmark for a url is the last (modified || added) for the url. If you have multiple bookmarks for the same uri, we'll unset the tags whenever the very last bookmark (excluding the internally-used items under tag containers, that is) for the url is removed.

Comment 6

[Tracy Walker [:tracy]]

The more I use the tagging system, the more I think this is a bad idea.  Tags shouldn't be handcuffed to a particular bookmark (the last and only bookmark with that tag).  

I may really like a tag (foo).  For some reason, I have removed every bookmark that is marked with tag foo. Then I find a new page I want to bookmark and tag with foo.  If this bug we're fixed, foo would be gone from the list. I'd wonder where the heck it went, then realize I have to create it again. I think many users will unintentionally remove wanted tags (data loss).

For those with privacy concerns, you'll have to clean up to your bookmarks, history and likely your cache to do any reasonable tracks erasing.  So if you add an embarrassing tag, remove it from the Library. Don't expect it to be cleaned up for you.

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

Ok, I agree with Tracy.  I was primarily thinking about users entering tags in the text field, but in the case of checking them off in the expanded view, I can see people being confused if a tag is suddenly missing, since the removal is an implicit action.

>So if you add an embarrassing tag, remove it from the Library.

Yeah, this seems reasonable.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

But still keep in mind that if you have an embarrassing bookmark with tags and you delete this bookmark, it definitely shouldn't be listed anymore under any of the tags. It really doesn't exist anymore.

Comment 9

[Asaf Romano (gone)]

But still keep in mind that if you've multiple copies of an embarrassing bookmark, there's nothing we can do but let your family/co-workers know what were your associations for that bookmark, until you remove the very last copy. Sorry.

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

>But still keep in mind that if you have an embarrassing bookmark with tags and
>you delete this bookmark, it definitely shouldn't be listed anymore under any
>of the tags. It really doesn't exist anymore.

Yeah, delete needs to delete, not just remove the tag.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #9)
> But still keep in mind that if you've multiple copies of an embarrassing
> bookmark, there's nothing we can do but let your family/co-workers know what
> were your associations for that bookmark, until you remove the very last copy.

That's still bad. If you create a new bookmark, tag this bookmark and duplicate it afterwards with another name only the first added bookmark is shown under the appropriate tag. That's what you said Asaf. BUT IMO this behavior will really confuse people. If you have two or more bookmarks pointing to the same URI and have given them deliberately different names, people will ask where the remaining bookmarks are. I'm not sure if showing entries under tags based on the URI only is good. Using real existing bookmarks therefor should also fix this bug. No bookmark no entry. And if you have multiple copies of an URI only the remaining bookmarks are listed. 

Comment 12

[Asaf Romano (gone)]

FWIW, there's absolutely no bookmark-under-tags concept, we (sort of) show the history entries under tags (with history titles).

Comment 13

[Asaf Romano (gone)]

One thing we could do is to associate the tags of a given uri with the last bookmark they were edited from.  That is, say you set some tags for bookmark A, duplicate it as bookmark B, then remove B, we won't unset the tags unless you changed them by editing bookmark B info.

Comment 14

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

OK, after getting Mano to explain this to me, and talking with mconnor, I think this is the way we should go:

Scenario:
 - ab.com is bookmarked by "Marker" and "Pointer"

(note: this scenario primarily comes about from migrated bookmark files - it's pretty hard to do this in Firefox 3, otherwise)

From bookmarking dialog ..:
 - adding a tag adds to "Marker" and "Pointer"
 - deleting the bookmark deletes "Marker" and "Pointer" (since that user has said they don't want that page bookmarked anymore)
 - read current location & set location of the most recently created bookmark

From places organizer (where one bookmark is selected) ..:
 - adding a tag adds to "Marker" and "Pointer" (limitation of our back-end)
 - deleting the bookmark only deletes the one selected

When the last URI is removed from a tag container, the tag container should be removed. This is similar to what flickr and del.ic.ious do.

So, for this bug, that means that when you delete a tagged bookmark, if that eliminates the last instance of that tag, then no matter where you're doing the delete action, the tag should not remain.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

I agree with beltzner.  In addition to external consistency with other tag systems (flickr, del.ic.ious), I am also worried about large numbers of tags accumulating over time that are not associated with any bookmarks.  Users would have to in an manually manage the tags every once in awhile.  This goes against the primary design objective of creating a system that allows you to organize as you browse, instead of having to switch into organize-mode.

Comment 16

[Asaf Romano (gone)]

Attachment: patch

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 300568 [details] [diff] [review]
patch


>+
>+  /**
>+   * Get all bookmarks for a URL, excluding items under tag or livemark
>+   * containers.
>+   */
>+  getBookmarksForURI:
>+  function PU_getBookmarksForURI(aURI) {
>+    var bmkIds = this.bookmarks.getBookmarkIdsForURI(aURI, {});
>+    for (var i = 0; i < bmkIds.length; i++) {
>+      var parent = this.bookmarks.getFolderIdForItem(bmkIds[i]);
>+      // Livemark child
>+      if (this.annotations.itemHasAnnotation(parent, LMANNO_FEEDURI)) {
>+        bkmIds.splice(i, 1);
>+        i--;
>+      }
>+      else {
>+        var grandparent = this.bookmarks.getFolderIdForItem(parent);
>+        // item under a tag container
>+        if (grandparent == this.tagsFolderId) {
>+          bmkIds.splice(i, 1);
>+          i--;
>+        }
>+      }
>+    }
>+    return bmkIds;
>+  },

ugh. would be nicer to use Array.filter().

ugh. extension developers are going to beat us for stuff like this.

sooner or later we're going to have to make helpers like IsLivemarkChildItem() and IsTagItem(). or IsReallyARegularBookmark().

Comment 18

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

Comment on attachment 300568 [details] [diff] [review]
patch

a=beltzner, good cleanup for beta 3

Comment 19

[Asaf Romano (gone)]

Attachment: use Array.filter

Comment 20

[Asaf Romano (gone)]

Attachment: again

Comment 21

[Dietrich Ayala (:dietrich)]

Comment on attachment 300670 [details] [diff] [review]
again

nice, thanks.

Comment 22

[Asaf Romano (gone)]

mozilla/browser/base/content/browser-places.js 1.89
mozilla/browser/components/places/content/utils.js 1.99
mozilla/browser/components/places/src/nsPlacesTransactionsService.js 1.14

Comment 23

[Asaf Romano (gone)]

For the litmus test, see comment 14.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is not completely fixed. I just run a test with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008020104 Minefield/3.0b3pre ID:2008020104

With following steps the tag still remains within the Library:

1. Add a bookmark and tag it
2. Open the Library and go to Tags
3. Click on the appropriate Tag and delete the bookmark

The tag gets removed from the bookmark but still remains under Tags.

Comment 25

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

Comment 24 is true, this isn't entirely fixed, but the part that isn't doesn't need to block Firefox 3 Beta 3 IMO, and instead blocks Beta 4.

FWIW, deleting the tag itself from the places organizer still works. That's not surprising, I guess, but if we needed to publish a workaround ...

Comment 26

[Asaf Romano (gone)]

Attachment: patch

Comment 27

[Dietrich Ayala (:dietrich)]

Comment on attachment 301405 [details] [diff] [review]
patch


>@@ -121,9 +126,10 @@ function run_test() {
> 
>   // test untagging
>   tagssvc.untagURI(uri1, ["tag 1"]);
>   do_check_eq(tag1node.childCount, 1);
> 
>   // removing the last uri from a tag should remove the tag-container
>   tagssvc.untagURI(uri2, ["tag 1"]);
>   do_check_eq(tagRoot.childCount, 1);
>+  
> }

nit: whitespace

>@@ -861,27 +874,40 @@ placesSortFolderByNameTransactions.proto
>     for (item in this._oldOrder)
>       PlacesUtils.bookmarks.setItemIndex(item, this._oldOrder[item]);
>   }
> };
> 
> function placesTagURITransaction(aURI, aTags) {
>   this._uri = aURI;
>   this._tags = aTags;
>+  this._unfiledItemId = -1;
>   this.redoTransaction = this.doTransaction;
> }
> 
> placesTagURITransaction.prototype = {
>   __proto__: placesBaseTransaction.prototype,
> 
>   doTransaction: function PTU_doTransaction() {
>+    if (PlacesUtils.getBookmarksForURI(this._uri).length == 0) {
>+      // Force an unfiled bookmark first
>+      this.__unfiledItemId =
>+        PlacesUtils.bookmarks
>+                   .insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
>+                                   this._uri, -1,
>+                                   PlacesUtils.history.getPageTitle(this._uri));
>+    }
>     PlacesUtils.tagging.tagURI(this._uri, this._tags);
>   },
> 
>   undoTransaction: function PTU_undoTransaction() {
>+    if (this.__unfiledItemId != -1) {
>+      PlacesUtils.bookmarks.removeItem(this.__unfiledItemId);
>+      this.__unfiledItemId = -1;
>+    }
>     PlacesUtils.tagging.untagURI(this._uri, this._tags);
>   }
> };

you initialize _unfiledItemId, but thereafter refer to __unfiledItemId.

r=me otherwise.

Comment 28

[Asaf Romano (gone)]

mozilla/browser/components/places/src/nsPlacesTransactionsService.js 1.15
mozilla/toolkit/components/places/public/nsITaggingService.idl 1.10
mozilla/toolkit/components/places/src/nsTaggingService.js 1.9
mozilla/toolkit/components/places/tests/unit/test_tagging.js 1.8

Comment 29

[Asaf Romano (gone)]

tree was closed, backed out

Comment 30

[Asaf Romano (gone)]

mozilla/browser/components/places/src/nsPlacesTransactionsService.js 1.17
mozilla/toolkit/components/places/public/nsITaggingService.idl, 1.12
mozilla/toolkit/components/places/src/nsTaggingService.js 1.11
mozilla/toolkit/components/places/tests/unit/test_tagging.js 1.10

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Now it's fixed. Thanks Asaf.

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko Minefield/3.0b4pre ID:2008020623

Comment 32

[Tracy Walker [:tracy]]

added to litmus

Comment 33

[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