429350 - Unable to delete tags in left column list

Status: VERIFIED FIXED

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

Description

[Marcia Knous [:marcia]]

Apologies if this is a dupe, but I could not find an existing bug. Seen while testing Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041604 Minefield/3.0pre.

STR:
1. Add some tags.
2. Open Organize Bookmarks.
3. Expand the tag folder dropdown
4. Either context click to delete, or Edit | Delete and try to remove the bookmark.

Expected: It would delete the tag.
Actual: It keeps the tag. If I place focus on the bookmark, it deletes it.

The odd thing is that all of the other items on the left hand side seem to be able to be deleted excepts tags (RSS feeds, The Entire Recent Tags folder, etc)

Comment 1

[Tracy Walker [:tracy]]

I found on Win XP from 20080423 the only way to delete a tag is to manually remove it from every bookmark that is tagged with it.  blocking?

Comment 2

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

Marco/Mano: what's involved in fixing this? We'd basically need to erase the tag container, right?

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes but i think that it would be too easy for the user to delete by error one tag and loose his organization work. So imo we should remove the delete option and the tag should disappear when there are no more tagged items inside it

Comment 4

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

What's the cost of supporting undo?

I also disagree that the potential for dataloss here is high - delete in the context menu would be something the user is looking for, and there's really no other way to get rid of a tag other than to root it out of the properties field in every bookmark that uses it. That means if I decide I want to get rid of my "Marco" tag (I never would, fwiw!) it's really troublesome to do so.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

if you think this is a needed polish i will take a look

Comment 6

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

After a lot of consideration, I don't think this blocks. However, it is a really-really-really-want, as without it, deleting tags is extremely painful.

Comment 7

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

(is this actually a regression?)

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #7)
> (is this actually a regression?)

yes, we were using real folders before so they were deleted like common bookmarks folders. So most likely regression from bug 419731

Comment 9

[Damian Shaw [Quan]]

If this isn't going to be fixed, wouldn't it be better to remove "delete" from the context menu as not to confuse the user?

Comment 10

[Tom Ledger]

Tags with no bookmarks also can't be deleted.

Comment 11

[Tong Ti Wun]

I tried deleting the tags by editing it and no left with 2 '(no title)' tags in my bookmarks and tagged bookmarks could not be deleted. This is pretty annoying.

Comment 12

[Tracy Walker [:tracy]]

I sort of see this with but a single tag on Mac 10.4. However, the profile has been around since the alpha builds. Other than the one tag which seems to be corrupt, I can delete tags by removing the tag from every bookmark it's associated with.

Can anyone reproduce with a clean profile?

Comment 13

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

Tested again with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008051206 Firefox/3.0

It's still a problem. You are not able to delete a tag from within the left pane.

Comment 14

[Kyle Dobbs]

I understand not allowing tags to be deleted because of the potential for an accident.  The REAL bug, IMHO, is the fact that if you somehow create a (no title) tag, there is no way to delete it.  I removed all of the bookmarks that were allowing these tags to persist, but the tags remain.  

If there is no way to prevent creating these (no title) tags, then perhaps there should be a delete tag feature, perhaps requiring clicking a few "yes, I understand the ramifications of this and I still want to do it" prompts.  As it stands, the only solution I see is to delete all of my bookmarks and manually import and tag them again.

Comment 15

[Sharpie]

As there is no solution yet, I have decided to post a workaround I discovered:
1) Backup your bookmarks (export them somewhere) and close Firefox.
2) Manually delete the file named "places.sqlite" in your profile folder.
3) Open Firefox and import your bookmarks.

This is by no means a solution, but only a workaround to get rid of those tags you can't delete. I really hope this will get fixed before the final release.

Comment 19

[SilverWave]

I can confirm the same problem. Ubuntu 8.04 64bit - Firefox 3rc2

I would advise that this is given a high priority as users are coming up with some of their own home brew solutions.

Quote:
ctrl+shift+b , and click on the tag you want to remove
if right-click delete doesn't work, then remove the tag from bookmarks using it.

That should auto-remove the tag once its no longer used. If it still doesn't delete, then:

(1) download extension: SQLLite: https://addons.mozilla.org/en-US/firefox/addon/5817
(2) run it.
(3) top right of screen, choose places.sqllite , click go. ( Don't have to navigate ' my computer' its a shortcut )
(4) select Tables -> moz_bookmarks
(5) click search, enter title="nameOfTag" ( type name of tag here, don't have to rename it. I think case mattered, not 100% though. )
(6) right click -> delete.  
End Quote

BTW this worked as a fix for my tags problem as well ;)

I wont be tagging anything else until a more robust and proven system is in place. (Pun intended).

Comment 20

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

Could this at least be fixed for firefox3.1?

Comment 21

[Dietrich Ayala (:dietrich)]

mano, do you have cycles for this for 3.1?

Comment 22

[Pete]

I think its a fairly condescending attitude.  "we can't add this, people would be too stupid to  use it". If I select Delete, its because I want to be delete.
If I select delete because I expect it not to happen I would be using a Microsoft product.

Comment 23

[Asaf Romano (gone)]

Yes yes.

Comment 24

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

Please see bug 448584 which is caused at least by this bug and results in dataloss.

Comment 25

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

For a testcase see my example sqlite database (attachment 331784 [details])

Comment 26

[Paul J. Weiss]

I have some tags that I use to indicate the status of bookmarks to a particular project. There are times when a tag is not in use in any bookmark, but I do not want it deleted, because I want the tag to remain available for future use. So can we have a user-set option on whether or not to delete a tag upon deletion of its last use?

Comment 27

[Gonzalo HIGUERA DÍAZ]

(In reply to comment #26)
> can we have a user-set option on whether or not to delete a tag upon deletion
> of its last use?

You should open a new bug report because it is a different topic and it will be better tracked. (If you then find that your report is pertinent in some discussion simply use the bug number when commenting.)

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Mano are you still woring on this?
i'd like to take a look, this is due to corrupt bookmarks/folders that are into tag folders by error, probably due to 3.0 alpha stage bugs... i think we should do a one time cleanup of those dangling entries and delete empty tags after that... and clearly check if it's still possible in current version to finish up into such a situation, even if i think we're more robust on new profiles.

Comment 29

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

Mano i hope you don't mind if i take this

the patch actually only untag all uris when we choose delete upon a tag query in the library. In all other places we should instead remove the special folder (so if i delete a tag special folder in the toolbar we will not untag all uris).

this patch requires fix for bug 458019 to use getURIsForTag with a concrete item id.

Also notice this patch cannot fix bogus tag containers, in alpha stage we were allowing insertion of folders, separators and other things into tag containers, and that is causing tags from not disappearing. To fix that we need to implement some sort of cleanup background task, we are trying to remove cleanup stuff from the history service and putting that there actually would be blocked to not unbitrot the fsync changes, so most likely the cleanup should be done in bug 431558.

Comment 30

[orangecrayon]

Firefox 3.03
After manually deleting the tag word in the bookmark (right hand side), the tag name under the Tag section (left side) changes to "No Title" and that remains as No Title even after rebooting. The bookmark also remains listed on the right hand.

Sensible way to me, is simply to make the bookmark "disappear" from the Tag section once the tag or tags removed in its panel (meaning it will still remain wherever it has been bookmarked, the folder or unsorted), and once there are no more bookmarks under a Tag, then it can be just deleted using the the right-click if wished.

If there is a solution to use in meanwhile, please just post one very simple easy to understand method not several confusing hard to understand ways "hidden" in a bug! Thank you.

Comment 31

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

going to change
+        var tagItemId = PlacesUtils.getConcreteItemId(node);
+        var URIs = PlacesUtils.tagging.getURIsForTag(tagItemId);

to
var URIs = PlacesUtils.tagging.getURIsForTag(node.title);

to avoid tagging api change

Comment 32

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

Comment 33

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

after asking permission to Mano in IRC, reassigning to me

Comment 34

[Asaf Romano (gone)]

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

>diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js
>--- a/browser/components/places/content/controller.js
>+++ b/browser/components/places/content/controller.js
>@@ -874,21 +874,32 @@ PlacesController.prototype = {
>     for (var i = 0; i < range.length; ++i) {
>       var node = range[i];
>       if (this._shouldSkipNode(node, removedFolders))
>         continue;
> 
>       if (PlacesUtils.nodeIsFolder(node))
>         removedFolders.push(node);
>       else if (PlacesUtils.nodeIsTagQuery(node.parent)) {
>-        var queries = asQuery(node.parent).getQueries({});
>-        var folders = queries[0].getFolders({});
>+        var tagItemId = PlacesUtils.getConcreteItemId(node.parent);
>         var uri = PlacesUtils._uri(node.uri);
>-        var tagItemId = folders[0];
>         transactions.push(PlacesUIUtils.ptm.untagURI(uri, [tagItemId]));
>+        continue;
>+      }
>+      else if (PlacesUtils.nodeIsTagQuery(node) && node.parent &&
>+               PlacesUtils.nodeIsQuery(node.parent) &&
>+               asQuery(node.parent).queryOptions.resultType ==
>+                 Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY) {
>+        // Untag all URIs tagged with this tag only if the tag container is
>+        // child of the "Tags" query in the library, in all other places we
>+        // must only remove the query node.
>+        var tag = node.title;
>+        var URIs = PlacesUtils.tagging.getURIsForTag(tag);
>+        for (var j = 0; j < URIs.length; j++)
>+          transactions.push(PlacesUIUtils.ptm.untagURI(URIs[j], [tag]));

This ought to be batched, better if you make that only for > N urls.

Comment 35

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

(In reply to comment #34)
> >+      else if (PlacesUtils.nodeIsTagQuery(node) && node.parent &&
> >+               PlacesUtils.nodeIsQuery(node.parent) &&
> >+               asQuery(node.parent).queryOptions.resultType ==
> >+                 Ci.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY) {
> >+        // Untag all URIs tagged with this tag only if the tag container is
> >+        // child of the "Tags" query in the library, in all other places we
> >+        // must only remove the query node.
> >+        var tag = node.title;
> >+        var URIs = PlacesUtils.tagging.getURIsForTag(tag);
> >+        for (var j = 0; j < URIs.length; j++)
> >+          transactions.push(PlacesUIUtils.ptm.untagURI(URIs[j], [tag]));
> 
> This ought to be batched, better if you make that only for > N urls.

humm, but this is doing transactions.push, all transactions are aggregated and executed in removeRowsFromBookmarks, and an aggregate transaction is batched.

Comment 36

[Dietrich Ayala (:dietrich)]

Approving blocking 3.1 for behavioral consistency of UI, and because it'd provide much utility to users of tags.

Comment 37

[Asaf Romano (gone)]

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

hrm, right. r=mano.

Comment 38

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/4ffb7e96343f

notice this will not delete bogus tag containers (those will be fixed apart)

Comment 39

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

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081017 Minefield/3.1b2pre ID:20081017185951

Marco, do we have any test in the test-suite? Or shall we also create a Litmus test?

Comment 40

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

mh litmus would be preffered imho, the manual test is really easy to do and results are rather visible and clear, if you delete a tag container in the library (or from most recent tags, i did not tried really but should work here as well) all contained uris should be untagged, if you delete it as a special query (for example copying the tag container to the toolbar) uris show not be untagged since you are only removing a copy of the container.

Comment 41

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=7062 added to Litmus, under the Library FFT set.

Comment 42

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

Sorry Marco, but I have to reopen the bug. The deletion of tags only works for the left side (tree). I'm still not able to delete tags when selecting the Tags container and trying to delete them from within the right pane.

Comment 43

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

mh, that should not be complex to fix, thank you for noticing that

Comment 44

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

mh actually the code is correct, the problem is that Tags and RecentTags are
queryType = QUERY_TYPE_HISTORY, so when deleting them they are passed to
_removeRowsFromHistory.

I'm unsure if we should upgrade both smart bookmarks and left pane folder to
set the correct query type, or detect on delete if the query is RESULTS_AS_TAG
and call _removeRowsFromBookmarks instead

if the former we should file another bug, if the latter we can fix it here.

Comment 45

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

"so when deleting them they are passed to _removeRowsFromHistory."
actually this happens only when these queries are roots in the Library since we choose the delete function based on the root.

Comment 46

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: tag queries patch

Actually we don't tell the user that TAG_QUERY and TAG_CONTENTS should be QUERY_TYPE_BOOKMARKS (and if you set this queryType to TAG_QUERY it is EMPTY because we wrongly try to filter it!), most likely if the user will create a query like place:type=7&folder=X it will work in a bad way showing duplicates, moreover TAG_QUERY adds an history observer that is completely useless since it's bookmarks related and should instead create an all bookmarks observer...

So i suggest forcing the queryType to QUERY_TYPE_BOOKMARKS for these 2 special result types, they are completely bookmarks related, and defining a different queryType does not make any sense. Imho this is more consistent than adding another special case in the frontend removal code.

Mano, what do you think? Imho we should also move this patch to a new bug and remark this as fixed, since it is a different issue effectively.

Comment 47

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 343707 [details] [diff] [review]
tag queries patch

moving patch to a new bug

Comment 48

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the original issue is fixed (so marking this as fixed), there's an underlying issue due to bad definition of tag queries, that will be addressed in a new bug blocking this, and will solve the right pane issue.

Comment 49

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i filed bug 460830

Comment 50

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

As mentioned in comment 42 this is working for the places list (left tree).

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081019 Minefield/3.1b2pre ID:20081019020302

Comment 54

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

Sorry Marco, but I have to reopen this bug. At least I missed a point when I tested it on OS X. With the latest build you are not able to delete tags by using the toolbar button and selecting delete. This issue only occurs on OS X. On Windows it's fine. There is no error shown in the error console. I'm able to reproduce with a fresh profile. So no bogus entries are stored in the places.sqlite.

Comment 55

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

could you file a new mac bug about the button then please?

Comment 56

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

Sure. It's covered by bug 469540 now.

Closing this bug again and marking it verified.

Comment 57

[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