429583 - Add option to restore JSON Bookmarks backup file without overwriting current bookmarks

Status: RESOLVED DUPLICATE of bug 271437

(Firefox :: Bookmarks & History, enhancement)

Description

[Louis]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041707 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041707 Minefield/3.0pre

As of right now I don't see any option to restore a .json bookmarks backup file without overwriting your current bookmarks. I realize that you can export as HTML, then import HTML, but this will not always be possible in some situations. Say you create a bookmarks backup, then your computer crashes and you have to reinstall windows. In a perfect world, you would restore your bookmarks before creating any others, but this will not always be the case. And since your old installation of Firefox is gone, you can not go back and create an HTML file, so all your left with is a .json file with no way of restoring it without overwriting all of your bookmarks.

Reproducible: Always

Comment 1

[Matthew Turnbull [Bluefang]]

The Import/Restore/Export/Backup system dose seem overly complex.

Wouldn't it be simpler to just have 2 Wizards? One fore Import and one for Export.

Import Wizard
() JSON File
() HTML File
[] Overwrite current bookmarks

Export Wizard
() JSON File
() HTML File (for pre-Firefox3 compatibility)

That makes much more sense than having 2 separate systems, with seemingly similar functionality (confusing to users), and 2 separate file formats.

Comment 2

[Louis]

Yes, that is the same sort of direction I am thinking. It doesn't make sense to have to create a .json backup AND an .html backup just because your never sure how or when you going to want to restore. As you said, you should have the choice between the 2 and both formats should be able to Backup/Restore in same ways. You shouldn't have different functionality for the 2.

Right now, the only actual difference is that you can open an HTML bookmark backup and read it easily, which isn't possible for a JSON backup. It would also be nice if Firefox were able to parse the JSON backup file into a readable form in the Firefox window. This is for a future bug/release though and is off-topic from this bug.

Comment 3

[Ria Klaassen (not reading all bugmail)]

Well, the simplest to do is to make a copy your places.sqlite file every day. With the bookmark backup extension or with a simple batch file.

Comment 4

[Louis]

In response to Comment #3:

Yes, but can this be restored without wiping out any existing bookmarks??

Firefox already has a mechanism in place to backup and restore bookmarks, so just the fact that there would be a need to bypass that system and backup places.sqlite using other means just reinforces the fact that the bookmarks backup/restore system needs work IMO. If it's there, it should perform the function it is there for in most or all possible situations and shouldn't need to be bypassed to accomplish something as simple as backing up bookmarks.

Comment 5

[Ria Klaassen (not reading all bugmail)]

Well, normally I copy the bookmarks.html or places.sqlite before I even start a new profile or seconds after I created it, or even copy a whole profile to the new computer, but if I understand the problem correctly you first bookmark some sites in the new profile and then you import the bookmarks.
Maybe merging bookmarks would solve this?

Comment 6

[Dietrich Ayala (:dietrich)]

The API supports both merge and replace of both formats.

Regarding the design in comment #1 - I'd like to avoid pushing the formats in front of non-technical users, if possible. For example, maybe use "From Firefox" instead of "JSON", and "From another application" instead of HTML.

Comment 7

[Louis]

Yes, I agree that having just the formats listed is not desirable. However, it might be fine to mention the format for the more advanced users?? Perhaps something like:

Import Wizard
()From a Firefox backup (JSON)
()From another browser (HTML)
[]Overwrite existing bookmarks

Export Wizard
()Export for Firefox (JSON)
()Export for another browser (HTML)

On importing, if they choose "Overwrite existing bookmarks", it would probably be good to bring up an extra warning just in case. Such as: "Are you sure you want to overwrite all of your existing bookmarks? This can't be undone!"

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I recently accidentally deleted a few bookmarks but didn't realize until a few days later.

I can't do a full restore since I've added more bookmarks since, but would like to selectively restore the bookmarks I lost.

One idea I had was the ability to restore into a folder, rather than restoring by overwriting all my current bookmarks.

Comment 9

[bomfog]

Just as a work-around, you might consider weave <http://labs.mozilla.com/featured-projects/#weave>. It seems to do correct merging, but I haven't watched closely enough to say for certain.

Comment 10

[Kevin Qi]

Attachment: patch 1

Description of changes:

The "Import and Backup" menu is now pretty similar to what was described in comment #7.

[Import and Export]
    Restore Bookmarks
    Import Bookmarks..
    Export Bookmarks..

The "Backup.." option is gone because exporting to JSON is moved to "Export Bookmarks..".
    
"Restore Bookmarks" still brings up a list of dates to restore bookmarks from, but the "Choose File.." option is gone because the functionality is moved to "Import Bookmarks.."

"Import Bookmarks.." brings up the migration wizard, which now contains a "From JSON file" option. If JSON is selected, a checkbox labeled "Overwrite existing bookmarks" appears and is checked by default -- if you uncheck it, existing bookmarks won't be overwritten. This was done by simply adding a boolean argument to PlacesUtils.restoreBookmarksFromJSONFile.

"Export Bookmarks.." brings up a new wizard for exporting bookmarks to either HTML or JSON. Exporting to HTML now also appends the current date, like JSON does.

Let me know if these changes are appropriate. Thanks!

Comment 11

[Dietrich Ayala (:dietrich)]

some questions and comments before moving forward:

- does the merge duplicate folders or just add bookmarks to existing folders? i don't think multiple of each folder is ever really what the user wants.
- should we add an option to restore into a folder?
- i'd rather not put "JSON" in front of the user, as noted earlier. please use the term "restore", possibly with "(*.json files)" or something like that.
- please don't add a new dtd file, just pull in the existing one.
- this needs a new unit test confirming the merge behavior works as expected.

Comment 12

[Kevin Qi]

Attachment: patch 2

The merge doesn't duplicate the "Bookmarks Toolbar", "Bookmarks Menu", or "Unsorted Bookmarks" folders, just adds to them as expected, if that's what you're referring to. 

I added a unit test which I think it tests for this behavior, but I'm not entirely if I did it right. The rest of the issues in comment #10 should be fixed now.

Comment 13

[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

Comment 14

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

How does this relate to how weave is handling merges?

Comment 15

[Dietrich Ayala (:dietrich)]

Comment on attachment 414175 [details] [diff] [review]
patch 2

r- for a couple of code changes, but mostly just because i want more eyes on the UI/X for this, and especially how it'll fit into the upcoming theme changes.

>-<!ENTITY cmd.exportHTML.label           "Export HTMLâ¦">
>-<!ENTITY cmd.exportHTML.accesskey       "E">
>-<!ENTITY cmd.importHTML.label           "Import HTMLâ¦">
>-<!ENTITY cmd.importHTML.accesskey       "I">
>-
>-<!ENTITY cmd.backup.label               "Backupâ¦">
>-<!ENTITY cmd.backup.accesskey           "B">
>-<!ENTITY cmd.restore2.label             "Restore">
>-<!ENTITY cmd.restore2.accesskey         "R">
>-<!ENTITY cmd.restoreFromFile.label      "Choose Fileâ¦">
>-<!ENTITY cmd.restoreFromFile.accesskey  "C">
>+<!ENTITY cmd.exportBookmarks.label      "Export Bookmarksâ¦">
>+<!ENTITY cmd.exportBookmarks.accesskey  "E">
>+<!ENTITY cmd.importBookmarks.label      "Import Bookmarksâ¦">
>+<!ENTITY cmd.importBookmarks.accesskey  "I">
>+<!ENTITY cmd.restoreBookmarks.label     "Restore Bookmarks">
>+<!ENTITY cmd.restoreBookmarks.accesskey "R">

hrm, now you've introduced ambiguity about what the difference between "import" and "restore" is.

maybe all of this could be in the new wizard, consolidated under the main "Organize" menu... this leans slightly towards the kind of consolidation being considered for the Firefox 4 theme, with the Application button, etc.

Alex, input here would be appreciated!

> 
>-<!ENTITY maintenance.label      "Import and Backup">
>+<!ENTITY maintenance.label      "Import and Export">

hm, i'm ok with this change, but let's get UX input here.

> <!ENTITY maintenance.accesskey  "I">
> 
> <!ENTITY backCmd.label       "Back">
> <!ENTITY backButton.tooltip  "Go back">
> 
> <!ENTITY forwardCmd.label       "Forward">
> <!ENTITY forwardButton.tooltip  "Go forward">
> 
> <!ENTITY detailsPane.more.label "More">
> <!ENTITY detailsPane.more.accesskey "e">
> <!ENTITY detailsPane.less.label "Less">
> <!ENTITY detailsPane.less.accesskey "e">
> <!ENTITY detailsPane.noPreviewAvailable.label "Preview">
> <!ENTITY detailsPane.selectAnItemText.description "Select an item to view and edit its properties">
>+
>+<!ENTITY export.wizard.title                "Export Bookmarks Wizard">

s/Wizard//

>diff --git a/toolkit/components/places/src/utils.js b/toolkit/components/places/src/utils.js
>--- a/toolkit/components/places/src/utils.js
>+++ b/toolkit/components/places/src/utils.js
>@@ -1663,24 +1663,23 @@ var PlacesUtils = {
>    */
>   toJSONString: function PU_toJSONString(aObj) {
>     var JSON = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON);
>     return JSON.encode(aObj);
>   },
> 
>   /**
>    * Restores bookmarks and tags from a JSON file.
>-   * WARNING: This method *removes* any bookmarks in the collection before
>-   * restoring from the file.
>-   *
>    * @param aFile
>    *        nsIFile of bookmarks in JSON format to be restored.
>+   * @param aReplace
>+   *        Boolean if true, replace existing bookmarks, else merge.
>    */

set a default for aReplace and document it. see next comment.

>@@ -1696,17 +1695,17 @@ var PlacesUtils = {
>       var jsonStr = "";
>       while (converted.readString(8192, str) != 0)
>         jsonStr += str.value;
>       converted.close();
> 
>       if (jsonStr.length == 0)
>         return; // empty file
> 
>-      this.restoreBookmarksFromJSONString(jsonStr, true);
>+      this.restoreBookmarksFromJSONString(jsonStr, aReplace);

this changes the behavior expected by consumers. while you've fixed instances internally, extensions that might be using this will expect the old behavior. please make the default to restore, not merge, so we don't have this compatibility issue.

>+  // clean up
>+  jsonFile.remove(false);
>+}
>+ 
>\ No newline at end of file

add please

Comment 16

[Kevin Qi]

Attachment: patch 3

In response to comment #15:
I made aReplace default to true, added a comment about the default behavior, and fixed the other problems you mentioned. 

I agree that "import" and "restore" are sort of ambiguous. In the current version of Firefox, "import" means loading from HTML files and "restore" means loading from JSON files. In this patch, "import" means loading bookmarks from any kind of file while "restore" means restoring autosaved bookmark backups (which seems reasonable to me). Suggestions are welcome.

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 418487 [details] [diff] [review]
patch 3

Argh, this is been unlooked at forever, sorry Kevin. Requesting ui-r from Alex.

Comment 18

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

Comment on attachment 418487 [details] [diff] [review]
patch 3

Better than the current interface since it helps to avoid dataloss.  Overall I think a check box that says "replace current bookmarks" is more clear than the difference between import and restore though.