Fix sorting and labels for the bookmark library restore menu

VERIFIED FIXED in Firefox 3.6a1

Status

()

--
minor
VERIFIED FIXED
11 years ago
3 years ago

People

(Reporter: mkohler, Assigned: mkohler)

Tracking

({polish, verified1.9.1, verifyme})

Trunk
Firefox 3.6a1
x86
All
polish, verified1.9.1, verifyme
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

11 years ago
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

Comment 1

11 years ago
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
(Assignee)

Comment 2

11 years ago
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 ;)
(Assignee)

Updated

10 years ago
OS: Windows Vista → All
(Assignee)

Comment 3

10 years ago
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

10 years ago
so, assigning to you for now, thanks for looking into this.
Assignee: nobody → michaelkohler
(Assignee)

Comment 5

10 years ago
Created attachment 362019 [details] [diff] [review]
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.
Attachment #362019 - Flags: review?(sdwilsh)

Updated

10 years ago
Attachment #362019 - Flags: review?(sdwilsh) → review?(adw)

Updated

10 years ago
Attachment #362019 - Flags: review?(adw) → review-

Comment 6

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

Comment 7

10 years ago
Created attachment 362053 [details] [diff] [review]
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.
Attachment #362019 - Attachment is obsolete: true
Attachment #362053 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #362053 - Flags: review? → review?(adw)

Updated

10 years ago
Attachment #362053 - Flags: review?(mak77)
Attachment #362053 - Flags: review?(adw)
Attachment #362053 - Flags: review+

Comment 8

10 years ago
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. :)

Updated

10 years ago
Attachment #362053 - Flags: review?(mak77) → review+

Comment 9

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

Comment 11

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

Comment 13

10 years ago
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
(Assignee)

Comment 15

10 years ago
Created attachment 362471 [details] [diff] [review]
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!
Attachment #362053 - Attachment is obsolete: true
Attachment #362471 - Flags: review?(mak77)
(Assignee)

Comment 16

10 years ago
Created attachment 362475 [details] [diff] [review]
Patch v2.1

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)

Updated

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

Comment 18

10 years ago
Created attachment 362523 [details] [diff] [review]
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!
Attachment #362475 - Attachment is obsolete: true
Attachment #362523 - Flags: review?(mak77)

Comment 19

10 years ago
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
(Assignee)

Comment 22

10 years ago
Created attachment 362558 [details] [diff] [review]
Patch v3.1

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)

Updated

10 years ago
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!

Updated

10 years ago
Keywords: polish
Target Milestone: --- → Firefox 3.2a1

Updated

10 years ago
Summary: Sort in the bookmark library restore menu → Fix sorting and labels for the bookmark library restore menu
(Assignee)

Comment 24

10 years ago
Created attachment 362566 [details] [diff] [review]
Patch v3.2
Attachment #362558 - Attachment is obsolete: true
Attachment #362566 - Flags: review?(mak77)

Updated

10 years ago
Attachment #362566 - Flags: review?(mak77) → review+

Updated

10 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/984fd7884a84
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Keywords: verifyme

Updated

10 years ago
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+
(Assignee)

Updated

10 years ago
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
Keywords: fixed1.9.1 → verified1.9.1
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.