Closed
Bug 845992
Opened 11 years ago
Closed 11 years ago
Merge similar code in folderPane.js and folderWidgets.xml for getting recent folders.
Categories
(Thunderbird :: Folder and Message Lists, defect)
Thunderbird
Folder and Message Lists
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 25.0
People
(Reporter: aceman, Assigned: aceman)
Details
Attachments
(1 file, 3 obsolete files)
9.67 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
The folderPane.js code for the Recent view uses very similar code to folderWidgets.xml for the Recent submenu of folder picker. They get the list of recent folders. So let's try to merge this code.
Attachment #719154 -
Flags: review?(neil)
Attachment #719154 -
Flags: review?(mkmelin+mozilla)
Comment 2•11 years ago
|
||
<rant> That's what you get from switching away from RDF in the first place. </rant>
Comment 3•11 years ago
|
||
Comment on attachment 719154 [details] [diff] [review] patch I can't test the folder pane so maybe I should just give f+ instead of r+? >+ let allFolders = toArray(this.fixIterator(this.MailServices.accounts.allFolders, >+ Ci.nsIMsgFolder)); this.toArray? >+ let allFolders = allFolders.filter(function (f) f.canFileMessages); What does this achieve, I don't see it taken from anywhere? >+ recentFolders.push({ folder: aFolder, >+ time: time, >+ name: aFolder.prettyName }); name is unused.
Attachment #719154 -
Flags: review?(neil)
Comment 4•11 years ago
|
||
Comment on attachment 719154 [details] [diff] [review] patch Review of attachment 719154 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/base/content/folderPane.js @@ +2348,5 @@ > function sorter(a, b) { > let sortKey = a._folder.compareSortKeys(b._folder); > if (sortKey) > return sortKey; > + return a.text.localeCompare(b.text); this isn't case insensitive anymore? ::: mailnews/base/content/folderWidgets.xml @@ +508,2 @@ > > + let allFolders = toArray(this.fixIterator(this.MailServices.accounts.allFolders, like neil noted, this is wrong Error: ReferenceError: toArray is not defined Source File: chrome://messenger/content/folderWidgets.xml Line: 509
Attachment #719154 -
Flags: review?(mkmelin+mozilla) → review-
(In reply to neil@parkwaycc.co.uk from comment #3) > I can't test the folder pane so maybe I should just give f+ instead of r+? You should r+ the mailnews changes. Should I split the patch? > >+ let allFolders = toArray(this.fixIterator(this.MailServices.accounts.allFolders, > >+ Ci.nsIMsgFolder)); > this.toArray? > > >+ let allFolders = allFolders.filter(function (f) f.canFileMessages); > What does this achieve, I don't see it taken from anywhere? It was there in folderWidgets.xml before this change.
(In reply to Magnus Melin from comment #4) > ::: mail/base/content/folderPane.js > @@ +2348,5 @@ > > function sorter(a, b) { > > let sortKey = a._folder.compareSortKeys(b._folder); > > if (sortKey) > > return sortKey; > > + return a.text.localeCompare(b.text); > > this isn't case insensitive anymore? It is still case insensitive.
Attachment #719154 -
Attachment is obsolete: true
Attachment #723204 -
Flags: review?(mkmelin+mozilla)
Attachment #723204 -
Flags: review?(neil)
Comment 8•11 years ago
|
||
(In reply to :aceman from comment #6) > > > + return a.text.localeCompare(b.text); > > > > this isn't case insensitive anymore? > > It is still case insensitive. how? localeCompare is case sensitive
It is not case sensitive in my tests. It sorts fine in the folder pane. Typing "a".localeCompare("B") into Firefox error console returns -1, the same as "a".localeCompare("b"). Do you have a testcase?
Comment 10•11 years ago
|
||
try "a".localeCompare("A").
Comment 11•11 years ago
|
||
Maybe that can't be called case sensitive, but it's a difference. Do we want it? I don't know.
Assignee | ||
Comment 12•11 years ago
|
||
Yeah, it would probably sort all 'a' before all 'A' and then 'b' and 'B'. On the other hand, the original comparison may choke on special accented characters. E.g. try this: "ň".toLowerCase() < "ú".toLowerCase() returns 'false' which is wrong. n is lower than u. I have not seen this manifest itself in real TB, but maybe it incidentally takes the .compareSortKeys() path in common cases.
Assignee | ||
Comment 13•11 years ago
|
||
Oh, I can see it in the Recent folders view, as that sorts by name only.
Comment 14•11 years ago
|
||
Comment on attachment 723204 [details] [diff] [review] patch v2 Review of attachment 723204 [details] [diff] [review]: ----------------------------------------------------------------- Giving it some though, I think i'm happy with this. r=mkmelin
Attachment #723204 -
Flags: review?(mkmelin+mozilla) → review+
Comment 15•11 years ago
|
||
But it needs a small fix due to bitrot
Assignee | ||
Comment 16•11 years ago
|
||
Thanks, unbitrotted.
Attachment #723204 -
Attachment is obsolete: true
Attachment #723204 -
Flags: review?(neil)
Attachment #727855 -
Flags: review?(neil)
Comment 18•11 years ago
|
||
Comment on attachment 727855 [details] [diff] [review] patch v3 r=me based on the interdiff.
Attachment #727855 -
Flags: review?(neil) → review+
Assignee | ||
Comment 19•11 years ago
|
||
Thanks. I just refreshed the patch and it is ready for check in.
Attachment #727855 -
Attachment is obsolete: true
Attachment #781937 -
Flags: review+
Flags: needinfo?(neil)
Keywords: checkin-needed
Comment 20•11 years ago
|
||
https://hg.mozilla.org/comm-central/rev/fb97446ddad9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
Comment 21•11 years ago
|
||
I've looked everywhere and can't find someone who has the same problem as myself. I hope you can help... Error: NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 [nslMsgFolder.compareSortKeys] Source File: chrome://messenger/content/folderPane.js Line 2426 Symptoms: My folder pane is completely empty and my address book doesn't show anything either. But, I do see the data there in the folders. I'm using MAC OS vs. 10.6.8 and tried uninstalling the most current version of TB and even went back to several versions older, but still having the same symptoms.
Assignee | ||
Comment 22•11 years ago
|
||
Is is somehow related to this bug? Does the problem happen only after this patch has landed? But you say it also happen in older versions so it can't be related... Also, what is nslMsgFolder ? It should be nsIMsgFolder.
Comment 23•11 years ago
|
||
I don't know. I've re-installed vs. 17.8 and then went back to vs 17.6 just to test. When I start them, both act the same...just blank folder panes. :( I'm not a programmer, but can't find anything online related specifically to this error message. Also, yes, I'm sure that was a typo (ie, nsI... vs nsl...). Do you have any suggestions? Thanks!
Assignee | ||
Comment 24•11 years ago
|
||
If you are not sure then please open a new bug and CC me on it. State there which TB version you were using when the problem first appeared.
You need to log in
before you can comment on or make changes to this bug.
Description
•