845992 - Merge similar code in folderPane.js and folderWidgets.xml for getting recent folders.

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[:aceman]

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.

Comment 1

[:aceman]

Attachment: patch

Comment 2

[[e-mail]]

<rant>
That's what you get from switching away from RDF in the first place.
</rant>

Comment 3

[[e-mail]]

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.

Comment 4

[Magnus Melin [:mkmelin]]

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

Comment 5

[:aceman]

(In reply to [e-mail] 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.

Comment 6

[:aceman]

(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.

Comment 7

[:aceman]

Attachment: patch v2

Comment 8

[Magnus Melin [:mkmelin]]

(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

Comment 9

[:aceman]

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

[Magnus Melin [:mkmelin]]

try "a".localeCompare("A").

Comment 11

[Magnus Melin [:mkmelin]]

Maybe that can't be called case sensitive, but it's a difference. Do we want it? I don't know.

Comment 12

[:aceman]

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.

Comment 13

[:aceman]

Oh, I can see it in the Recent folders view, as that sorts by name only.

Comment 14

[Magnus Melin [:mkmelin]]

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

Comment 15

[Magnus Melin [:mkmelin]]

But it needs a small fix due to bitrot

Comment 16

[:aceman]

Attachment: patch v3

Thanks, unbitrotted.

Comment 17

[:aceman]

Neil, ping :)

Comment 18

[[e-mail]]

Comment on attachment 727855 [details] [diff] [review]
patch v3

r=me based on the interdiff.

Comment 19

[:aceman]

Attachment: patch v4

Thanks. I just refreshed the patch and it is ready for check in.

Comment 20

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/fb97446ddad9

Comment 21

[Chris]

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.

Comment 22

[:aceman]

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

[Chris]

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!

Comment 24

[:aceman]

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.