Closed Bug 417228 Opened 13 years ago Closed 13 years ago

Bookmarks outside the three default folders aren't restored

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: teoli, Assigned: dietrich)

References

Details

(Keywords: dataloss)

Attachments

(1 file, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3

Bookmarks that are stored outside the "Bookmarks Toolbar", "Bookmarks Menu" and "Unfiled Bookmarks" aren't backuped, leading to dataloss (example: http://forums.mozillazine.org/viewtopic.php?t=627986 )


Reproducible: Always

Steps to Reproduce:
1.Create a bookmark called "XXXXXXXXXXXXXXX"
2.Move it, via the Library, at the top level (i.e. outside the toolbar, the menu, the unfiled bookmarks)
3.Do "Import and Backup" -> Backup
Actual Results:  
No "XXXXXXXXXXXXXXX" bookmark in the generated file

Expected Results:  
Either the bookmarks in the generated file, either the inability to put a bookmark there.

This may lead to dataloss if another problem happen. It already happened to some in beta2 -> beta3.

It is similar, though not identical (and thus not a dup) of bug 405936. A common fix may be possible.
Oh I forgot: people using the bookmarks sidebar are very likely to have lots of bookmarks at this position.
Here you go: Bug 404658 should take a decision on that

why putting items there at all if you already have unfiled bookmarks to put them in?

the backup problem will be solved by new JSON backup (Bug 384370), actually bookmarks saved our of the three main folders are not saved 
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
(In reply to comment #2)
> why putting items there at all if you already have unfiled bookmarks to put
> them in?
> 

I think unfiled isn't the best place for a Feeds folder, wich I lost with the update to beta3
Flags: blocking-firefox3?
Version: unspecified → Trunk
I think I stated in bug 404658 that if allowing top-level folders causes problems like this, then we shouldn't do it. Setting dependency, but I agree that this blocks, since we're creating an expectation that backup is for all bookmarks. 

Either prevent these bookmarks to be created or be sure to back them up.
Depends on: 404658
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Duplicate of this bug: 411616
I have problems with the backing up of 'unfiled bookmarks'
When I make a backup, that folder isn't included.

In the profile folder, the .html files of the bookmarks however have that folder

It is maybe not the same, but maybe it has to do with the same problem?
(In reply to comment #6)
> I have problems with the backing up of 'unfiled bookmarks'
> When I make a backup, that folder isn't included.

Related to bug 402302?
(In reply to comment #7)
> (In reply to comment #6)
> > I have problems with the backing up of 'unfiled bookmarks'
> > When I make a backup, that folder isn't included.
> 
> Related to bug 402302?
> 

Yes, but not on XP, on a mac Leopard and Windows Vista
I have also made a new places.sqlite after some problems...
Summary: Bookmarks outside the three default folders aren't backuped. → Bookmarks outside the three default folders aren't backed up
Depends on: 384370
Assignee: nobody → dietrich
Priority: P3 → P2
Target Milestone: --- → Firefox 3 beta5
Depends on: 415389
Bug 384370 ensures that all bookmarks are backed up to JSON backups, however it doesn't restore anything outside the tags, menu, toolbar, unfiled folders. Morphing summary to reflect this.
Summary: Bookmarks outside the three default folders aren't backed up → Bookmarks outside the three default folders aren't restored
Target Milestone: Firefox 3 beta5 → Firefox 3
Whiteboard: [needs status update]
Attached patch wip (obsolete) — Splinter Review
Whiteboard: [needs status update] → [has wip patch]
Blocks: 426081
Attached patch more (obsolete) — Splinter Review
- fix copy-last-backup-to-backups-dir
- make allBookmarks and leftPane getters not memo-ized
- tmp fixes for testability

need to figure out what's going when a root node of a query is deleted out from under it. right now, this patch works fine, other than leaving db cruft: it is re-creating new left pane hierarchies when a restore occurs while the organizer is open.
Attachment #313287 - Attachment is obsolete: true
Attached patch fix (obsolete) — Splinter Review
- de-memoize leftPane and allBookmarks, to allow for the id to change when restoring. (accessed very rarely, so not worried too much about this)

- add observer to places.js to reset the left pane when it's deleted/re-created
Attachment #314367 - Attachment is obsolete: true
Attachment #315631 - Flags: review?(mano)
Whiteboard: [has wip patch] → [has patch][needs review mano]
Comment on attachment 315631 [details] [diff] [review]
fix

marco, can you do first review here please?
Attachment #315631 - Flags: review?(mano) → review?(mak77)
Comment on attachment 315631 [details] [diff] [review]
fix

the fix in bitrotted due to the leftpane versioning patch... so r- for that, however here is a code review, will do a functionality test with the next version.

+  _resetLeftPane: function() {
+    this._initFolderTree();
+    this.selectLeftPaneQuery("AllBookmarks");
+    this._oldLeftPaneId = PlacesUIUtils.leftPaneFolderId;
+  },

is not this._initFolderTree(); already setting this._oldLeftPaneId?

+    if (folder == PlacesUtils.placesRootId
+        && id == this._oldLeftPaneId) {

nit: IIRC we put && at the end on the first line as +

+    if (this._leftPaneFolderId == -1 ||
+        PlacesUtils.bookmarks.getItemIndex(this._leftPaneFolderId) == -1) {
+      var leftPaneRoot = -1;
+      var allBookmarksId;


allBookmarksId can be directly defined in the batch callback since it is only used there

-  get allBookmarksFolderId() {
-    // ensure the left-pane root is initialized;
-    this.leftPaneFolderId;
-    delete this.allBookmarksFolderId;
-    return this.allBookmarksFolderId = this.leftPaneQueries["AllBookmarks"];
+  

nit: trailing space

+          for (var i = 0; i < root.childCount; i++) {
+            var aRootItemId = root.getChild(i).itemId;

why the prepended a? is not that commonly used to decorate passed-in vars? could be confusing

+              }
+            }
+            else if ([this._utils.toolbarFolderId, this._utils.tagsFolderId,
+                      this._utils.unfiledBookmarksFolderId,
+                      this._utils.bookmarksMenuFolderId].indexOf(aRootItemId) != -1)

another check for this._utils.tagsFolderId? is not it already served by the previous if?

+            else if ([this._utils.toolbarFolderId, this._utils.tagsFolderId,
+                      this._utils.unfiledBookmarksFolderId,
+                      this._utils.bookmarksMenuFolderId].indexOf(aRootItemId) != -1)
+              this._utils.bookmarks.removeFolderChildren(aRootItemId);
+            else
+              this._utils.bookmarks.removeItem(aRootItemId);
+          }
+          root.containerOpen = false;
+        }
+      

nit: trailing space

-                var uris = [];
-                tags.forEach(function(aTag) {
-                  var tagURIs = this.tagging.getURIsForTag(aTag);
-                  for (let i in tagURIs)
-                    this.tagging.untagURI(tagURIs[i], [aTag]);
-                }, this);
+        

nit: trailing space

-                tags.forEach(function(aTag) {
-                  var tagURIs = this.tagging.getURIsForTag(aTag);
-                  for (let i in tagURIs)
-                    this.tagging.untagURI(tagURIs[i], [aTag]);
-                }, this);
+        

nit: trailing space

+                break;
+              case "toolbarFolder":
+                container = this.toolbarFolderId;
+                break;
             }
+  

nit: trailing space

+    // create a test root
+    this._folderTitle = "test folder";
+    this._folderId = 

test "root"?

+  // export json to file
+  try {
+    PlacesUtils.backupBookmarksToFile(jsonFile);
+  } catch(ex) { do_throw("couldn't export to file: " + ex); }
+
+  // restore json file
+  try {
+    PlacesUtils.restoreBookmarksFromJSONFile(jsonFile);
+  } catch(ex) { do_throw("couldn't import the exported file: " + ex); }

what about adding some "litter" in the middle, like create a new folder in the root, add some tag... to ensure that they are correctly removed on restore
Attachment #315631 - Flags: review?(mak77) → review-
Whiteboard: [has patch][needs review mano] → [has wip patch]
Attached patch fixes comments (obsolete) — Splinter Review
note: this also changes the endupdatebatch assertion to a warning, since it can occur in valid scenarios, such as when a new observer is added *during* a batch operation.
Attachment #315631 - Attachment is obsolete: true
Attachment #319446 - Flags: review?(mano)
Whiteboard: [has wip patch] → [has patch][needs review mano]
Comment on attachment 319446 [details] [diff] [review]
fixes comments

Discussed over #places.
Attachment #319446 - Flags: review?(mano) → review-
Whiteboard: [has patch][needs review mano] → [has patch]
Attached patch wip (obsolete) — Splinter Review
alternate approach:

- allow exclusions at backup: for example, the left pane folder shouldn't be exported, if it's not going to be replaced during restore. otherwise every restore would add another invisible left pane folder hierarchy.

- allow exclusions at restore: for example, when doing a replace-and-restore, don't delete the left pane folder

this patch is mostly there, just need to write a separate test for exclusions at backup time.
Attachment #319446 - Attachment is obsolete: true
Attached patch with testSplinter Review
Attachment #319504 - Attachment is obsolete: true
Attachment #319520 - Flags: review?(mano)
Whiteboard: [has patch] → [has patch][needs review mano]
Comment on attachment 319520 [details] [diff] [review]
with test

a+ schrep
Attachment #319520 - Flags: approval1.9? → approval1.9+
Whiteboard: [has patch][needs review mano] → [needs landing
Whiteboard: [needs landing → [has patch][has review][has approval]
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has approval]
Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9) Gecko/2008052903 Firefox/3.0

tested on MAC - RC2 works OK

Status: RESOLVED → VERIFIED
Depends on: 450938
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.