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)
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)
3.08 KB,
patch
|
mak
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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)
Updated•16 years ago
|
Component: Bookmarks → Places
QA Contact: bookmarks → places
Version: unspecified → Trunk
Comment 1•16 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•16 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•15 years ago
|
OS: Windows Vista → All
Assignee | ||
Comment 3•15 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•15 years ago
|
||
so, assigning to you for now, thanks for looking into this.
Assignee: nobody → michaelkohler
Assignee | ||
Comment 5•15 years ago
|
||
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•15 years ago
|
Attachment #362019 -
Flags: review?(sdwilsh) → review?(adw)
Updated•15 years ago
|
Attachment #362019 -
Flags: review?(adw) → review-
Comment 6•15 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•15 years ago
|
||
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•15 years ago
|
Attachment #362053 -
Flags: review? → review?(adw)
Updated•15 years ago
|
Attachment #362053 -
Flags: review?(mak77)
Attachment #362053 -
Flags: review?(adw)
Attachment #362053 -
Flags: review+
Comment 8•15 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•15 years ago
|
Attachment #362053 -
Flags: review?(mak77) → review+
Comment 9•15 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.
Comment 10•15 years ago
|
||
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•15 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.
Comment 12•15 years ago
|
||
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•15 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.
Comment 14•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
Attachment #362475 -
Flags: review?(mak77) → review-
Comment 17•15 years ago
|
||
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•15 years ago
|
||
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•15 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 20•15 years ago
|
||
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-
Comment 21•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
Attachment #362558 -
Flags: review?(mak77) → review+
Comment 23•15 years ago
|
||
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•15 years ago
|
Summary: Sort in the bookmark library restore menu → Fix sorting and labels for the bookmark library restore menu
Assignee | ||
Comment 24•15 years ago
|
||
Attachment #362558 -
Attachment is obsolete: true
Attachment #362566 -
Flags: review?(mak77)
Updated•15 years ago
|
Attachment #362566 -
Flags: review?(mak77) → review+
Updated•15 years ago
|
Keywords: checkin-needed
Comment 25•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/984fd7884a84
Assignee | ||
Comment 26•15 years ago
|
||
Verified with this http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux/1234891600/ . Waiting for tomorrow's nightly.
Updated•15 years ago
|
Attachment #362566 -
Flags: approval1.9.1?
Comment 27•15 years ago
|
||
Comment on attachment 362566 [details] [diff] [review] Patch v3.2 almost no risk, would be a nice polish change for a release.
Comment 28•14 years ago
|
||
i think this has more advantages than risks, wanted imo
Flags: wanted-firefox3.1?
Updated•14 years ago
|
Attachment #362566 -
Flags: approval1.9.1? → approval1.9.1+
Comment 29•14 years ago
|
||
Comment on attachment 362566 [details] [diff] [review] Patch v3.2 a191=beltzner
Assignee | ||
Updated•14 years ago
|
Flags: wanted-firefox3.5?
Comment 30•14 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/611067897bf2
Keywords: fixed1.9.1
Comment 31•14 years ago
|
||
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
Comment 32•14 years ago
|
||
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.
Description
•