Closed Bug 429350 Opened 12 years ago Closed 12 years ago

Unable to delete tags in left column list

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3.1b2

People

(Reporter: marcia, Assigned: mak)

References

(Depends on 1 open bug)

Details

(4 keywords)

Attachments

(1 file, 2 obsolete files)

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)
Whiteboard: DUPEME
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?
Flags: blocking-firefox3?
Keywords: regression
OS: Mac OS X → All
Hardware: PC → All
Whiteboard: DUPEME
Marco/Mano: what's involved in fixing this? We'd basically need to erase the tag container, right?
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
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.
if you think this is a needed polish i will take a look
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.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
(is this actually a regression?)
Keywords: polish, ue
(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

Blocks: 419731
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?
Tags with no bookmarks also can't be deleted.
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.
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?

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.
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.
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.
Duplicate of this bug: 438984
Duplicate of this bug: 439996
Duplicate of this bug: 440559
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).
Could this at least be fixed for firefox3.1?
Flags: blocking-firefox3.1?
Priority: -- → P2
mano, do you have cycles for this for 3.1?
Target Milestone: --- → Firefox 3.1a1
Target Milestone: Firefox 3.1a1 → Firefox 3.1a2
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.
Please see bug 448584 which is caused at least by this bug and results in dataloss.
For a testcase see my example sqlite database (attachment 331784 [details])
No longer blocks: 448584
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?
(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.)
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.
Target Milestone: Firefox 3.1a2 → Firefox 3.1b1
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #341279 - Flags: review?(mano)
Depends on: 458019, 431558
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.
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
Attachment #341279 - Flags: review?(mano)
Attached patch patchSplinter Review
Attachment #341279 - Attachment is obsolete: true
Attachment #342888 - Flags: review?(mano)
after asking permission to Mano in IRC, reassigning to me
Assignee: mano → mak77
Status: NEW → ASSIGNED
Target Milestone: Firefox 3.1b1 → Firefox 3.1b2
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.
Attachment #342888 - Flags: review?(mano) → review-
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.
Attachment #342888 - Flags: review- → review?(mano)
Approving blocking 3.1 for behavioral consistency of UI, and because it'd provide much utility to users of tags.
Flags: blocking-firefox3.1? → blocking-firefox3.1+
http://hg.mozilla.org/mozilla-central/rev/4ffb7e96343f

notice this will not delete bogus tag containers (those will be fixed apart)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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?
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Flags: in-litmus?
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.
https://litmus.mozilla.org/show_test.cgi?id=7062 added to Litmus, under the Library FFT set.
Flags: in-litmus? → in-litmus+
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
mh, that should not be complex to fix, thank you for noticing that
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.
"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.
Attached patch tag queries patch (obsolete) — Splinter Review
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.
Attachment #343707 - Flags: review?(mano)
Comment on attachment 343707 [details] [diff] [review]
tag queries patch

moving patch to a new bug
Attachment #343707 - Attachment is obsolete: true
Attachment #343707 - Flags: review?(mano)
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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 460830
i filed bug 460830
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
Status: RESOLVED → VERIFIED
Summary: Unable to delete tags from column list → Unable to delete tags in left column list
Duplicate of this bug: 461728
Duplicate of this bug: 459243
Duplicate of this bug: 437338
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.
Status: VERIFIED → REOPENED
Keywords: fixed1.9.1
Resolution: FIXED → ---
could you file a new mac bug about the button then please?
Sure. It's covered by bug 469540 now.

Closing this bug again and marking it verified.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: verified1.9.1
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.