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

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[:aceman]

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

Comment 1

[Florian Quèze [:florian]]

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.

Comment 2

[:aceman]

Good catch.
So what is the proposal? (5) RSS (6) IM ? Or something better?

Comment 3

[Florian Quèze [:florian]]

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.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

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.  ;)

Comment 5

[:aceman]

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

Comment 6

[Florian Quèze [:florian]]

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

Comment 7

[Blake Winton (:bwinton) (:☕️)]

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

Comment 8

[:aceman]

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

Comment 9

[:aceman]

Attachment: proof of concept

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.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 11

[:aceman]

Thanks. Who would be good to review something like this?

Comment 12

[Blake Winton (:bwinton) (:☕️)]

The Magic 8-ball says…

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

Potential super-reviewers:
  bienvenu: 11
  neil: 6
  standard8: 3

Comment 13

[:aceman]

And what do those numbers mean?

Comment 14

[Blake Winton (:bwinton) (:☕️)]

They're the number of times they've reviewed code in files like the ones you've changed.  (Or, basically, they're meaningless.  ;)

Comment 15

[:aceman]

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 16

[Florian Quèze [:florian]]

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.

Comment 17

[:aceman]

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.

Comment 18

[Florian Quèze [:florian]]

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

Comment 19

[:aceman]

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.

Comment 20

[Ian Neal]

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

Comment 21

[:aceman]

Some more (function has different name):
http://mxr.mozilla.org/comm-central/source/mail/base/content/msgViewNavigation.js#173
http://mxr.mozilla.org/comm-central/source/suite/mailnews/msgViewNavigation.js#175

Comment 22

[:aceman]

Good catches Ian! Looks like this will be worth something.

Comment 23

[:aceman]

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));
}

Comment 24

[Blake Winton (:bwinton) (:☕️)]

aceman: I think Florian meant something like the Schwartzian Transform - https://en.wikipedia.org/wiki/Schwartzian_transform  :)

Later,
Blake.

Comment 25

[:aceman]

Attachment: temporary patch

This patch just shows the current state. It is not final and complete, just to show SM people where I am standing now.

Comment 26

[Florian Quèze [:florian]]

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

Comment 27

[:aceman]

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? :)

Comment 28

[[e-mail]]

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?

Comment 29

[:aceman]

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?

Comment 30

[:aceman]

Attachment: real patch

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.

Comment 31

[Blake Winton (: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.

Comment 32

[Ian Neal]

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

Comment 33

[:aceman]

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

Comment 34

[:aceman]

Attachment: patch v2

Thanks, fixed.

Comment 35

[Ian Neal]

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.

Comment 36

[:aceman]

Attachment: patch v3

Comment 37

[[e-mail]]

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 ;-)

Comment 38

[:aceman]

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

Comment 39

[Florian Quèze [:florian]]

(In reply to :aceman from comment #38)
> (In reply to [e-mail] 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.

Comment 40

[:aceman]

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 .

Comment 41

[Florian Quèze [:florian]]

(In reply to :aceman from comment #40)
> Would this suffice?

I think it would, yes.

Comment 42

[[e-mail]]

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

Comment 43

[[e-mail]]

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.

Comment 44

[:aceman]

Does everyone agree with the suggestion to fold the server and account sorting functions into only one version?

Comment 45

[:aceman]

Does an account always have a server attached so that sorting accounts can be mapped onto sorting servers?

Comment 46

[Magnus Melin [:mkmelin]]

No, e.g. feeds don't have a server.

Comment 47

[[e-mail]]

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

Comment 48

[:aceman]

Ok, I could sort accounts without servers to the end of the list.

Comment 49

[:aceman]

Attachment: patch v4

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.

Comment 50

[:aceman]

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

Comment 51

[:aceman]

Comment on attachment 635002 [details] [diff] [review]
patch v4

This will need an updated patch depending on how bug 763236 pans out.

Comment 52

[[e-mail]]

Attachment: Centralise server order

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.

Comment 53

[:aceman]

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

Comment 54

[[e-mail]]

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

Comment 55

[:aceman]

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

Comment 56

[[e-mail]]

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

Comment 57

[:aceman]

Maybe in mail/components/im/imIncomingServer.js. Florian would know.

Comment 58

[[e-mail]]

You can probably add it as sortOrder: 300000000, or get sortOrder() 300000000,

Comment 59

[:aceman]

Attachment: Centralise server order v2

Yes, that seems to work. This new patch just adds that to the IM server definition.

Comment 60

[:aceman]

Attachment: patch v5

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.

Comment 61

[Florian Quèze [:florian]]

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.

Comment 62

[:aceman]

Attachment: Centralise server order v3

Good idea florian.

Comment 63

[:aceman]

It is sad to see this miss another TB version again...

Comment 64

[:aceman]

Neil, standard8 ?

Comment 65

[Mark Banner (: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.

Comment 66

[:aceman]

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?

Comment 67

[[e-mail]]

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.

Comment 68

[:aceman]

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 69

[:aceman]

Comment on attachment 644782 [details] [diff] [review]
patch v5

I have some tweaks to do here.

Comment 70

[:aceman]

After the review I'll put a comment into nsMsgAccountManager::GetSortOrder to note what the values returned really are.

Comment 71

[Mark Banner (:standard8)]

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.

Comment 72

[:aceman]

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.

Comment 73

[:aceman]

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

Comment 74

[:aceman]

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?

Comment 75

[:aceman]

OK, it looks it is order by accounts and that is what we want.

Comment 76

[:aceman]

Attachment: Centralise server order v4

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?

Comment 77

[:aceman]

Attachment: patch v6

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 78

[[e-mail]]

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?]

Comment 79

[:aceman]

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.

Comment 80

[:aceman]

standard8: ping?

Comment 81

[:aceman]

Attachment: patch v7

Unbitroted patch.

Comment 82

[Mike Conley (: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

Comment 83

[:aceman]

mconley, in the other patch named 'Centralise server order v4'! :)

Comment 84

[Mike Conley (:mconley) (:⚙️)]

(In reply to :aceman from comment #83)
> mconley, in the other patch named 'Centralise server order v4'! :)

Ah, thanks

Comment 85

[Mike Conley (:mconley) (:⚙️)]

aceman:

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

-Mike

Comment 86

[:aceman]

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.

Comment 87

[Mike Conley (:mconley) (:⚙️)]

Hm - so, I forgot to update my tree. Nevermind - it applies now.

Comment 88

[:aceman]

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 89

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 674000 [details] [diff] [review]
patch v7

I've tinkered with it, and I can't see anything wrong. Thanks aceman!

Comment 90

[:aceman]

Thanks to all! Let's try to push it ;)

Comment 91

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/0e156c9327d2
https://hg.mozilla.org/comm-central/rev/eb8f507508d7

Comment 92

[Ryan VanderMeulen [:RyanVM]]

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

Comment 93

[:aceman]

Is it confirmed that this broke those tests?

Comment 94

[Mark Banner (:standard8)]

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

Comment 95

[Mike Conley (:mconley) (:⚙️)]

I can confirm that too.

Comment 96

[Mike Conley (:mconley) (:⚙️)]

And sorry for not catching that in my review. I should have known that this would affect tests.

Comment 97

[:aceman]

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.

Comment 98

[:aceman]

I'll check it out of course ;)

Comment 99

[:aceman]

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.

Comment 100

[:aceman]

Attachment: patch v8 (fixes test)

The suspicion was correct. This updated patch contains fix for the test.

Comment 101

[:aceman]

Attachment: patch v9 (fixes test)

Fix bitrot after bug 804008.

Comment 102

[Mike Conley (: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.

Comment 103

[:aceman]

Another push :)

Comment 104

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/9781fec9c16e
https://hg.mozilla.org/comm-central/rev/8d3c052f2e14

Comment 105

[:aceman]

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.

Comment 106

[steffen.zschaler]

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?

Comment 107

[:aceman]

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.

Comment 108

[steffen.zschaler]

The order is indeed consistent throughout. However, NEWS used to be before Local Folders and has now moved after Local Folders.

Comment 109

[:aceman]

OK, and you do not use Local folders so it is just in the way for you?

Comment 110

[steffen.zschaler]

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

Comment 111

[:aceman]

So you need to change the first "Cursor Up" do "Cursor Down" or something, and the rest will work as before?

Comment 112

[steffen.zschaler]

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.

Comment 113

[steffen.zschaler]

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.

Comment 114

[Magnus Melin [:mkmelin]]

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.

Comment 115

[steffen.zschaler]

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.