Closed Bug 421834 Opened 16 years ago Closed 15 years ago

Fix sorting and labels for the bookmark library restore menu

Categories

(Firefox :: Bookmarks & History, defect)

x86
All
defect
Not set
minor

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: mkohler, Assigned: mkohler)

References

()

Details

(Keywords: polish, verified1.9.1, verifyme)

Attachments

(1 file, 6 obsolete files)

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)
Component: Bookmarks → Places
QA Contact: bookmarks → places
Version: unspecified → Trunk
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.
Severity: trivial → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
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 ;)
OS: Windows Vista → All
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).
so, assigning to you for now, thanks for looking into this.
Assignee: nobody → michaelkohler
Attached patch Patch v0.1a (obsolete) — Splinter Review
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.
Attachment #362019 - Flags: review?(sdwilsh)
Attachment #362019 - Flags: review?(sdwilsh) → review?(adw)
Attachment #362019 - Flags: review?(adw) → review-
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.
Attached patch Patch v1.0 (obsolete) — Splinter Review
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.
Attachment #362019 - Attachment is obsolete: true
Attachment #362053 - Flags: review?
Attachment #362053 - Flags: review? → review?(adw)
Attachment #362053 - Flags: review?(mak77)
Attachment #362053 - Flags: review?(adw)
Attachment #362053 - Flags: review+
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. :)
Attachment #362053 - Flags: review?(mak77) → review+
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.
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.
(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.
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.
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.
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
Attached patch Patch v2.0 (obsolete) — Splinter Review
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!
Attachment #362053 - Attachment is obsolete: true
Attachment #362471 - Flags: review?(mak77)
Attached patch Patch v2.1 (obsolete) — Splinter Review
Corrected a code mistake.. Now it's

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

instead of the ugly

> if(...)
> {
>   ...
> }
Attachment #362471 - Attachment is obsolete: true
Attachment #362475 - Flags: review?(mak77)
Attachment #362471 - Flags: review?(mak77)
Attachment #362475 - Flags: review?(mak77) → review-
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.
Attached patch Patch v3.0 (obsolete) — Splinter Review
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!
Attachment #362475 - Attachment is obsolete: true
Attachment #362523 - Flags: review?(mak77)
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 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()));
Attachment #362523 - Flags: review?(mak77) → review-
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
Attached patch Patch v3.1 (obsolete) — Splinter Review
Thanks for help, Marco and Simon Bünzli.

Anything else I can improve? I've learnt a lot so far.
Attachment #362523 - Attachment is obsolete: true
Attachment #362558 - Flags: review?(mak77)
Attachment #362558 - Flags: review?(mak77) → review+
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!
Keywords: polish
Target Milestone: --- → Firefox 3.2a1
Summary: Sort in the bookmark library restore menu → Fix sorting and labels for the bookmark library restore menu
Attached patch Patch v3.2Splinter Review
Attachment #362558 - Attachment is obsolete: true
Attachment #362566 - Flags: review?(mak77)
Attachment #362566 - Flags: review?(mak77) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/984fd7884a84
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Keywords: verifyme
Attachment #362566 - Flags: approval1.9.1?
Comment on attachment 362566 [details] [diff] [review]
Patch v3.2

almost no risk, would be a nice polish change for a release.
i think this has more advantages than risks, wanted imo
Flags: wanted-firefox3.1?
Attachment #362566 - Flags: approval1.9.1? → approval1.9.1+
Flags: wanted-firefox3.5?
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090415 Shiretoko/3.5b4pre
Status: RESOLVED → VERIFIED
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.