The default bug view has changed. See this FAQ.

All tags removed when moving a bookmark in the Bookmarks folder structure (cut & paste)

VERIFIED FIXED in Firefox 3

Status

()

Firefox
Bookmarks & History
P2
major
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: Cyriel van 't End, Assigned: dietrich)

Tracking

({dataloss})

Trunk
Firefox 3
dataloss
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008020204 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008020204 Minefield/3.0b3pre

All tags associated with a bookmark are removed when moving an item from the root of the Bookmarks Menu folder to another folder.

Reproducible: Always

Steps to Reproduce:
1. Create a bookmark in the Bookmark Menu folder
2. Add some tags to the bookmark
3. Create another folder within the Bookmark Menu folder
4. Move the bookmark to the folder which was just created.
Actual Results:  
Bookmark is moved without the given tags

Expected Results:  
Bookmark is moved with the given tags

Experienced this issue when moving items using Cut and Paste. Not sure if this happens in other situations (Drag and drop doesn't seem to work in this build aswell)
(Reporter)

Updated

9 years ago
Version: unspecified → Trunk
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008020223 Minefield/3.0b3pre

At the moment he only way to drag a bookmark into a folder is in the sidebar BTW. No problem migrating the tag.
The problem occurs only while trying to cut & paste. 
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: All tags removed when moving a bookmark in the Bookmarks folder structure → All tags removed when moving a bookmark in the Bookmarks folder structure (cut & paste)

Updated

9 years ago
Duplicate of this bug: 419499

Updated

9 years ago
Severity: normal → major
Keywords: dataloss
OS: Windows XP → All
(Assignee)

Updated

9 years ago
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2

Updated

9 years ago
Assignee: nobody → dietrich
(Assignee)

Updated

9 years ago
Target Milestone: --- → Firefox 3
(Assignee)

Updated

9 years ago
Depends on: 384370
(Assignee)

Updated

9 years ago
Whiteboard: [swag: 1d]
(Assignee)

Updated

9 years ago
Whiteboard: [swag: 1d] → [swag: .5d]

Updated

9 years ago
Duplicate of this bug: 421697
(Assignee)

Updated

9 years ago
Hardware: PC → All
(Assignee)

Comment 4

9 years ago
Created attachment 310080 [details] [diff] [review]
fix

de/serializes tags as a property of the bookmark. this has a few benefits:

1. complete partial serializations: tags are carried around w/ the item they describe.
2. backup size: currently we serialize the tag root, where a uri is serialized once for for each tag it has. this means that a bookmark with 3 tags is saved in the backup as 4 bookmarks, inflating the size of the backup, and slowing down import and export.
3. restore perf: instead of tagging the URI once for each tag, we can now make one call to the tag service for all a uri's tags. maybe a few less db queries, but certainly less xpconnect traversal.

notes:

1. adds exclude filtering for json export, which could be useful to extension consumers using these methods for sync, etc.

2. older backups since json landed will still work, since we still handle import of the tag root, if present.
Attachment #310080 - Flags: review?(mano)
(Assignee)

Updated

9 years ago
Whiteboard: [swag: .5d] → [has patch][needs review mano]
(Assignee)

Comment 5

9 years ago
Comment on attachment 310080 [details] [diff] [review]
fix

not ready, need to adjust index values
Attachment #310080 - Flags: review?(mano)
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review mano]
(Assignee)

Comment 6

9 years ago
Created attachment 310114 [details] [diff] [review]
fix
Attachment #310080 - Attachment is obsolete: true
Attachment #310114 - Flags: review?(mano)
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review mano]
(Assignee)

Updated

9 years ago
Blocks: 417228
Comment on attachment 310114 [details] [diff] [review]
fix

Dietrich, doesn't this cause us to remove tags as you *redo* item removal? That can be fixed by only setting the tags property if you're removing the last bookmark for the bookmark's url.
Attachment #310114 - Flags: review?(mano) → review-
Status: NEW → ASSIGNED
(Assignee)

Comment 8

9 years ago
(In reply to comment #7)
> (From update of attachment 310114 [details] [diff] [review])
> Dietrich, doesn't this cause us to remove tags as you *redo* item removal? That
> can be fixed by only setting the tags property if you're removing the last
> bookmark for the bookmark's url.
> 

No it doesn't cause tags to be removed when doing redo item removal. The child transactions (adding the tags) are executed yet again.
(Assignee)

Updated

9 years ago
Attachment #310114 - Flags: review- → review?(mano)
I still don't think it's right. We should export the tags-root *as-is* and don't re-tag after restoring from backup.
(Assignee)

Comment 10

9 years ago
Per irc, i looked at only serializing tags inline for cut. However, it suffers from the same problem: Annotations on tag items will be lost. Our current approach towards tags in the transaction layer has the same problem. Basically any tag removal via the tagging API incurs the loss of annotations on the "tag bookmarks".

I think we need to document that the tag root is "special internal use", and can be modified out-of-band, so extensions should not use it directly. Use the tagging API instead. And in 3.1 we can fix bug 424160, moving tags out of the bookmark tree.
Dietrich, this isn't just annotations, this is alsp a speed issue and may lead to different Tags Root in certain cases, I'm sure. I would really like not to change our current, pretty cheap, solution with something that heavy.
(Assignee)

Comment 12

9 years ago
(In reply to comment #11)
> Dietrich, this isn't just annotations, this is alsp a speed issue and may lead
> to different Tags Root in certain cases, I'm sure. I would really like not to
> change our current, pretty cheap, solution with something that heavy.
> 

I'm ok with backup/restore the whole root for now. My point is that regardless of how we fix this bug, we still need to discourage extension developers from annotating tag bookmarks, due to the dataloss problem.

How could this lead to a different tags root?
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review mano]
(Assignee)

Updated

9 years ago
Flags: in-litmus?
(Assignee)

Comment 13

9 years ago
Created attachment 311851 [details] [diff] [review]
fix v3

- always backup tag root
- always restore tag root if present
- only inline tags for copy/drag
- always restore inline tags if present
Attachment #310114 - Attachment is obsolete: true
Attachment #311851 - Flags: review?(mano)
Attachment #310114 - Flags: review?(mano)
I would rename aInlineTags to something more generic, there are few more places i would use that info. Maybe aIsUICommand?
+      var tags = aData.tags.split(", ");
+      childTxns.push(this.ptm.tagURI(itemURL, tags));

We should filter the incoming tags list here, otherwise undoing this transaction will remove old tags as well.
(Assignee)

Comment 16

9 years ago
Created attachment 311864 [details] [diff] [review]
fix v4

addresses comments 14 and 15.
Attachment #311851 - Attachment is obsolete: true
Attachment #311864 - Flags: review?(mano)
Attachment #311851 - Flags: review?(mano)
I don't understand how's that addressing comment 15.

basically, here's my scenraio:
1. have two bookmarks a, b for the same uri, set with some tags
2. copy&paste b
3. undo paste b

Are the tags for a still set?
(Assignee)

Comment 18

9 years ago
Created attachment 311911 [details] [diff] [review]
fix v5

sorry, my misunderstanding. this adds filtering to properly handle that case.
Attachment #311864 - Attachment is obsolete: true
Attachment #311911 - Flags: review?(mano)
Attachment #311864 - Flags: review?(mano)
Comment on attachment 311911 [details] [diff] [review]
fix v5

r=mano
Attachment #311911 - Flags: review?(mano) → review+
(Assignee)

Comment 20

9 years ago
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.128; previous revision: 1.127
done
Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.8; previous revision: 1.7
done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Verified with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/20080429006
Minefield/3.0pre
Status: RESOLVED → VERIFIED
Test case https://litmus.mozilla.org/show_test.cgi?id=7478 has been created in litmus for regression testing.
Flags: in-litmus? → in-litmus+
(In reply to comment #22)
> Test case https://litmus.mozilla.org/show_test.cgi?id=7478 has been created in
> litmus for regression testing.

Aakash, could you please also add it to the 3.0 testrun, since it has been fixed for Firefox 3? It would be great.
Sure, created test case https://litmus.mozilla.org/show_test.cgi?id=7479 for rel3.0 regression testing
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

Comment 26

6 years ago
Bug present in FF6 !
(Reporter)

Comment 27

6 years ago
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0

This issue indeed seems to have been regressed into the latest release version of Firefox 6.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
another bug has already been filed on the new issue, correctly, this should never ever be reopened.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.