374528 - Provide backup and restore functionality for Places bookmarks

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

This is a tracking bug for item PLCS-003c in the Firefox 3 PRD.

Comment 1

[Dietrich Ayala (:dietrich)]

iirc, this item is about supporting import/export of bookmarks.html, marking fixed.

Comment 2

[Tracy Walker [:tracy]]

per Deitrich in irc: we backup bookmarks from places.sqlite to bookmarks.html at shutdown

Comment 3

[Dietrich Ayala (:dietrich)]

mconnor noted that this bug was originally about adding user-friendly restore functionality to the organizer, and that bug 374528 is the "continue exporting bookmarks.html" bug.

i'm going to:

- reopen this bug
- add a dependency on bug 384370
- resolve bug 374528, remove it's dependency on bug 384370

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: wip

adds a "restore" menu to the file menu in the organizer. import is brute-forced (delete all, then import).

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: screenshot

Comment 6

[Dietrich Ayala (:dietrich)]

mconnor, this patch is a first-shot at the "easy restore" that we talked about yesterday. this implements it as a menu item, the workflow being:

1. notice your bookmarks are horked in some way
2. find out about the restore feature somehow
3. open the menu, select the most recent date
4. read the warnings in the popup dialog and click OK (not in this patch)
5. wait while your bookmarks are restored
6. if the bookmarks are still not correct, start again at step 3, select a different date

a wizard might be a better approach than having the backup list in the menu itself, in that we'd be able to better communicate to the user about what's going on before they actually select a backup.

Comment 7

[Dan Mills [:thunder]]

Attachment: Add IDs to places organizer menupopups

(needed for next attachment)

Comment 8

[Dan Mills [:thunder]]

Attachment: Backup / revert extension for the places organizer

This is a different approach, more like the one OS X's Address Book takes:

- back up menu item brings up a 'save' dialog, with a prepopulated filename (with a date).
- revert menu item opens a file picker to the default save directory.

It's an extension to make it easier to try out, etc.  Maybe we should combine this with your 'menu of dates' approach, something like:

- backup defaults to Documents folder or the like
- revert is a menu with:
  2007-08-07
  2007-08-10
  Choose file...
Where 'Choose file' would bring up the file picker, defaulting to the same folder as the backup menuitem.

Comment 9

[Dietrich Ayala (:dietrich)]

mconnor:

so, wizard vs. menu

a) I'd like to be able to do a full backup and restore manually, not just restore the automated bookmarks

b) I'd like to make it easy to restore from an online backup service

so wizard is probably a little better, since that also gets you the progress/interstitial bit for free

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 276020 [details] [diff] [review]
wip


>+    // delete all bookmarks
>+    PlacesUtils.bookmarks.removeFolderChildren(PlacesUtils.bookmarks.bookmarksRoot);

this needs to be removed, and instead just flip initialImport param to true below.

>+
>+    // import from file
>+    var importer = Cc["@mozilla.org/browser/places/import-export-service;1"].
>+                   getService(Ci.nsIPlacesImportExportService);
>+    importer.importHTMLFromFile(aFile, false);
>+  },
>+

Comment 11

[Dan Mills [:thunder]]

Attachment: Backup / Restore (wip)

New version of Dietrich's first "wip" patch.
Haven't worked on it much, just added a persistent 'Choose File...' menu item at the end of the restore menu (not hooked up to a dialog yet).
Also fixed what Dietrich said in comment #10.

Comment 12

[Dan Mills [:thunder]]

Attachment: Backup / Restore v1

Adds a 'Backup...' menu item which prompts for where to save, and makes the 'Choose File...' menuitem from the last patch work.
Adds a confirmation dialog before reverting.

The only thing I don't like about this, is that it's not clear where the backups in the Restore menu come from / why backing up manually doesn't add to that list.  Other than that, it's pretty darned convenient ;)

Comment 13

[Dan Mills [:thunder]]

Attachment: screenshot (v1)

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 279845 [details] [diff] [review]
Backup / Restore v1

>diff -r 57773ee2f902 browser/components/places/content/places.js
>--- a/browser/components/places/content/places.js	Wed Sep 05 17:14:14 2007 -0700
>+++ b/browser/components/places/content/places.js	Wed Sep 05 19:41:53 2007 -0700

>+    // get list of files
>+    var fileList = [];
>+    var files = bookmarksBackupDir.directoryEntries;
>+    while (files.hasMoreElements()) {
>+      var f = files.getNext().QueryInterface(Ci.nsIFile);
>+      if (!f.isHidden())
>+        fileList.push(f);
>+    }

should we sort this newest to oldest? restoring a more recent backup seems a
more likely scenario than restoring an older one (in my very unscientific opinion).

>+
>+    if (fileList.length == 0) {
>+      restorePopup.parentNode.setAttribute("disabled", true);
>+      return;
>+    }

shouldn't the menu still be enabled so that users can select a file to restore from?

>+
>+    // enable menu
>+    restorePopup.parentNode.removeAttribute("disabled");
>+
>+    // populate menu
>+    for (var i = 0; i < fileList.length; i++) {
>+      var m = restorePopup.insertBefore(document.createElement("menuitem"),
>+                                        document.getElementById("restoreFromFile"));
>+      var dateStr = fileList[i].leafName.replace("bookmarks-", "").replace(".html", "");

should we show any non-.html or .htm files?

should check that post-filtered-and-renamed dateStr length is > 0 before adding?

nit: 80 char line length

>+  /**
>+   * Backup bookmarks to desktop, auto-generate a filename with a date
>+   */
>+  backupBookmarks: function PO_backupBookmarks() {
>+    var fp = Cc["@mozilla.org/filepicker;1"].createInstance(Ci.nsIFilePicker);
>+    fp.init(window, PlacesUtils.getString("bookmarksBackupTitle"),
>+            Ci.nsIFilePicker.modeSave);
>+    fp.appendFilters(Ci.nsIFilePicker.filterHTML);
>+  
>+    var dirSvc = Cc["@mozilla.org/file/directory_service;1"].
>+      getService(Ci.nsIProperties);
>+    var backupsDir = dirSvc.get("Desk", Ci.nsILocalFile);
>+    fp.displayDirectory = backupsDir;
>+  
>+    function twoDigits(num) {
>+      if (num < 10)
>+        return "0" + num;
>+      return num;
>+    }
>+    var d = new Date();
>+    var date = d.getFullYear() + "-" +
>+      twoDigits(d.getMonth()) + "-" + twoDigits(d.getDate());

should use the scriptable date formatter here?

>diff -r 57773ee2f902 browser/locales/en-US/chrome/browser/places/places.properties
>--- a/browser/locales/en-US/chrome/browser/places/places.properties	Wed Sep 05 17:14:14 2007 -0700
>+++ b/browser/locales/en-US/chrome/browser/places/places.properties	Wed Sep 05 19:41:53 2007 -0700
>@@ -14,16 +14,23 @@ noTitle=(no title)

>+
>+bookmarksRestoreAlertTitle=Revert Bookmarks
>+bookmarksRestoreAlert=This will replace all of your current bookmarks with the backup.  Are you sure?

is the extra space intentional?

can you request ui-r? from ux for these strings? (don't block landing for this though)

note: this may soon be rotted by bug 387740 (or the other way around).

Comment 15

[Dan Mills [:thunder]]

(In reply to comment #14)

> should we show any non-.html or .htm files?

I think we shouldn't, for now.  When we add support for json files, we should show those.

> should check that post-filtered-and-renamed dateStr length is > 0 before
> adding?

I fall back to the full filename now.  Is that better?

> should use the scriptable date formatter here?

Oy.  I was looking for one, but didn't find it.  Can you link me please? :)

> >+bookmarksRestoreAlert=This will replace all of your current bookmarks with the backup.  Are you sure?
> 
> is the extra space intentional?

Sorry; I have two-spaces-after-period ingrained in me.  Fixed.

Comment 16

[Dan Mills [:thunder]]

Attachment: Backup / Restore v1.1

Comments fixed, except as noted in the last comment.

Comment 17

[Dietrich Ayala (:dietrich)]

(In reply to comment #15)
> (In reply to comment #14)
> 
> > should we show any non-.html or .htm files?
> 
> I think we shouldn't, for now.  When we add support for json files, we should
> show those.
> 

ok, this patch does not currently filter out non-.htm(l) files when populating the menu. can you add that?

> 
> > should use the scriptable date formatter here?
> 
> Oy.  I was looking for one, but didn't find it.  Can you link me please? :)

http://mxr.mozilla.org/seamonkey/source/intl/locale/idl/nsIScriptableDateFormat.idl

http://mxr.mozilla.org/seamonkey/search?string=scriptabledate

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 280157 [details] [diff] [review]
Backup / Restore v1.1

please file followups on the 2 remaining issues.

Comment 19

[Dan Mills [:thunder]]

(In reply to comment #17)

> ok, this patch does not currently filter out non-.htm(l) files when populating
> the menu. can you add that?

Hmm? It does:

fp.appendFilters(Ci.nsIFilePicker.filterHTML);


> > > should use the scriptable date formatter here?
> > 
> > Oy.  I was looking for one, but didn't find it.  Can you link me please? :)
> 
> http://mxr.mozilla.org/seamonkey/source/intl/locale/idl/nsIScriptableDateFormat.idl
> 
> http://mxr.mozilla.org/seamonkey/search?string=scriptabledate

Thanks, patch coming up.  Unfortunately, scriptableDateFormat doesn't seem to be very flexible, so I can't get the same output (year first, two digits for months and days, dash-separated).  So I'm using dateFormatShort, eg '1/2/07'.

Seems to work on fine OSX, not sure about saving filenames with slashes on other platforms.

Comment 20

[Dan Mills [:thunder]]

Attachment: Backup / Restore v1.2

Comment 21

[Dan Mills [:thunder]]

Attachment: Backup / Restore v1.3

Misunderstood the filtering comment.  This version filters non-html files in the generated menu.

Comment 22

[Dietrich Ayala (:dietrich)]

Comment on attachment 280185 [details] [diff] [review]
Backup / Restore v1.3

requesting 1.9 approval: UI change providing user-driven backup and restore of bookmarks.

Comment 23

[Mike Connor [:mconnor]]

Comment on attachment 280185 [details] [diff] [review]
Backup / Restore v1.3

pseudo-ui-r=me, lets get this in and tweak later

a=me, let's get this into M8

Comment 24

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/content/places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.100; previous revision: 1.99
done
Checking in browser/components/places/content/places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v  <--  places.xul
new revision: 1.81; previous revision: 1.80
done
Checking in browser/locales/en-US/chrome/browser/places/places.dtd;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.dtd,v  <--  places.dtd
new revision: 1.29; previous revision: 1.28
done
Checking in browser/locales/en-US/chrome/browser/places/places.properties;
/cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.properties,v  <--  places.properties
new revision: 1.27; previous revision: 1.26
done

Comment 25

[Scott A.]

Nice, another bug marked off of Places PRD :)  Been waiting for this for this one for some time.

Comment 26

[pal-moz]

Attachment: screenshot

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090918 Minefield/3.0a8pre ID:2007090918

File>Backup,
default backupfilename (date) is wrong.
bug or only for me?

Comment 27

[Peter van der Woude [:Peter6]]

(In reply to comment #26)
> Created an attachment (id=280303) [details]
> screenshot
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090918
> Minefield/3.0a8pre ID:2007090918
> 
> File>Backup,
> default backupfilename (date) is wrong.
> bug or only for me?
> 
-> Bug 395633

Comment 28

[Tracy Walker [:tracy]]

verified with - Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007091304 Minefield/3.0a8pre

adding in-litmus?  we need a full set of test cases around this area.

Comment 29

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

The mockup in bug #393514 shows the new menu structure.  The "revert to backup" menuitem should display a dialog, instead of the current sub-menu.  I can mock this up for you if you want, or just review what you come up with.

Comment 30

[Aakash Desai [:aakashd]]

There's been a whole suite of test cases added to the Bookmarks FFT in the 3.x test runs.


For 3.0,
https://litmus.mozilla.org/show_test.cgi?id=6092
https://litmus.mozilla.org/show_test.cgi?id=5943
https://litmus.mozilla.org/show_test.cgi?id=7452
https://litmus.mozilla.org/show_test.cgi?id=6852
https://litmus.mozilla.org/show_test.cgi?id=6853
https://litmus.mozilla.org/show_test.cgi?id=6854
https://litmus.mozilla.org/show_test.cgi?id=6859
https://litmus.mozilla.org/show_test.cgi?id=6860
https://litmus.mozilla.org/show_test.cgi?id=6933
https://litmus.mozilla.org/show_test.cgi?id=6905


For 3.1,
https://litmus.mozilla.org/show_test.cgi?id=4134
https://litmus.mozilla.org/show_test.cgi?id=3985
https://litmus.mozilla.org/show_test.cgi?id=5234
https://litmus.mozilla.org/show_test.cgi?id=5235
https://litmus.mozilla.org/show_test.cgi?id=5236
https://litmus.mozilla.org/show_test.cgi?id=5242
https://litmus.mozilla.org/show_test.cgi?id=5243
https://litmus.mozilla.org/show_test.cgi?id=5291
https://litmus.mozilla.org/show_test.cgi?id=5320
https://litmus.mozilla.org/show_test.cgi?id=5321

Comment 31

[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