421834 - Fix sorting and labels for the bookmark library restore menu

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Michael Kohler [:mkohler]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008030904 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008030904 Minefield/3.0b5pre

Always I open the bookmark library [CTRL]+[SHIFT]+[B] and go to "Import and Backup" -> "Restore..." afaik the sort isn't correct. Shouldn't it begin with the newest and go to the oldest date?

Reproducible: Always

Steps to Reproduce:
1. Open the bookmark library
2. Go to "Image and Backup"
3. Go to the "Restore..." - submenu
Actual Results:  
The sort doesn't begin with the newest date.

Expected Results:  
Begin with the newest and not with a older file.

I'm sorry if that is a feature instead of a bug! ;)

(As you see English isn't my native language, I'm sorry)

Comment 1

[Simon Bünzli]

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/components/places/content/places.js&rev=1.145&mark=366-368#357

Looks like sorting by leafName would yield a slightly more polished look for the Restore... menu, in case one of the backups was touched/modified out of order (though I don't really see how that might've happened).

Nice catch, Michael.

Comment 2

[Michael Kohler [:mkohler]]

Thanks, Mr. Bünzli.

(The current sort is now:

2008-03-11
2008-03-10
2008-03-07
2008-03-09
2008-03-06

PS: On 2008-03-08 I didn't open Minefield, if that information should be important ;)

Comment 3

[Michael Kohler [:mkohler]]

Working on a patch for this. I'll also change the behaviour to show also other files like .json~ (which are generated i.e. of gedit when editing a file).

Comment 4

[Marco Bonardo [:mak]]

so, assigning to you for now, thanks for looking into this.

Comment 5

[Michael Kohler [:mkohler]]

Attachment: Patch v0.1a

This patch fixes it, but I guess the code is not that good (it's my first patch).

So please feel free to give me a hint.

I tried to sort it with fileList.sort() (shouldn't that work because these are ISO 8601 date formats?) but it just sorted and then printed a for me undefinable sort of the list. Also changes the "a.lastModifiedTime" to "a.leafName" and for "b" the same thing didn't succeed.

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 362019 [details] [diff] [review]
Patch v0.1a

> I tried to sort it with fileList.sort() (shouldn't that work because these are
> ISO 8601 date formats?)

sort converts the array elements to strings if you don't supply a function.  (See https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/Array/sort)  For most XPConnect objects like the nsIFile instances here, calling toString on them will return something like "[xpconnect wrapped nsIFile @ 0x17b4a150 (native @ 0x17b49b60)]".  Not what we want.

> undefinable sort of the list. Also changes the "a.lastModifiedTime" to
> "a.leafName" and for "b" the same thing didn't succeed.

Hmm, I'm not sure how subtraction is defined on strings.  You could use < and > on the leafNames, though.  But a slightly better solution would be...

>     fileList.sort(function PO_fileList_compare(a, b) {
>-      return b.lastModifiedTime - a.lastModifiedTime;
>+      var rx = new RegExp("^(bookmarks|" + localizedFilenamePrefix + ")-");
>+      var dateA = a.leafName.replace(rx, "").replace(/\.json$/, "").replace(/-/g, "");
>+      var dateB = b.leafName.replace(rx, "").replace(/\.json$/, "").replace(/-/g, "");
>+      return dateB - dateA;
>     });

There's a string method called localeCompare you could use here to make your sort function easier to follow (in fact, one line).  (See https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Objects/String#Methods_2)  Try using it on the leafNames of b and a.

Comment 7

[Michael Kohler [:mkohler]]

Attachment: Patch v1.0

Thanks for help, Drew. I assume that's okay now and also that the RegExp change is welcome.

I'm not that experienced in programming in JS, so I bookmarked the developers.mozilla.org site about JS.

Comment 8

[Drew Willcoxon :adw]

Comment on attachment 362053 [details] [diff] [review]
Patch v1.0

Thanks Michael!  It looks good to me.  I'm asking mak77 to review it too, only because I'm not a peer.

> I'm not that experienced in programming in JS, so I bookmarked the
> developers.mozilla.org site about JS.

If you'd like to learn more, I definitely suggest checking out that site.  Also try http://javascript.crockford.com/

Thanks for contributing. :)

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 362053 [details] [diff] [review]
Patch v1.0

ideally we should sort only the date part, for a simple reason:
we used to have the file name localized, while in 3.1 we changed it to be always bookmarks-date.json.
so, localized files will be older than new not localized ones.
now suppose in your locale "bookmarks" word starts with the letter 'a', you will end up having all dates from the old localized files above the new ones.

Btw, i think adding a complexity level to handle that is not worth since old backups will be purged in a week or less (and if we are not purging them we have a bug to fix), so i think this is fine as it is.

Could you please check if we have a bug about using correct date formatting based on locale settings in the menu?
iso date is not exactly european friendly, we should show dates as the user is expecting them.

Comment 10

[Marco Bonardo [:mak]]

You could also immediately parse the filename when you get the file, extract the date part and convert it to a real date. Build an array of objects with date and file properties. Then you can use the date property both to sort and to print out a locale correct string. This would be a different approach that could solve both issues.

Comment 11

[Michael Kohler [:mkohler]]

(In reply to comment #9)
> Could you please check if we have a bug about using correct date formatting
> based on locale settings in the menu?
> iso date is not exactly european friendly, we should show dates as the user is
> expecting them.

I just found bug 457627. Is this enough specific for us or shall I fill a new bug?

(In reply to comment #10)
> You could also immediately parse the filename when you get the file, extract
> the date part and convert it to a real date. Build an array of objects with
> date and file properties. Then you can use the date property both to sort and
> to print out a locale correct string. This would be a different approach that
> could solve both issues.

Also a nice idea. But I think it's okay to take this patch here and then do all date format things in a different bugzilla entry.

Comment 12

[Marco Bonardo [:mak]]

no that bug is about another issue...

If you feel to address both issues here would be awesome since they touch the same code, if we  would handle formatting in a new bug we should also probably touch this code again to properly sort by date. Let us know if you feel to try that.

Comment 13

[Michael Kohler [:mkohler]]

Actually I don't have any idea how to do that, but I'll try your solution from comment 10. But pls don't expect a result too soon.

Comment 14

[Marco Bonardo [:mak]]

feel free to ask, and maybe sneak into #places channel in irc.mozilla.org if you need assistance. creating the array should be straight forward
for the date formatting code look in 
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/treeView.js#418

Comment 15

[Michael Kohler [:mkohler]]

Attachment: Patch v2.0

This should now be what you've expected. Works on 1.9.2/1.9.1 and 1.9.0 branch.

Code improvements are very welcome!

Comment 16

[Michael Kohler [:mkohler]]

Attachment: Patch v2.1

Corrected a code mistake.. Now it's

> if(...) {
>  .....
> }

instead of the ugly

> if(...)
> {
>   ...
> }

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 362475 [details] [diff] [review]
Patch v2.1

first of all, thanks for bringin on work on this! You're learning fast and i hope this is an interesting opportunity for both you and us.

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>--- a/browser/components/places/content/places.js
>+++ b/browser/components/places/content/places.js
>+        var fileProperties = [];
>+        var rx = new RegExp("^(bookmarks|" + localizedFilenamePrefix + ")-");
>+        var dateOnly = f.leafName.replace(rx, "").replace(/\.json$/, "").replace(/-/g, "");
>+        var year = dateOnly.substr(0, 4);
>+        var month = dateOnly.substr(4, 2);
>+        var day = dateOnly.substr(6, 2);

i think you could do var dateObj = new Date("YYYY/MM/DD"), you should only convert - to / instead of ""
Maybe you could also directly extract the date part from the string with a unique regexp using match
So extract only the date part with match, replace - with /, convert to a date with the above method.

>+        fileProperties["dateObject"] = new Date(year, month, day);
>+        fileList.push(fileProperties);

for clarity you should do

fileList.push({date: your_date_object, filename: f.leafName});

So you are pushing an object with 2 properties, later you will access these as fileList[i].date and fileList[i].filename

>+      m.setAttribute("label", fileList[i]["dateObject"].toLocaleDateString());

i know using system locale could work, but since we must adhere to application defaults like we do in trees. To do that we need to use nsIScriptableDateFormat.

var dateSvc = Cc["@mozilla.org/intl/scriptabledateformat;1"].
                         getService(Ci.nsIScriptableDateFormat);
dateSvc.FormatDate("", Ci.nsIScriptableDateFormat.dateFormatShort,
       dateObj.getFullYear(), dateObj.getMonth() + 1,
       dateObj.getDate());

the service should be defined out of any loop, while you can use it where you need it.

Comment 18

[Michael Kohler [:mkohler]]

Attachment: Patch v3.0

Now everything should be okay. Thanks a lot for the help, Marco!

If you still have improvements, they are *very* welcome.

(In reply to comment #17)
> first of all, thanks for bringin on work on this! You're learning fast and i
> hope this is an interesting opportunity for both you and us.

Yes, it's very interesting!

Comment 19

[Simon Bünzli]

Comment on attachment 362523 [details] [diff] [review]
Patch v3.0

Nice work. There's slightly more future-proofing we could do here, though:

>+      var rx = new RegExp("^(bookmarks|" + localizedFilenamePrefix + ")-.+\.json$");

We should be even stricter at what filenames we accept here (only those with dates in place of ".+"), as with this patch a file named e.g. "bookmarks-backup.json" or "bookmarks-2009-02-16.copy.json" will cause FormatDate to throw.

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 362523 [details] [diff] [review]
Patch v3.0

Nice, we are near the result

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>--- a/browser/components/places/content/places.js
>+++ b/browser/components/places/content/places.js
>@@ -412,51 +412,56 @@ var PlacesOrganizer = {
>+    var dateSvc = Cc["@mozilla.org/intl/scriptabledateformat;1"].
>+                         getService(Ci.nsIScriptableDateFormat);

we usually format services init with getService is directly under Cc, as:
var dateSvc = Cc["@mozilla.org/intl/scriptabledateformat;1"].
              getService(Ci.nsIScriptableDateFormat);

>+      var rx = new RegExp("^(bookmarks|" + localizedFilenamePrefix + ")-.+\.json$");

Yeah, i second Simon's suggestion, see below for a valid regexp, since we can use the same regexp here and below, define it only here but use it in both places (to check for existance and to match)

>+      if (!f.isHidden() && f.leafName.match(rx)) {
>+        var rx = new RegExp("^(bookmarks|" + localizedFilenamePrefix + ")-");
>+        var date = f.leafName.replace(rx, "").replace(/\.json$/, "").replace(/-/g, "/");

use match please:

regexp is "^(bookmarks|" + localizedFilenamePrefix + ")-([0-9]{4}-[0-9]{2}-[0-9]{2})\.json$"

then you do:

var date = f.leafName.match(rx)[2].replace(/-/g, "/");

the [2] will take the second () match (0 is the full string, 1 is first parentheses), that's a date in format NNNN-NN-NN

>+      m.setAttribute("label", dateSvc.FormatDate("", Ci.nsIScriptableDateFormat.
>+                dateFormatShort, fileList[i].date.getFullYear(), fileList[i].
>+                date.getMonth() + 1, fileList[i].date.getDate()));

this needs some formatting
m.setAttribute("label",
               dateSvc.FormatDate("", Ci.nsIScriptableDateFormat.dateFormatShort,
               fileList[i].date.getFullYear(), fileList[i].date.getMonth() + 1,
               fileList[i].date.getDate()));

Comment 21

[Marco Bonardo [:mak]]

ops, formatting in my last comment has gone fancy:

m.setAttribute("label",
               dateSvc.FormatDate("",
                                  Ci.nsIScriptableDateFormat.dateFormatShort,
                                  fileList[i].date.getFullYear(),
                                  fileList[i].date.getMonth() + 1,
                                  fileList[i].date.getDate()));

the only rule is that functions should be readable and lines not go over 80 chars

Comment 22

[Michael Kohler [:mkohler]]

Attachment: Patch v3.1

Thanks for help, Marco and Simon Bünzli.

Anything else I can improve? I've learnt a lot so far.

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 362558 [details] [diff] [review]
Patch v3.1

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>--- a/browser/components/places/content/places.js
>+++ b/browser/components/places/content/places.js
>+      m.setAttribute("label",
>+               dateSvc.FormatDate("",

formatting here is still a bit fancy, don't bother if you go over 80 chars by 1 or 2 chars, move params to be aligned

>+                                  Ci.nsIScriptableDateFormat.dateFormatShort,

I've tried this locally, and being this a restore menu, i find the long format much more readable (since it also states the weekday name, so i can tell: "i can recall that on monday bookmarks were fine, let's restore monday's bookmarks!")
So, please change this to Ci.nsIScriptableDateFormat.dateFormatLong

Nice work, r=me with these comments addressed
Thanks for your contribution!

Comment 24

[Michael Kohler [:mkohler]]

Attachment: Patch v3.2

Comment 25

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/984fd7884a84

Comment 26

[Michael Kohler [:mkohler]]

Verified with this http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux/1234891600/ . Waiting for tomorrow's nightly.

Comment 27

[Marco Bonardo [:mak]]

Comment on attachment 362566 [details] [diff] [review]
Patch v3.2

almost no risk, would be a nice polish change for a release.

Comment 28

[Marco Bonardo [:mak]]

i think this has more advantages than risks, wanted imo

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 362566 [details] [diff] [review]
Patch v3.2

a191=beltzner

Comment 30

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/611067897bf2

Comment 31

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090415 Shiretoko/3.5b4pre

Comment 32

[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