Closed Bug 749200 Opened 12 years ago Closed 12 years ago

merge sortAccounts() function in AccountManager.js and serverCompare() in folderWidgets.xml

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 19.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

(Keywords: addon-compat)

Attachments

(2 files, 13 obsolete files)

14.79 KB, patch
standard8
: review+
neil
: feedback+
Details | Diff | Splinter Review
31.95 KB, patch
mconley
: review+
Details | Diff | Splinter Review
It seems the implementations are very similar. One sorts account, the other one servers (I know that folderWidgets.xml was just recently converted to sort servers in bug 742238). Maybe it would be beneficial to merge them and make a common implementation (or both versions) available in some module, like folderUtils.js .

Also, the comment about the ordering says:
/**
309              * This is our actual function for sorting servers.  Servers go
310              * in the following order: (1) default account (2) other mail
311              * accounts (3) Local Folders (4) news
312              */

Maybe it would be time to think where IM type should be positioned if we are sorting accounts where the IM type is still present (it is not in servers if called via accountManager.allServers).
While touching this code, I was surprised that RSS accounts aren't at the end with news accounts. They are currently sorted alphabetically with mail accounts; which didn't seem quite right, but I didn't want to include a behavior change in the patch for bug 742238 which was fixing a regression.
Good catch.
So what is the proposal? (5) RSS (6) IM ? Or something better?
What are the use cases? The case I modified in bug 742238 doesn't want to have IM accounts in the list at all.

Anyway, you probably want to hear Blake's opinion on this before starting work on a patch.
In general I'm happy when people want to clean up code.  Happier still if they're going to write tests to ensure they don't break things first.  ;)
Per the bug summary the only place I know of NOW is the Account manager. That one will want to sort IM accounts in some way (currently they are mixed into mail accounts).
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #4)
> In general I'm happy when people want to clean up code.

I think the question for you was: Should IM accounts be displayed after email accounts, or mixed together?

(In reply to :aceman from comment #5)
> Per the bug summary the only place I know of NOW is the Account manager.

See also the Go -> Folder menupopup.
(In reply to Florian Quèze from comment #6)
> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #4)
> > In general I'm happy when people want to clean up code.
> I think the question for you was: Should IM accounts be displayed after
> email accounts, or mixed together?

Not mixed.

(1) default account (2) other mail accounts (3) Local Folders (4) IM accounts (5) rss (6) news

Later,
Blake.
(In reply to Florian Quèze from comment #6)
> See also the Go -> Folder menupopup.
But there I assume your version without IM accounts is used so their sort order is not important.

But yes, I intend to make the functions universal so they should handle IM accounts/servers properly IF encountered in the array.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #7)
> (1) default account (2) other mail accounts (3) Local Folders (4) IM
> accounts (5) rss (6) news

Thanks, nice order.
Assignee: nobody → acelists
Depends on: 738810
Attached patch proof of concept (obsolete) — Splinter Review
So this is the direction I indent to go.

I will still do some polish (like comments) if the general idea is approved.
Also folderUtils.jsm is not the best place for it, but I couldn't find a better one so far.
Attachment #619317 - Flags: ui-review?(bwinton)
Attachment #619317 - Flags: feedback?(florian)
Status: NEW → ASSIGNED
Comment on attachment 619317 [details] [diff] [review]
proof of concept

Yep, that looks like the order I think we want.  :)

ui-r=me!

Thanks,
Blake.
Attachment #619317 - Flags: ui-review?(bwinton) → ui-review+
Thanks. Who would be good to review something like this?
The Magic 8-ball says…

Potential reviewers:
  bienvenu: 10
  standard8: 6
  asuth: 5
  iann: 4

Potential super-reviewers:
  bienvenu: 11
  neil: 6
  standard8: 3
And what do those numbers mean?
They're the number of times they've reviewed code in files like the ones you've changed.  (Or, basically, they're meaningless.  ;)
OK. Let's get feedback from florian if it doesn't break his use case (bug 742238) and then I'll polish the patch and try review.
Comment on attachment 619317 [details] [diff] [review]
proof of concept

(In reply to :aceman from comment #15)
You took care of my use case so I don't see anything that would break it.

However, I don't really like this code (especially the 2 global variables, and maybe also the duplication inside the compareAccountsServers function).

Not sure this would work as I haven't actually tried, but if I wanted to rewrite that function, I would probably write a function converting an incoming server to a number, and compare only these numbers. Feel free to ignore this comment and request review on a patch with the current approach though, it's so close to what already existed (that I already disliked) that it may pass review anyway.

Coding style nit: the indent in compareAccountsServers isn't right.
Attachment #619317 - Flags: feedback?(florian)
What duplication do you mean?
You mean converting server to a number representing it's precedence in the list (like the (1) .. (6) from the list bwinton proposed) and then sort those numbers? That could work too, however I am not sure if it would be a more readable abstraction.

Surely this code works, I have tested it in the folder pickers, just not polished the code enough.
I'll see the indent.
(In reply to :aceman from comment #17)
> What duplication do you mean?

This pattern:
      let aIs<type> = aType == "<type>";
      let bIs<type> = bType == "<type>";
      if (aIs<type> && !bIs<type>)
        return 1;
      if (bIs<type> && !aIs<type>)
        return -1;

> You mean converting server to a number representing it's precedence in the
> list (like the (1) .. (6) from the list bwinton proposed) and then sort
> those numbers?

Yes. (It's still the accounts/servers you want to sort, not the numbers, but I'm almost sure that's what you meant.)

> That could work too, however I am not sure if it would be a
> more readable abstraction.

Not sure either, as I haven't tried. I just thought it may be less error prone when adding more types in the future. I dislike editing code that involves copy and paste ;-).
To remove that duplication I would probably need some new helper function. But the perf is not critical here, so maybe not a problem.

I can try the numbering scheme to see if it is any prettier.
Note the results of this search http://mxr.mozilla.org/comm-central/ident?i=sortAccounts

Looks like the goodness can be spread to other parts of the code base (at least 3 of them anyway).
Good catches Ian! Looks like this will be worth something.
Florian, did you mean something like this?
(the numbers are taken directly from the list in comment 7, so it looks more readable than the +1, -1 and comparison order dependent result)
function getServerPrecedence(AccountServer) {
  if (AccountServer.key == gDefaultAccountServerKey)
    return 1;

  let serverType;
  if (gSortAccounts)
    serverType = AccountServer.incomingServer.type;
  else
    serverType = AccountServer.type;

  switch (serverType) {
    case "none":
      return 3;
    case "im":
      return 4;
    case "rss":
      return 5;
    case "nntp":
      return 6;
    default:
      return 2;
  }
}

function compareAccountsServers(a, b) {
  return (getServerPrecedence(a) - getServerPrecedence(b));
}
aceman: I think Florian meant something like the Schwartzian Transform - https://en.wikipedia.org/wiki/Schwartzian_transform  :)

Later,
Blake.
Attached patch temporary patch (obsolete) — Splinter Review
This patch just shows the current state. It is not final and complete, just to show SM people where I am standing now.
(In reply to :aceman from comment #23)
> Florian, did you mean something like this?

Yes, I like this code much better!

What Blake suggests in comment 24 seems like an additional performance improvement, but I was more worried by code readability issues than by performance.
Yes, I understand that the precendence is counted from the type multiple times for each element as the .sort is comparing the elements. So the values could be precomputed before running the sort. But it may require a new global "cache" array to lookup the values. Is it needed?

What numbers of accounts do users get into? 10s, 100s? :)
We already have a precedence function for non-root nsIMsgFolder instances, either using the sortOrder property or more directly using compareSortKeys which compares both the sortOrder and the name. (For some reason folderWidgets.xml doesn't trust this though...) Would it be worthwhile to add a precedence function to nsIMsgIncomingServer and/or nsIMsgAccount and/or hook up sortOrder and/or compareSortKeys to work with root folders?
That could be follow-up work after this bug. I am just consolidating all the places that already do NOT use the .sortOrder and compareSortkeys, but order by server.type.

Then, the central sorting function (created in this patch) can be updated to use anything you wish or consider better. Could you please file that?
Attached patch real patch (obsolete) — Splinter Review
This patch does this:
1. creates common functions in folderUtils.jsm for sorting of accounts and servers based on their server type. 
2. adds coverage for IM and RSS server type to be properly placed (this is added functionality that would be needed to be done in all those places (from step 3) if this patch is not accepted.
3. finds and converts many places in the codebase what were doing their own (mostly copied) sorting of any servers/accounts they needed.

So, the patch is big but it is mostly very nice code removal.

These spots were indentified and converted (/mailnews and TB):
- the account manager tree order
- the folder picker order
- the folder pane order
- the order of identities in the compose window
- something in msgViewNavigation.js (I've tested the jump into next account for unread messages)

All these areas I have tested by hand and with xpcshell tests (they pass).

Seamonkey:
- the account manager tree order
- the folder picker order in /mailnews (does SM use that?)
- the folder pane order
- the order of identities in the compose window
- something in msgViewNavigation.js
- also there was sorting code in mailnews/base/src/nsMsgAccountManagerDS.cpp#461 that is supposedly used for ordering of the folder pane in SM. That order probably should be kept in sync with the msgViewNavigation.js function so I updated the C++ code (just couldn't de-duplicate).

The SM parts are completely untested I just copied the TB changes.

The new functions have functionality to accept a list to sort instead of getting all servers/accounts by themselves. But I have not yet used this anywhere.
Usually it looked to me more beneficial (as in shorter code) to just let the new functions fetch a complete list and only then strip away objects not needed (like IM or deferred POP3 accounts) compared to building the stripped list first itself and then let it sort.

Also, maybe some places could be converted to use servers directly instead of accounts. But that is not up to me to decide. So I have just converted the code as is.
Attachment #619317 - Attachment is obsolete: true
Attachment #619906 - Attachment is obsolete: true
Attachment #619932 - Flags: review?(iann_bugzilla)
Attachment #619932 - Flags: ui-review?(bwinton)
Comment on attachment 619932 [details] [diff] [review]
real patch

I played around with it, and it seemed good to me.  ui-r+

Thanks,
Blake.
Attachment #619932 - Flags: ui-review?(bwinton) → ui-review+
Comment on attachment 619932 [details] [diff] [review]
real patch

This is just on code inspection, not tested as yet.

>+++ b/mail/base/content/folderPane.js
>-  _sortedAccounts: function ftv_getSortedAccounts()
>-  {
>-      const Cc = Components.classes;
>-      const Ci = Components.interfaces;
>-      let acctMgr = Cc["@mozilla.org/messenger/account-manager;1"]
>-                       .getService(Ci.nsIMsgAccountManager);
>-      let accounts = [a for each
>-                      (a in fixIterator(acctMgr.accounts, Ci.nsIMsgAccount))];
>-      // Bug 41133 workaround
>-      accounts = accounts.filter(function fix(a) { return a.incomingServer; });
>+  _sortedAccounts: function ftv_getSortedAccounts() {
>+    let accounts = allAccountsSorted(accounts);
You're passing accounts as an argument yet you have removed its definition.
> 
>-      // Don't show deferred pop accounts
>-      accounts = accounts.filter(function isNotDeferred(a) {
>-        let server = a.incomingServer;
>-        return !(server instanceof Ci.nsIPop3IncomingServer &&
>-                 server.deferredToAccount);
Is Ci defined elsewhere within the scope as you have removed its definition.

>+++ b/mailnews/base/util/folderUtils.jsm

>+function getServerPrecedence(AccountServer) {
Other functions have arguments of the format aVariableName, so best to stick with that format.

>+  if (AccountServer.key == gDefaultAccountServerKey)
>+    return 1;
>+
>+  let serverType;
>+  if (gSortAccounts)
>+    serverType = AccountServer.incomingServer.type;
>+  else
>+    serverType = AccountServer.type;
>+
>+  switch (serverType) {
>+    case "none":
>+      return 3;
>+    case "im":
>+      return 4;
>+    case "rss":
>+      return 5;
>+    case "nntp":
>+      return 6;
>+    default:
>+      return 2;
>+  }
>+}
As this only might be short lived, if what Neil suggests happens, I won't insist that these numbers should be constants e.g. const kServerTypeDefault = 1; const kServerTypeRSS = 5; etc, but someone else might.

>+
>+/**
>+ * Compares the passed in objects according to their precedence.
>+ */
>+function compareAccountsServers(a, b) {
>+  return (getServerPrecedence(a) - getServerPrecedence(b));
>+}
Maybe the argument variable names could be tweaked to be of the format aVariableName here too, but not too worried about this function.
Not sure you need the extract brackets/braces around what you are returning.

>+/**
>+ * Returns a list ofservers sorted by server type.
>+ *
>+ * @param serverList  the list of servers to sort.
>+ *                    If not specified, all existing servers are used.
>+ */
>+function allServersSorted(serverList) {
Do we ever pass an argument in? I cannot see anywhere that we do or is this for a potential future use? If you keep the argument needs to be of the format aVariableName.

>+  if (!serverList) {
>+    // allServers array does not contain IM accounts
>+    let allServers = MailServices.accounts.allServers;
>+    let count = allServers.Count();
>+
>+    serverList = [];
>+    for (let i = 0; i < count; i++)
>+      serverList.push(
>+        allServers.QueryElementAt(i, Components.interfaces.nsIMsgIncomingServer)
>+      );
Can this be done something like:
serverList = [a for each (a in fixIterator(MailServices.accounts.allServers,
                                           Components.interfaces.nsIMsgIncomingServer))];

See http://mxr.mozilla.org/comm-central/source/mail/base/modules/MailUtils.js#72 and http://mxr.mozilla.org/comm-central/source/mail/components/search/content/searchCommon.js#348

>+/**
>+ * Returns a list of accounts sorted by server type.
>+ *
>+ * @param accountList  the list of accounts to sort.
>+ *                     If not specified, all existing accounts are used.
>+ */
>+function allAccountsSorted(accountList) {
Do we ever pass an argument in? I cannot see anywhere that we do or is this for a potential future use? If you keep the argument needs to be of the format aVariableName.

>+++ b/suite/mailnews/compose/MsgComposeCommands.js
> function FillIdentityList(menulist)
> {
>+  let accounts = allAccountsSorted();
Leave as var please.

>+++ b/suite/mailnews/msgViewNavigation.js

> function GetRootFoldersInFolderPaneOrder()
> {
>+  let accounts = allAccountsSorted();
Leave as var please.

>-  var serversMsgFolders = new Array();
>-  for each (var acct in accounts)
>-    serversMsgFolders.push(acct.incomingServer.rootMsgFolder);
>+  let serversMsgFolders = new Array();
Leave as var please.
>+  for each (let account in accounts)
>+    serversMsgFolders.push(account.incomingServer.rootMsgFolder);

r- for the moment
Attachment #619932 - Flags: review?(iann_bugzilla) → review-
(In reply to Ian Neal from comment #32)
> >+++ b/mail/base/content/folderPane.js
> Is Ci defined elsewhere within the scope as you have removed its definition.
I think it is not used anywhere else, but will check again.

> >+++ b/mailnews/base/util/folderUtils.jsm
> >+/**
> >+ * Returns a list ofservers sorted by server type.
> >+ *
> >+ * @param serverList  the list of servers to sort.
> >+ *                    If not specified, all existing servers are used.
> >+ */
> >+function allServersSorted(serverList) {
> Do we ever pass an argument in? I cannot see anywhere that we do or is this
> for a potential future use? If you keep the argument needs to be of the
> format aVariableName.
For future use, see comment 30.

> >+/**
> >+ * Returns a list of accounts sorted by server type.
> >+ *
> >+ * @param accountList  the list of accounts to sort.
> >+ *                     If not specified, all existing accounts are used.
> >+ */
> >+function allAccountsSorted(accountList) {
> Do we ever pass an argument in? I cannot see anywhere that we do or is this
> for a potential future use? If you keep the argument needs to be of the
> format aVariableName.
For future use, see comment 30.

All the rest good catches, thanks. I'll update it.
Attached patch patch v2 (obsolete) — Splinter Review
Thanks, fixed.
Attachment #619932 - Attachment is obsolete: true
Attachment #620849 - Flags: review?(iann_bugzilla)
Comment on attachment 620849 [details] [diff] [review]
patch v2

>+++ b/mail/base/content/folderPane.js

>+    // Don't show deferred pop accounts
Nit: Comment needs a full stop at the end of it.

>+    // Don't show IM accounts
Nit: Comment needs a full stop at the end of it.

>+++ b/mail/base/content/msgViewNavigation.js

>+  // Don't show IM accounts
Nit: Comment needs a full stop at the end of it.

r=me with those comments fixed.
Attachment #620849 - Flags: review?(iann_bugzilla) → review+
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #620849 - Attachment is obsolete: true
Attachment #622456 - Flags: review?(neil)
Comment on attachment 622456 [details] [diff] [review]
patch v3

Sorry for not getting to this review earlier.

>+            var servers = this.allServersSorted();
> 
>             // Now generate our folder-list.  Note that we'll special case this
>             // situation below, to avoid destroying the sort order we just made
>             folders = servers.map(function(s) s.rootFolder);
This seems to be the only caller of allServersSorted. Would it be possible to switch this to use allAccountsSorted, and then write
folders = accounts.map(function(a) a.incomingServer.rootFolder);

>+var gSortAccounts;
IMHO, this sucks. Sorry about that.

>+function compareAccountsServers(aAccountServer1, aAccountServer2) {
>+  return getServerPrecedence(aAccountServer1) - getServerPrecedence(aAccountServer2);
What happens when two accounts have the same precedence?

>+ * @param aServerList  the list of servers to sort.
>+ *                     If not specified, all existing servers are used.
Does anyone actually specify the server (or account) list?

>+    aServerList = [a for each (a in fixIterator(
>+      MailServices.accounts.allServers, Components.interfaces.nsIMsgIncomingServer))];
Converting from an array to an iterator to an array seems suboptimal.

>     let identites = queryISupportsArray(account.identities,
>                                         Components.interfaces.nsIMsgIdentity);
Ah, now this is just the thing we could have done with above ;-)
(In reply to neil@parkwaycc.co.uk from comment #37)
> >+            var servers = this.allServersSorted();
> > 
> >             // Now generate our folder-list.  Note that we'll special case this
> >             // situation below, to avoid destroying the sort order we just made
> >             folders = servers.map(function(s) s.rootFolder);
> This seems to be the only caller of allServersSorted. Would it be possible
> to switch this to use allAccountsSorted, and then write
> folders = accounts.map(function(a) a.incomingServer.rootFolder);
Maybe. Florian, can you tell?

> >+var gSortAccounts;
> IMHO, this sucks. Sorry about that.
Why? Bad name? Any suggestion?

> >+function compareAccountsServers(aAccountServer1, aAccountServer2) {
> >+  return getServerPrecedence(aAccountServer1) - getServerPrecedence(aAccountServer2);
> What happens when two accounts have the same precedence?
Nothing, they stay in the order they are defined (relatively to each other). Assuming stable sort in JS.

> >+ * @param aServerList  the list of servers to sort.
> >+ *                     If not specified, all existing servers are used.
> Does anyone actually specify the server (or account) list?
No. For potential future use, see comment 30. Should I remove the functionality?

> >+    aServerList = [a for each (a in fixIterator(
> >+      MailServices.accounts.allServers, Components.interfaces.nsIMsgIncomingServer))];
> Converting from an array to an iterator to an array seems suboptimal.
> 
> >     let identites = queryISupportsArray(account.identities,
> >                                         Components.interfaces.nsIMsgIdentity);
> Ah, now this is just the thing we could have done with above ;-)
Sorry, I just copied all this, I can't tell what is good code and what not. The thing with servers was recommended to me in comment 32.
I can't make this better myself without suggestions.
(In reply to :aceman from comment #38)
> (In reply to neil@parkwaycc.co.uk from comment #37)
> > >+            var servers = this.allServersSorted();
> > > 
> > >             // Now generate our folder-list.  Note that we'll special case this
> > >             // situation below, to avoid destroying the sort order we just made
> > >             folders = servers.map(function(s) s.rootFolder);
> > This seems to be the only caller of allServersSorted. Would it be possible
> > to switch this to use allAccountsSorted, and then write
> > folders = accounts.map(function(a) a.incomingServer.rootFolder);
> Maybe. Florian, can you tell?

allServers doesn't contain the servers of IM accounts.
allAccounts includes IM accounts, so you would need to add a filter call to exclude IM servers.
Would this suffice?
+    let accounts = allAccountsSorted();
+    // Don't show IM accounts.
+    accounts = accounts.filter(function(a) {
+      return a.incomingServer.type != "im";
+    });

It is already there in the patch for mail/base/content/folderPane.js .
(In reply to :aceman from comment #40)
> Would this suffice?

I think it would, yes.
(In reply to aceman from comment #38)
> (In reply to comment #37)
> > >+var gSortAccounts;
> > IMHO, this sucks. Sorry about that.
> Why? Bad name? Any suggestion?
Well, it's the way you need to fiddle about with global variables so that you can use the same routine to sort both accounts and servers.

> Assuming stable sort in JS.
I don't think you can assume that.

> > >+ * @param aServerList  the list of servers to sort.
> > >+ *                     If not specified, all existing servers are used.
> > Does anyone actually specify the server (or account) list?
> No. For potential future use, see comment 30. Should I remove the
> functionality?
Perhaps a better idea would be to provide an optional filter function, that way you could filter the list before sorting it?

> Sorry, I just copied all this, I can't tell what is good code and what not.
> The thing with servers was recommended to me in comment 32.
> I can't make this better myself without suggestions.
Accidental submit FTL :-( This never happened over http...

> Sorry, I just copied all this, I can't tell what is good code and what not.
> The thing with servers was recommended to me in comment 32.
Fair enough, but my personal preference would have been for the original code.
Does everyone agree with the suggestion to fold the server and account sorting functions into only one version?
Blocks: 763236
Does an account always have a server attached so that sorting accounts can be mapped onto sorting servers?
No, e.g. feeds don't have a server.
(In reply to Magnus Melin from comment #46)
> No, e.g. feeds don't have a server.
Yes they do, the have an nsRSSIncomingServer.

Even local folders have an nsNoIncomingServer.

But invalid accounts might not have a server.
Ok, I could sort accounts without servers to the end of the list.
Attached patch patch v4 (obsolete) — Splinter Review
Managed to get rid of the global variables and still keep both the functions. Just in case they diverge more in the future. They already differ in IM accounts. Yes that can be open-coded at the callers, but the point of this bug is to merge and hide this duplication and eliminate unneeded open-coding.

I see the current allAccountsSorted already strips accounts without servers away so there is not much distinction between accounts and servers (except IM). But let's have it more future-proof (so other divergence is easy to add). The new precedence function should handle accounts without servers, but maybe we never want to show those dummy accounts so I left the stripping in as it was.
Attachment #622456 - Attachment is obsolete: true
Attachment #622456 - Flags: review?(neil)
Attachment #635002 - Flags: review?(neil)
It seems sorting via servers vs. sorting via account may produce different ordering (see bug 763236). So if the folderWidgets.xml case can be changed back to accounts as Neil suggests in comment 37, maybe I should really get rid of the servers version to avoid the confusion. Or just reduce it to 
servers = accountsSorted.map(function(a) a.incomingServer).
No longer blocks: 763236
Depends on: 763236
Comment on attachment 635002 [details] [diff] [review]
patch v4

This will need an updated patch depending on how bug 763236 pans out.
Attachment #635002 - Flags: review?(neil)
Attached patch Centralise server order (obsolete) — Splinter Review
This adds a method to the account manager that returns the sort order for a server. The sort order is a number between 100000000 and 900000000 so that RDF can use it as a string. (I added a few extra zeros in case someone churned through 1000 accounts.) It relies on each server type knowing its own sort order, so I may have missed one, since I only checked mailnews.
Attachment #639856 - Flags: feedback?(mbanner)
Neil, I assume I should use your new function instead of my Javascript GetServerPrecendence function? But I think you wanted to use some sortKeys instead of these server-specific precedence/order numbers. Has something changed?

Now that bug 763236 landed I can update the patch here ot any have one version to sort accounts (as sorting servers in not needed anywhere).
(In reply to aceman from comment #53)
> Neil, I assume I should use your new function instead of my Javascript
> GetServerPrecendence function?
Well, assuming the approach gets positive feedback ;-)

> But I think you wanted to use some sortKeys instead of these
> server-specific precedence/order numbers. Has something changed?
The sort order numbers are unique to each server, so no secondary key is required, unlike when sorting mail folders, where mundane folders needs to be sorted by name. (News folders don't need to be sorted by name either, but unfortunately we still use the secondary key...)
Blocks: 244347
I am not sure when IM is going to be enabled by default in a release but we should get this ordering straight, before some users are sensitive to it (see e.g. bug 763236).

Neil, I do not see the 300000000 value in your patch, for IM servers. Is it possible to add it?

(For those curious, Neil's patch has all the values shifted by 1 relative to comment 7, so the default account is "0", etc.)
(In reply to aceman from comment #55)
> Neil, I do not see the 300000000 value in your patch, for IM servers. Is it
> possible to add it?
Sure, do you know where it goes?
Maybe in mail/components/im/imIncomingServer.js. Florian would know.
You can probably add it as sortOrder: 300000000, or get sortOrder() 300000000,
Attached patch Centralise server order v2 (obsolete) — Splinter Review
Yes, that seems to work. This new patch just adds that to the IM server definition.
Attachment #639856 - Attachment is obsolete: true
Attachment #639856 - Flags: feedback?(mbanner)
Attachment #644780 - Flags: superreview?(mbanner)
Attachment #644780 - Flags: review?(mozilla)
Attachment #644780 - Flags: feedback?(florian)
Attached patch patch v5 (obsolete) — Splinter Review
Apply on top of the "Centralise server order v2" patch.

This patch does:
1. remove the allServersSorted function, it is not needed now. We always want to sort by accounts (to keep the relative positions inside server-type groups) and if anybody wants the servers out of them, he can exctract them afterwards. But no such occurrence was found yet. Users of .allServers do not care for ordering.
2. the parameter to allAccountsSorted is now a boolean to indicate if we want to exclude IM accounts. The default is leave IM accounts in. If other needs arise in the future the argument can be reworked.
Attachment #635002 - Attachment is obsolete: true
Attachment #644782 - Flags: review?(neil)
Comment on attachment 644780 [details] [diff] [review]
Centralise server order v2

I've nothing against adding a sortOrder getter in imIncomingServer.js, but I think it would be useful for people adding new account types in the future to have a comment in nsIMsgIncomingServer.idl explaining which values the sortOrder attribute should have. There's some explanation here in comment 52, but I think it would be nice to have it in the code too.
Attachment #644780 - Flags: feedback?(florian) → feedback+
Attached patch Centralise server order v3 (obsolete) — Splinter Review
Good idea florian.
Attachment #644780 - Attachment is obsolete: true
Attachment #644780 - Flags: superreview?(mbanner)
Attachment #644780 - Flags: review?(mozilla)
Attachment #645419 - Flags: review?(mbanner)
It is sad to see this miss another TB version again...
Neil, standard8 ?
Comment on attachment 645419 [details] [diff] [review]
Centralise server order v3

Review of attachment 645419 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +3788,5 @@
> +  if (aServer != defaultServer) {
> +    findServerByKeyEntry findEntry;
> +    aServer->GetKey(findEntry.key);
> +    aServer->GetSortOrder(&findEntry.index);
> +    m_accounts->EnumerateForwards(findServerIndexByServer, (void *)&findEntry);

I'm not sure this is going to do quite what you expect. The GetSortOrder call will set the initial value of index, e.g. 100000000, you'll then get added on to that the position it is stored in the m_accounts array e.g. 100000002.

If that's what you're expecting, then I guess this is fine, however, that then means this is taking account of the position in the array, but when I look at the other patch, it only takes into account the sort order and not the account position and/or name.
I am not sure if Neil intended that but I think it would not be bad.

The intended sort is like this:
1. you have account1(pop3),account2(RSS),account3(IMAP).
2. if getsortorder() returns 100000000 for POP3 and IMAP, and 400000000 for RSS, then the resulting order after sorting the values becomes account1,account3,account2 UNDER THE ASSUMPTION OF A STABLE SORT IN JS array.sort(). The docs say JS sort is stable, but Neil in comment 42 is not sure.

So if instead Neil implemented it that account 1 gets 100000001, account2 400000001 and account3 100000002, then the result also becomes account1,account3,account2 but it is guaranteed even WITHOUT STABLE SORT. So if we accept nobody will have 100000000 accounts, this solution would be quite nice (except doc-dev-needed that the returned values are not the precise base value that is visible in code under *Server::GetSortOrder).

But I will run with the patch to see if the numbers really are what you say.

Neil?
I believe that JS doesn't guarantee that sort() is stable. I'm not 100% sure which sort of sort RDF uses but I wouldn't be surprised if it wasn't quick sort, which isn't stable. The old RDF code fetches the account index and adds on either 1000, 2000 (local) or 3000 (news) to generate the sort key.
The docs I was referring to is this: https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Array/sort which claims Gecko CURRENTLY has a stable sort. I agree that if it costs us nothing then we should make our code so that is does not rely on that.

Neil, so you confirm your patch intentionally produces sortorders like 100000001, 100000002, ... ?

If yes, I like that solution as reasoned in comment 66. Standard8, it looks like you can continue with review, thanks.
Comment on attachment 644782 [details] [diff] [review]
patch v5

I have some tweaks to do here.
Attachment #644782 - Attachment is obsolete: true
Attachment #644782 - Flags: review?(neil)
After the review I'll put a comment into nsMsgAccountManager::GetSortOrder to note what the values returned really are.
Comment on attachment 645419 [details] [diff] [review]
Centralise server order v3

Review of attachment 645419 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I'd missed that it was effectively maintaining what was already happening.
Attachment #645419 - Flags: review?(mbanner) → review+
It is maintaining what was in mailnews/base/src/nsMsgAccountManagerDS.cpp, the other JS places were different (without the +index part). Now we want to consolidate all the places to use the same code.

Thanks for the review.
(In reply to Mark Banner (:standard8) from comment #65)
> If that's what you're expecting, then I guess this is fine, however, that
> then means this is taking account of the position in the array, but when I
> look at the other patch, it only takes into account the sort order and not
> the account position and/or name.

Yes, the other patch was not using am.getSortOrder(server) (but server.sortOrder directly) because I didn't notice that Neil is doing the +index stuff and that am.getSortOrder() is the better function to use. I update the second patch now.
I played with Neil's patch a while and I am now not sure what number the m_accounts->EnumerateForwards(findServerIndexByServer, (void *)&findEntry);
returns. Is it the index in the server list? How is the server list determined?
It probably isn't the order from the mail.accountmanager.accounts pref. Neil?
OK, it looks it is order by accounts and that is what we want.
I updated the patch a bit with there changes:
- make AM::getSortOrder work when there is no default account (which may be enabled in the near future)
- make it call GetDefaultAccount() as in the original the m_defaultAccount was not defined at first invocation of the function.
- use FindServerIndex instead of open-coding the logic and make the returned value of sortOrder + serverIndex much more obvious (I missed it too until standard8 pointed it out). This version produces number lower by 1 from the original patch but I don't think that really matters.
- convert PRint32 to int32_t
- fix compiler warning about unused 'rv' in FindServerIndex.
- unbitrot

Neil, can you please look if you agree with my changes?
Attachment #645419 - Attachment is obsolete: true
Attachment #664624 - Flags: review?(mbanner)
Attachment #664624 - Flags: feedback?(neil)
Attached patch patch v6 (obsolete) — Splinter Review
For those interested, this is the accompanying patch that uses the getSortOrder for the sorting of the accounts in the UI. I'll request a review once the 'Centralise server order' patch is accepted.
Comment on attachment 664625 [details] [diff] [review]
patch v6

>+function getServerSortOrder(aServer) {
>+  // If there is no server sort this object to the end.
>+  if (!aServer)
>+    return 999999999;
[When would this happen?]
It should not happen for now, because we strip those accounts away (see the "This is a HACK to work around bug 41133" block). But I wanted it to be robust.

We could also make nsMsgAccountManager::GetSortOrder take a null server and return 9999999999. So you can decide where to put this check, or just remove it altogether.
Attachment #664624 - Flags: feedback?(neil) → feedback+
standard8: ping?
Attachment #664624 - Flags: review?(mbanner) → review+
Attachment #664625 - Flags: review?(mconley)
Attached patch patch v7 (obsolete) — Splinter Review
Unbitroted patch.
Attachment #664625 - Attachment is obsolete: true
Attachment #664625 - Flags: review?(mconley)
Attachment #674000 - Flags: review?(mconley)
Comment on attachment 674000 [details] [diff] [review]
patch v7

Review of attachment 674000 [details] [diff] [review]:
-----------------------------------------------------------------

aceman:

This looks pretty good - I'm glad to see a bunch of duplicated code going away! Just curious where getSortOrder comes from - see below.

-Mike

::: mailnews/base/util/folderUtils.jsm
@@ +125,5 @@
> +  if (!aServer)
> +    return 999999999;
> +
> +  // Otherwise get the server sort order from the Account manager.
> +  return MailServices.accounts.getSortOrder(aServer);

Maybe I've missed something here, but I'm not seeing a getSortOrder function inside the nsIMsgAccountManager IDL...

http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgAccountManager.idl
mconley, in the other patch named 'Centralise server order v4'! :)
(In reply to :aceman from comment #83)
> mconley, in the other patch named 'Centralise server order v4'! :)

Ah, thanks
aceman:

This patch looks good to me, but I'd like to give it a spin. Can you de-bitrot it for me?

-Mike
Again? I de-bitrotted it 2 days ago :) I can't upload a new patch for the next 3 hours, but if you paste here which code does not apply, maybe I can post you the correct code.
Hm - so, I forgot to update my tree. Nevermind - it applies now.
Great. Yes definitely try to run with the patch, it is a highly visible change so must be tested. This reorders accounts in the Account manager, folder pane, compose window (identities list), 'Move to' item in msg context menu (folder picker), etc. Check it all out:)
Comment on attachment 674000 [details] [diff] [review]
patch v7

I've tinkered with it, and I can't see anything wrong. Thanks aceman!
Attachment #674000 - Flags: review?(mconley) → review+
Thanks to all! Let's try to push it ;)
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/0e156c9327d2
https://hg.mozilla.org/comm-central/rev/eb8f507508d7
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 19.0
This was backed out for breaking mozmill tests.

https://hg.mozilla.org/comm-central/rev/de0531591b52
https://hg.mozilla.org/comm-central/rev/76cb1dc61a14

https://tbpl.mozilla.org/php/getParsedLog.php?id=16447830&tree=Thunderbird-Trunk

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/account/test-archive-options.js | test-archive-options.js::test_archive_options_enabled
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/account/test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_mail_account_setup
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/mozmill/account/test-mail-account-setup-wizard.js | test-mail-account-setup-wizard.js::test_bad_password_uses_old_settings
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Is it confirmed that this broke those tests?
> TEST-UNEXPECTED-FAIL |
> /home/cltbld/talos-slave/test/build/mozmill/account/test-archive-options.js
> | test-archive-options.js::test_archive_options_enabled

It definitely broke this test, the other two were random orange.
I can confirm that too.
And sorry for not catching that in my review. I should have known that this would affect tests.
Actually it is not expected for this to affect tests as it is just code shuffle. Only if the test is depending on a specific order of the accounts in the folder picker. But the "archive options" test may be doing just that, there are pickers on that AM page.
I'll check it out of course ;)
Without running the test yet, I suspect that this patch broke this assumption of the test:
// XXX: This is pretty brittle, and assumes 1) that there are 8 items in each
// account's tree, and 2) that the order of the accounts is as we expect.
// The + 1 when index != 0 is for the line used by the IRC account,
// which is at the second position.

It is quite possible the IRC account got moved to the bottom, according to the new order. This should be easy to fix, I have a better account selection coded in test-account-actions.js.
Attached patch patch v8 (fixes test) (obsolete) — Splinter Review
The suspicion was correct. This updated patch contains fix for the test.
Attachment #674000 - Attachment is obsolete: true
Attachment #675628 - Flags: review?(mconley)
Blocks: 805953
Fix bitrot after bug 804008.
Attachment #675628 - Attachment is obsolete: true
Attachment #675628 - Flags: review?(mconley)
Attachment #675992 - Flags: review?(mconley)
Comment on attachment 675992 [details] [diff] [review]
patch v9 (fixes test)

Review of attachment 675992 [details] [diff] [review]:
-----------------------------------------------------------------

I like it! Great work, aceman.
Attachment #675992 - Flags: review?(mconley) → review+
Another push :)
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/9781fec9c16e
https://hg.mozilla.org/comm-central/rev/8d3c052f2e14
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Flags: in-testsuite- → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
For addon author, if you rely on the sort order of the accounts, use the new 
allAccountsSorted exclusively. That is the order shown in the folder pane and account manager and folder pickers. Beware that accountManager.allServers may return the servers (and related accounts) in a different order so do not use it if order of enumeration is important.
Keywords: addon-compat
Just wanted to mention that this 'fix' caught me somewhat by surprise. And an unpleasant surprise at that. I understand cleaning up the code base, but from a user perspective it is annoying for two reasons:

a) The order of accounts in the 'Move To' menu has changed. This is problematic, as I do a lot of moving messages to local folders and use the cursor keys to navigate the menu quite quickly. I have gotten very used to the sequence of accounts over the years so that I tend not to look at the menu any more until I get to the folder or sub-folder level. The change in order means that I regularly move things to the wrong place and---even worse---accidentally invoke other commands (e.g., ignoring the current thread in my inbox when I thought I was moving to a folder starting with 'K').
b) My accounts are now somewhere different in my folder pane, making me search for them for longer.

It seems that this could really have done with a little bit of user testing. Are there any plans of exposing the sort order to plugins so that users can control the order of accounts themselves again?
Can you tell us what is type of accounts that have moved?
It is also possible, that the order you were used to previously was not enforced consistently in all lists inside TB. That may have been OK with you, but other users have been annoyed by that.

Now in case a) and b) the order should be the same. Is it not the case?

The sort order can be changed by addons (and it always has been), but only among the account type groups (you can change IMAP1, IMAP2, NEWS1, NEWS2 to IMAP2, IMAP1, NEWS2, NEWS1, but not IMAP1, NEWS1, IMAP2, NEWS2). Also, if addons create new account types, they can choose were to put them among the account types order.
The order is indeed consistent throughout. However, NEWS used to be before Local Folders and has now moved after Local Folders.
OK, and you do not use Local folders so it is just in the way for you?
No, I do use Local folders a lot. 

The problem is that I move messages into Local Folders frequently. To do so, I used to open the Move Message context menu, do a Cursor Up, Cursor Right to get into the Local Folders menu and then use first character shortcuts to navigate to the appropriate folder. 

Doing this now, gets me into the News account, which probably doesn't have a folder of the appropriate name (annoying) or does, but only at the first level (really annoying). In the former case, TB just chomps my shortcuts and I can notice and rectify. In the latter case, my message gets moved into the News account (by the first shortcut I used) and the other shortcuts are interpreted at the top level---for example, switching the next thread to 'Ignore' if I was trying to go to a folder starting with 'K'.
So you need to change the first "Cursor Up" do "Cursor Down" or something, and the rest will work as before?
Yes, in principle that would work. However, as I indicated in my original posting, I have grown very used to the key sequence and now perform it without thinking about it. While this is a very quick way of moving messages, it also means that it is very cumbersome for me to adjust to the new ordering of accounts.

I'm sure I am not the only one who is annoyed by this sudden change in behaviour. This is why, in my original post, I said that this change could have benefited from some amount of user study.
Just noticed that issue 244347 seems to discuss a potential solution for my problem: If I were able to reorder accounts through the TB UI, I could set it up exactly as I expect it to be. My problem is not in the added consistency in the TB code, but in the sudden change without an option for me to affect this.
User study? It's called beta testing, and this one landed already for tb19 (beta) - we do want more beta testers so you're certainly welcome to join with that. I don't think anyone else complained so far. Beta testing and filing bugs is *the* way to affect things.
Good point. Unfortunately, the problem occurs where I use TB in a production scenario. I'm not happy to use a Beta version in this scenario.
Blocks: 1002240
Blocks: 1359410
You need to log in before you can comment on or make changes to this bug.