Provide backup and restore functionality for Places bookmarks

VERIFIED FIXED in Firefox 3 alpha8

Status

()

defect
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: dietrich, Assigned: hello)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PLCS-003c, )

Attachments

(5 attachments, 6 obsolete attachments)

Reporter

Description

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

Updated

12 years ago
Blocks: 374945

Updated

12 years ago
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 alpha5
Flags: blocking-firefox3? → blocking-firefox3+
Whiteboard: PLCS-003c
Reporter

Comment 1

12 years ago
iirc, this item is about supporting import/export of bookmarks.html, marking fixed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
per Deitrich in irc: we backup bookmarks from places.sqlite to bookmarks.html at shutdown
Status: RESOLVED → VERIFIED
Reporter

Comment 3

12 years ago
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
Reporter

Updated

12 years ago
Assignee: nobody → dietrich
Status: REOPENED → NEW
Reporter

Comment 4

12 years ago
Posted patch wip (obsolete) — Splinter Review
adds a "restore" menu to the file menu in the organizer. import is brute-forced (delete all, then import).
Reporter

Comment 5

12 years ago
Posted image screenshot
Reporter

Comment 6

12 years ago
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
Assignee

Comment 7

12 years ago
(needed for next attachment)
Assignee

Comment 8

12 years ago
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.
Reporter

Comment 9

12 years ago
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
Reporter

Comment 10

12 years ago
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);
>+  },
>+
Assignee

Comment 11

12 years ago
Posted 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
Assignee

Comment 12

12 years ago
Posted 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)
Assignee

Comment 13

12 years ago
Posted image screenshot (v1)
Whiteboard: PLCS-003c → PLCS-003c [need review dietrich]
Reporter

Comment 14

12 years ago
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-
Reporter

Updated

12 years ago
Whiteboard: PLCS-003c [need review dietrich] → PLCS-003c
Assignee

Comment 15

12 years ago
(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.
Assignee

Comment 16

12 years ago
Posted 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)
Assignee

Updated

12 years ago
Attachment #280157 - Flags: ui-review?(faaborg)
Reporter

Comment 17

12 years ago
(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
Reporter

Comment 18

12 years ago
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+
Assignee

Comment 19

12 years ago
(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.
Assignee

Comment 20

12 years ago
Posted patch Backup / Restore v1.2 (obsolete) — Splinter Review
Attachment #280157 - Attachment is obsolete: true
Attachment #280157 - Flags: ui-review?(faaborg)
Assignee

Updated

12 years ago
Attachment #280184 - Flags: ui-review?(faaborg)
Assignee

Comment 21

12 years ago
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)
Reporter

Comment 22

12 years ago
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+
Reporter

Comment 24

12 years ago
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: 12 years ago12 years ago
Resolution: --- → FIXED

Comment 25

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

Comment 26

12 years ago
Posted 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.