439367 - De-rdfifiy msgViewNavigation.js

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Joey Minta]

Attachment: patch v1

msgViewNavigation currently uses the rdf datasources to sort accounts, when really, there's a standard (known) way to do so.  We can make this file rdf-independent by reimplementing the sorting function.

(And I couldn't resist cleaning up the remaining global variables.)

Comment 1

[Mark Banner (:standard8)]

Comment on attachment 325205 [details] [diff] [review]
patch v1

+  function accountCompare(a, b) {

Rather than ending up with compare functions all over the place (and therefore having multiple places to change for things like if we implement a fix for allowing the user to change the order of accounts), would it not be better to provide a function/attribute in nsIMsgAccountManager along the lines of:

readonly attribute nsI?Array sortedAccounts;

+  var count = acctMgr.accounts.Count();
 
...
+  for (var i = 0; i < count; i++) {
+    var acct = acctMgr.accounts.GetElementAt(i)
+                      .QueryInterface(Components.interfaces.nsIMsgAccount);

IMO this is possibly inefficient - acctMgr.accounts will be going across the xpconnect boundary each time its called. Seeing as you don't use acctMgr elsewhere I think you could just store acctMgr.accounts in a temporary.

Comment 2

[David :Bienvenu]

I agree with what Standard8 said. But, ultimately this logic should be in the folder view, i.e., it should be a method of the current folder view to find the next folder with unread messages in the current folder view, just like it is for the thread pane view to find the next unread message. I'll try this patch out, but ultimately this logic should all change.

Comment 3

[David :Bienvenu]

Comment on attachment 325205 [details] [diff] [review]
patch v1

r/sr=me, modulo Standard8's comment about caching accountMgr.accounts, and my comments about how this will need to change once you kick rdf out of the folder pane :-)

Comment 4

[Joey Minta]

I do use acctMgr elsewhere (to get the default key), but I cached it anyway.

Checking in mailnews/base/resources/content/msgViewNavigation.js;
/cvsroot/mozilla/mailnews/base/resources/content/msgViewNavigation.js,v  <--  msgViewNavigation.js
new revision: 1.60; previous revision: 1.59
done

Comment 5

[[e-mail]]

Comment on attachment 325205 [details] [diff] [review]
patch v1

>+  function compareFolderSortKey(folder1, folder2) {
>+    return folder1.compareSortKeys(folder2);
Maybe we should just implement compareSortKeys for root folders?

>+  var acctMgr = Components.classes["@mozilla.org/messenger/account-manager;1"].
>+                getService(Components.interfaces.nsIMsgAccountManager);
Since you have .classes I prefer .getService

>+    var acct = acctMgr.accounts.GetElementAt(i)
>+                      .QueryInterface(Components.interfaces.nsIMsgAccount);
Nit: this should use .queryElementAt instead.