Closed Bug 374528 Opened 14 years ago Closed 14 years ago

Provide backup and restore functionality for Places bookmarks

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3 alpha8

People

(Reporter: dietrich, Assigned: hello)

References

()

Details

(Whiteboard: PLCS-003c)

Attachments

(5 files, 6 obsolete files)

This is a tracking bug for item PLCS-003c in the Firefox 3 PRD.
Blocks: 374945
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 alpha5
Flags: blocking-firefox3? → blocking-firefox3+
Whiteboard: PLCS-003c
iirc, this item is about supporting import/export of bookmarks.html, marking fixed.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
per Deitrich in irc: we backup bookmarks from places.sqlite to bookmarks.html at shutdown
Status: RESOLVED → VERIFIED
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
Status: VERIFIED → REOPENED
Depends on: 384370
Resolution: FIXED → ---
Target Milestone: Firefox 3 alpha5 → Firefox 3 M8
Assignee: nobody → dietrich
Status: REOPENED → NEW
Attached patch wip (obsolete) — Splinter Review
adds a "restore" menu to the file menu in the organizer. import is brute-forced (delete all, then import).
Attached image screenshot
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.
Status: NEW → ASSIGNED
(needed for next attachment)
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.
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 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);
>+  },
>+
Attached patch Backup / Restore (wip) (obsolete) — Splinter Review
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.
Assignee: dietrich → thunder
Attachment #276020 - Attachment is obsolete: true
Attachment #276242 - Attachment is obsolete: true
Attached patch Backup / Restore v1 (obsolete) — Splinter Review
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 ;)
Attachment #279710 - Attachment is obsolete: true
Attachment #279845 - Flags: review?(dietrich)
Attached image screenshot (v1)
Whiteboard: PLCS-003c → PLCS-003c [need review 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).
Attachment #279845 - Flags: review?(dietrich) → review-
Whiteboard: PLCS-003c [need review dietrich] → PLCS-003c
(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.
Attached patch Backup / Restore v1.1 (obsolete) — Splinter Review
Comments fixed, except as noted in the last comment.
Attachment #279845 - Attachment is obsolete: true
Attachment #280157 - Flags: review?(dietrich)
Attachment #280157 - Flags: ui-review?(faaborg)
(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 on attachment 280157 [details] [diff] [review]
Backup / Restore v1.1

please file followups on the 2 remaining issues.
Attachment #280157 - Flags: review?(dietrich) → review+
(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.
Attached patch Backup / Restore v1.2 (obsolete) — Splinter Review
Attachment #280157 - Attachment is obsolete: true
Attachment #280157 - Flags: ui-review?(faaborg)
Attachment #280184 - Flags: ui-review?(faaborg)
Misunderstood the filtering comment.  This version filters non-html files in the generated menu.
Attachment #280184 - Attachment is obsolete: true
Attachment #280185 - Flags: ui-review?(faaborg)
Attachment #280184 - Flags: ui-review?(faaborg)
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.
Attachment #280185 - Flags: approval1.9?
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
Attachment #280185 - Flags: ui-review?(faaborg)
Attachment #280185 - Flags: ui-review+
Attachment #280185 - Flags: approval1.9?
Attachment #280185 - Flags: approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Nice, another bug marked off of Places PRD :)  Been waiting for this for this one for some time.
Attached image 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?
(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

Depends on: 395633
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
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.
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.