389139 - Thunderbird should NOT show the source account under "Inbox for different account" in the advanced settings.

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Saïvann Carignan]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.5) Gecko/20070720 Ubuntu/7.10 (gutsy) Firefox/2.0.0.5
Build Identifier: Mozilla Thunderbird 2.0.0.4

This bug happens in the Advanced settings inside the Account settings in thunderbird.

When configuring the inbox folders of a account through the Account settings in Thunderbird, it's possible to choose the actual account as "inbox for different account". This is clearly an error because the actual account should not be listed in the "inbox for different account" list.

If I have an account named George and that I change the inbox folder of that account to "Inbox for different account" and that I choose the George account has the account, Thunderbird will crash on the program reboot without any Talkback ID. It freezes on Linux and it crashes on Windows.

The George account should never be shown on the list because it's already that account that we are configuring.

This bug happens on Thunderbird 2.0.0.4 and Thunderbird 3 alpha, but only Thunderbird 2.0.0.4 will crash. Thunderbird 3 will works bad with the configuration but not crash.

Reproducible: Always

Steps to Reproduce:
1. Create a new account in Thunderbird
2. Go to "Account settings"
3. Under "server settings", click on "Advanced settings"
4. Change the inbox for "Inbox for different account" and choose the actual account as the "different account" ( Not Local folder or any other ).
5. Byebye, thunderbird won't start again.
Actual Results:  
The configured account will disappear from the Inbox ( not in account settings, just into Thunderbird ) and Thunderbird will crashes on next program start without any TalkID. 

Expected Results:  
It should never have show the actual Inbox in the list because it's logic. The end user can make a mistake and do a bad configure, leading to a hard crash.

Comment 2

[WADA:World Anti-bad-Duping Agency]

Crash/freeze of Tb 2.0 after this bug is Bug 388892.
Changing severity of this bug to minor.

Comment 3

[WADA:World Anti-bad-Duping Agency]

Confirming, according to test result in Bug 388892 Comment #1.

Comment 4

[Saïvann Carignan]

Because this bug will lead to permanently lose access to an entire profile and all email inside it, this bug should not be marked as critical?

People who experience this bug will lose all their data.

Comment 5

[Saïvann Carignan]

Ok, the critical bug is Bug_388892.

Comment 6

[:aceman]

So was the crash fixed?

And does it now work if you select the same account? If not, then it should be prevented.
I am not able to code this in the folderpicker (pass it a server it should not show in the list) but I could do it in the account manager.
But it needs UI decision on what should happen. The account would still be in the list in the picker and the user could select it. What then:
1. A warning that that is not an allowed choice?
2. Or automatically revert the choice to the previous value?
3. Or automatically select the radio button "Inbox for this server's account" instead of the currently selected "Inbox of a different account"?

Notice this same approach should then be used in the Copies & Folders pane which has the same pickers and problems.
Bwinton?

Comment 7

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

Aceman?

;)

I think we should not show the account in the list, or show it greyed out, and not let the user select it.  Since the folderpicker is just more code, why can't we fix it there?

Thanks,
Blake.

Comment 8

[:aceman]

Because I have not yet found the code of the folder picker implementation. Can you point me to the file? :)

Comment 9

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

http://mxr.mozilla.org/comm-central/source/mailnews/base/content/folderWidgets.xml#46  :)

Later,
Blake.

Comment 10

[:aceman]

Thanks!
How I hate these xml definitions. I have no idea how they really work. But maybe I could just tap into the _filter infrastructure there.

Comment 11

[:aceman]

Attachment: patch

This patch is working for me.
As I couldn't find a way to automatically find the current account from inside the filter_defered function, I added a new generic argument to exclude servers from the list (that applies before the existing filter modes). It accepts a comma separated list of server keys.

The current problem:
I couldn't find a way to set the argument before the folderpicker is built for the first time. So after it is ready, I just tear it down, set the argument and rebuild again. Any ideas?

While I am in the folderWidgets.xml file, the patch also includes a spelling fix for the defered mode of filter.
And also fixes a strict warning for undefined node.id (probably some folder picker element in TB does not have an id).

What do you think?

I was wrong this would be needed in the Copies&Folders settings. Even in the "other" case, the current account is needed to be able to select a different folder (other than Drafts, Sent, etc).

Are there any other places where this new exclusiong feature would be useful?

Comment 12

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

Comment on attachment 619385 [details] [diff] [review]
patch

This didn't seem to fix the bug for me, so ui-r-.

(The account still showed up in the list of accounts.)

So, given that, and how much of a pain this is to test manually, I really think we need an automated test for it.  But I'm sure mconley will help you write one, if you need any assistance.  ;)

Thanks,
Blake.

Comment 13

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

Comment on attachment 619385 [details] [diff] [review]
patch

Huh.  Uh, re-testing, as requested, and I can't seem to reproduce the error I was seeing.  So, I guess this is kinda ui-r=me, but I agree that we should remove "Local Folders" since that's covered under a different option, and if there's nothing left, we should probably disable the option entirely, so I guess it's still kinda ui-r-.  ;)

Thanks, and I'm sorry about that.
Blake.

Comment 14

[:aceman]

Attachment: patch v2

Addressed the improvements from comment 13. But I had to add a .numberOfItems property to the picker so that it can tell if it is empty. And also I had to extend .selectFolder(null) so that it just selects anything (the first menuitem it has). I think we do not care which account the picker shows initially (for accounts not yet deferred). This fixed a kind of 'TODO' comment in the am-server-advanced.js .

Try with several pop3 servers set up, and also with only 1. The last option should be disabled completely in the latter case.

Comment 15

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

Comment on attachment 620074 [details] [diff] [review]
patch v2

I like it, with one request.

The empty list (as seen in http://dl.dropbox.com/u/2301433/Screenshots/EmptyList.png ) is a little small.  Could we add a minwidth, so that it keeps _mostly_ the same width?

ui-r=me with that fixed.

Thanks,
Blake.

Comment 16

[:aceman]

Yes it is ugly.
Same width as what?

Comment 17

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

As when there will be other servers.  (I think you can just pick a reasonable width for it, and I promise not to complain too much.)

Comment 18

[:aceman]

Should that be set in some global css file/theme or can the picker set it itself internally?

Comment 19

[:aceman]

Attachment: patch v3

Done, in the theme.

Comment 20

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

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

I like it!  ui-r=me!

Thanks,
Blake.

Comment 21

[Ian Neal]

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

I defer to Neil on this one.

Comment 22

[[e-mail]]

Well, I only briefly skimmed the patch, but the correct way to remove Local Folders from that particular menulist is to change the way "defered" (sic) works in folderWidgets.xml; meanwhile to prevent selection of the current account my preference would be to disable the appropriate menuitem, although I don't know whether bwinton would be happy with that.

Comment 23

[:aceman]

(In reply to [e-mail] from comment #22)
> Well, I only briefly skimmed the patch, but the correct way to remove Local
> Folders from that particular menulist is to change the way "defered" (sic)
> works in folderWidgets.xml;
And what would that be?

Comment 24

[[e-mail]]

Sorry for taking so long to get back to this.

(In reply to aceman from comment #23)
> (In reply to comment #22)
> > Well, I only briefly skimmed the patch, but the correct way to remove Local
> > Folders from that particular menulist is to change the way "defered" (sic)
> > works in folderWidgets.xml;
> And what would that be?
Adding "aFolder.server.type != none &&" to line 109 should do the trick.

Comment 25

[:aceman]

(In reply to [e-mail] from comment #24)
> > > Well, I only briefly skimmed the patch, but the correct way to remove Local
> > > Folders from that particular menulist is to change the way "defered" (sic)
> > > works in folderWidgets.xml;
> Adding "aFolder.server.type != none &&" to line 109 should do the trick.

Ok, that could work. But only in the assumption that we never want the Local Folders there, that the mode="deferred" is only used from the Account manager subdialog where Local Folders is handled separately. If we incidentally call the picker with mode="deferred" from any other place, it will be bad it does not contain Local Folders, is it is a valid target for deferring.

I'll try making the unwanted menu item (target account disabled). But that would make the possibility of empty picker menu moot. That may be a good thing :)

Comment 26

[:aceman]

Attachment: patch v4

Ok, this new version addresses what Neil wants. Local folders is removed, but the current account is just disabled.
If all accounts that can be target of deferring (POP3/RSS) are deferred to Local Folders, the picker still has no items (Local folders away, deferred accounts are not listed too) so I have to keep the min-width.

While playing with the various combinations I have found one new undesirable behaviour. It was possible to defer accountA to accountB and then accountB to accountC (A,B,C all different). In that case neither accountA nor accountB is shown in the folder pane. But accountB still receives accountA's mail but the user can't access it. I discussed this with Bienvenu and jcranmer that this probably should be disallowed too. This patch contains the fix for this: if an account already has some other account deferred to it (in the example accountB determines this), then it can't be defered further to not allow such chaining.

Bwinton, in the case described above, all the options for deferring will be disabled. Only the "this account's Inbox" stays as only option and will be selected. Do you want any more UI for this? Should there be a label telling that this account can't be deferred?

Comment 27

[David :Bienvenu]

these ifs can be combined and the braces removed:

+
+            if (aExcludeServers.length > 0) {
+              if (folder.isServer &&
+                  aExcludeServers.indexOf(folder.server.key) != -1)
+                node.setAttribute("disabled", "true");
+            }

why would aFolder be undefined?

these ifs can also be combined:

+    if (!deferToGlobalSetUp) {
+      // If even Local Folders does not exist, disable the second option.
+      if (!localFoldersAccount)
+        document.getElementById("globalInbox").disabled = true;
     }

this patch has bit-rotted a bit...
+            if ((aFolder == undefined) || (child._folder.URI == aFolder.URI) ||

Comment 28

[:aceman]

(In reply to David :Bienvenu from comment #27)
> +            if (aExcludeServers.length > 0) {
> +              if (folder.isServer &&
> +                  aExcludeServers.indexOf(folder.server.key) != -1)
> +                node.setAttribute("disabled", "true");
> +            }
> 
> why would aFolder be undefined?
> 
I see no aFolder in this code. But if you ask about the selectFolder() implementation then yes, calling selectFolder() without argument is a new feature and causes aFolder to be undefined. It means "just select the first usable item in the list, whatever it is". I use that in the am-server-advanced dialog.

Comment 29

[David :Bienvenu]

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

I think this wants to use /// single line comment style.
+  /* does any other server have deferred storage to this account ? */

would be good to use MailServices.accounts here:

-    gAccountManager = Components.classes["@mozilla.org/messenger/account-manager;1"].getService(Components.interfaces.nsIMsgAccountManager);
+    let radioGroup = document.getElementById("folderStorage");
+
+    gAccountManager = Components.classes["@mozilla.org/messenger/account-manager;1"]
+                                .getService(Components.interfaces.nsIMsgAccountManager);

Since the patch didn't apply, I didn't run with it, but feedback+ for the general idea...

Comment 30

[:aceman]

Attachment: patch v5

Bwinton, see comment 26 on what you should review.

David, I could not see any bitrot...

Comment 31

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

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

(I haven't run with this patch, I'm just basing this review on the comment.)

> Bwinton, in the case described above, all the options for deferring will be
> disabled. Only the "this account's Inbox" stays as only option and will be
> selected. Do you want any more UI for this?

No, I think that'll be okay.

> Should there be a label telling that this account can't be deferred?

I don't think a label is really necessary.  If you wanted to add a tooltip, that would be fine.  If you didn't, that would also be fine.  ;)  (And I reserve the right to change my mind if a lot of people complain to support about this.  :)

Thanks,
Blake.

Comment 32

[:aceman]

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

Thanks. I have no idea where to put a popup now. If there is a need to add an explanation later I have no problem to fix that bugreport :)

Comment 33

[:aceman]

Attachment: patch v6

Now I see bitrot from bug 763236.

Comment 34

[David :Bienvenu]

+  var localFoldersServer = MailServices.accounts.localFoldersServer;
+  return MailServices.accounts.FindAccountForServer(localFoldersServer);

this can just be MailServices.accounts.FindAccountForServer(MailServices.accounts.localFoldersServer);

Comment 35

[[e-mail]]

Comment on attachment 638494 [details] [diff] [review]
patch v6

Unfortunately a side effect of disabling rather than skipping the current account is that the test for an empty menulist no longer works in some cases.

If the account that you edit is initially not deferred then the checkbox is enabled although if you change the selection to not defer the account then the checkbox is disabled.

Comment 36

[[e-mail]]

As a heavy IMAP user, I have to admit to not understanding the problem here well enough. Sorry for messing you around like that.

After trying a test profile with a number of POP accounts both with and without the patch, I think I was wrong when I suggested disabling the menuitem for the current account.

However excluding it should be a simple case of scanning the menulist for the appropriate menuitem and removing it, as it should already be built by the time the dialog loads.

As for Local Folders, it has the convenient effect of ensuring the menulist has at least one item. Otherwise if the menulist has no items then it may be better to hide the radio item completely.

I assume only POP3 accounts should show up in the list? It would probably be a good idea to strengthen the account type check to exclude all other accounts, such as RSS.

Comment 37

[David :Bienvenu]

Comment on attachment 638494 [details] [diff] [review]
patch v6

clearing review request based on neil's review.

Comment 38

[:aceman]

Looks like we are moving in circles here :)

I must think of what Neil actually wants to say now. I did not undertand comment 35 yet. And to comment 36: RSS seems to be a valid target for deferring, it is intentionaly included.

I can implement all the possibilities (hidden current server/Local folders, disabled current server/Local folders, hidden/disabled radio item). Just decide on something already.

Comment 39

[:aceman]

Neil, which checkbox do you mean in comment 35?

Bwinton, you wanted to disabled the third radio option in case the menulist is empty. Can you decide with Neil if it should be disabled or fully hidden?

Comment 40

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

I prefer disabled to hidden, so that users can see what the functionality could be.  Neil?

Comment 41

[:aceman]

Neil, ping?

Comment 42

[:aceman]

Comment on attachment 638494 [details] [diff] [review]
patch v6

So I propose:
1. hide the current account in the folder picker of "other account" radio option
2. disable the Local folders account in the folder picker of "other account" radio option
3. disable the "other account" radio option if it contains no usable/not-disabled items

Comment 43

[:aceman]

That is not what patch v6 already implements, I just wanted to put f? somewhere so that Neil does not miss it.

Comment 44

[[e-mail]]

Comment on attachment 638494 [details] [diff] [review]
patch v6

Sorry for the delay. I've played around with the current patch again to give me a feel for what the options are. I'm OK with hiding the current folder and disabling the local folders menuitems. I was wondering what should happen to already deferred other POP accounts (I'm not sure whether they should be disabled or hidden, but I'm leaning more towards disabled; bwinton?). I wasn't sure whether deferring to an RSS account supported all the functionality that deferring to a POP account or local folders does but if it does then I have no issue with keeping RSS accounts in the list.

Comment 45

[:aceman]

Please move the "deferring to RSS account" to bug 425020. I will not touch it in this bug.

Comment 46

[:aceman]

I am going to produce a new patch.

Comment 47

[:aceman]

It could be nice to show but disable other deferred POP accounts. However the functions to determine which accounts are usable for deferring is this:
aFolder.server.canCreateFoldersOnServer &&               !aFolder.supportsOffline;
Only these servers show up in the list. If we want to preserve that then I am not able to create a condition that returns "servers that would be usable for deferring had they not been deferred themselves".

So I implement what is said in comment 42 and also disable all radios except the first one if the current account is already a target of deferring of some other account.

Comment 48

[:aceman]

Attachment: patch v7

Comment 49

[[e-mail]]

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

This broke the junk panel for some reason :-(

>+#deferredServerPopup {
>+  min-width: 5em;
>+}
[Do you still need these?]

>+          this._numberOfItems = this.querySelectorAll("menuitem:not([disabled])").length +
>+                                this.querySelectorAll("menu:not([disabled])").length;
[Not sure why you put this here and not in am-server-advanced.js]

>+          if (aExcludeServers.length > 0) {
>+            for (let i = 0; i < folders.length; i++)
>+              if (folders[i].isServer &&
>+                  aExcludeServers.indexOf(folders[i].server.key) != -1)
>+                folders.splice(i, 1);
>+          }
Can you not use .filter()?

>+              if (aFolder == undefined)
I would just use !aFolder in these tests.

>+    { // If there are no other suitable accounts (outside of Local Folders)
>+      // to defer to, then disable the third option.
[Comments on open braces look odd.]

>+      if (folderPopup.numberOfItems == 0)
>+        document.getElementById("deferToServer").disabled = true;
>+      else // Otherwise leave it enabled and preselect first usable account.
>+        folderPopup.selectFolder();
[Maybe selectFolder() should return true or false depending on whether it succeeded, then you could write
if (!folderPopup.selectFolder())
  document.getElementById("deferToServer").disabled = true;
]

Comment 50

[:aceman]

(In reply to [e-mail] from comment #49)
> >+#deferredServerPopup {
> >+  min-width: 5em;
> >+}
> [Do you still need these?]
Not if we decide that Local Folders will always exist and will always be in the list (even though disabled). This was not sure so far. But if we decide on the current design I can remove it.

> >+          this._numberOfItems = this.querySelectorAll("menuitem:not([disabled])").length +
> >+                                this.querySelectorAll("menu:not([disabled])").length;
> [Not sure why you put this here and not in am-server-advanced.js]
> 
I wanted it to be a global new property of the folder picker, that could be used also somewhere else in the future.

> >+      if (folderPopup.numberOfItems == 0)
> >+        document.getElementById("deferToServer").disabled = true;
> >+      else // Otherwise leave it enabled and preselect first usable account.
> >+        folderPopup.selectFolder();
> [Maybe selectFolder() should return true or false depending on whether it
> succeeded, then you could write
> if (!folderPopup.selectFolder())
>   document.getElementById("deferToServer").disabled = true;
> ]

Yes, if you do not wish the numberOfItems functionality added I can do this.

Comment 51

[:aceman]

Attachment: patch v8

Ok, I could see the junk panel breakage in a new account. Also our account tests failed.
Try this version with your issues fixed.

Comment 52

[[e-mail]]

Comment on attachment 702990 [details] [diff] [review]
patch v8

I'm going to have to read selectFolder when I'm awake, so before I forget:

>+          if (aExcludeServers.length > 0) {
>+            folders = folders.filter(function (aFolder) { return !(aFolder.isServer &&
>+              aExcludeServers.indexOf(aFolder.server.key) != -1) });
>+          }
Weird wrapping here. Try
if (aExcludeServers.length > 0) {
  folders = folders.filter(function (aFolder) {
    return !(aFolder.isServer && aExcludeServers.indexOf(aFolder.server.key) != -1);
  });
}
If that line's too long you could wrap it after the &&. Or there's always this style:
if (aExcludeServers.length > 0) {
  folders = folders.filter(function (aFolder)
    !(aFolder.isServer && aExcludeServers.indexOf(aFolder.server.key) != -1)
  );
}

Comment 53

[[e-mail]]

Comment on attachment 702990 [details] [diff] [review]
patch v8

>+            if (!aFolder || (child._folder.URI == aFolder.URI) ||
>                 (child.tagName == "menu" &&
>+                 child._folder.isAncestorOf(aFolder)))
>+            {
>+              if (!aFolder || (child._folder.URI == aFolder.URI)) {
>                 // Making an assumption about our DOM positioning here.
>+                this.parentNode.selectedItem = child;
>               }
>+              if (aFolder) {
>+                // If this is a subfolder of what's in question, we merely appear
>+                // to select this node.
>+                setupParent(aFolder, this.parentNode);
>+              }
>+              return true;
This won't give the menulist the right icons in the case of default-selecting an element, because you don't call setupParent on the _folder like you do in the all items disabled case. Probably easiest fix is to use
setupParent(aFolder || child._folder, this.parentNode);

>+          if (aFolder) {
>+            // If the caller specified a folder to select and that was not
>+            // found, blow up.
>+            Components.utils.reportError("Unable to find folder to select " +
>+              (aFolder ? ":" + aFolder.prettyName : "") + "!");
Already checked for aFolder! r=me with those fixed.

Comment 54

[:aceman]

Attachment: patch v9

Thanks for the review.

Comment 55

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

Comment on attachment 704014 [details] [diff] [review]
patch v9

I think it's too weird to have Local Folders there, and unselectable, and will confuse people who then won't look for Local Folders in the option above.  So I've got to say ui-r-.  But I understand how having Local Folders in the list makes a lot of things come out nicer.  So, what do you both think of this:

Leave Local Folders in the list, enabled, and remove the "Global Inbox" radio button option?  (Perhaps, to ease the transition, we could append "(Global Inbox)" to the Local Folders menu item?)  Hopefully that'll be a fairly easy change, and might even make some of the back-end logic a little simpler?

Thanks,
Blake.

Comment 56

[:aceman]

Reducing the options to 2 seems fine (the backend is no problem). But adding the Global inbox suffix would be a new element to be hacked again into the folderWidgets.xml .

So why can't Local folders be hidden, so that the user looks for it around the dialog and finds it in option 2?

Comment 57

[:aceman]

Well, actually nothing happen if we leave them enabled. Selecting Local Folders or the Global inbox has the same effect. After reopening the dialog the Global inbox (2) will be selected.

Comment 58

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

If adding "Global Inbox" is too hard, then I'm happy to not do that.

Hiding Local Folders would be fine by me, but I do wonder about having a drop-down with nothing in it in that case…

Selecting Local Folders, and then having it change to Global Inbox would surprise and confuse me, so I don't think we should do that.

Comment 59

[:aceman]

We already had the empty picker solved in previous patches, but we removed it :(
The drop-down in that case has a min-width and is also disabled.

Comment 60

[:aceman]

Attachment: patch v10

OK, I found a hack how to rename the Local folders to Global inbox without introducing some new account renaming infrastructure. I just base all the logic on the mode="deferred" attribute and keep it internal to folderWidgets.xml. I also move the Local folder account to the top of the list. And remove the 2nd radio option. I like this solution because as I already said, if we eventually need the deferred accounts list in some other place that does not have a 3 state radio, we need Local folders in the list (so we can't hardcode its removal in folderWidgets.xml).

Please see if this is the solution we want before I polish it into final patch (translation entities, etc).

Comment 61

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

Comment on attachment 731340 [details] [diff] [review]
patch v10

UI-wise, this seems perfect to me!  Thank you for all your hard work getting it just right!  :)  ui-r=me.

Comment 62

[:aceman]

Neil?

Comment 63

[[e-mail]]

Comment on attachment 731340 [details] [diff] [review]
patch v10

Oh, this is just a second ui-review? Fair enough, but the actual patch will need to decide whether it still wants to use disableServers and also localise the Global Inbox menuitem for instance.

Comment 64

[:aceman]

(In reply to [e-mail] from comment #63)
> Oh, this is just a second ui-review? Fair enough, but the actual patch will
> need to decide whether it still wants to use disableServers and also
> localise the Global Inbox menuitem for instance.
Sure. I left "disableServers" in as a infrastructure possibility that could go in but is unused by this patch. Should I remove it?

Comment 65

[[e-mail]]

I won't insist on it.

Comment 66

[:aceman]

Attachment: patch v11

OK, this should be a complete version. The new string bundle may be a bit overkill, however I will need it for bug 802609. Or you can propose some better existing bundle.

Comment 67

[[e-mail]]

Comment on attachment 734265 [details] [diff] [review]
patch v11

>       <method name="_build">
>         <parameter name="aFolders"/>
>+        <parameter name="aExcludeServers"/>
>+        <parameter name="aDisableServers"/>
[I wonder why we bother passing these into _build when we could read them here just as easily.]

>+          let renameLocalFolders = false;
This does look a little awkward. A slight improvement might be to store the root folder of the local folders server instead.

>+            // If we are showing the accounts for deferring, move Local Folders to the top.
>+            if (mode == "deferred") {
>+              let localFoldersIndex = folders.indexOf(localFoldersServer.rootFolder);
>+              if (localFoldersIndex != -1) {
>+                renameLocalFolders = true;
>+                folders.splice(0, 0, folders[localFoldersIndex]);
>+                folders.splice(localFoldersIndex + 1, 1);
That would then allow you to write
folders.splice(localFoldersIndex, 1);
folders.unshift(globalInboxFolder);

>+              if (this.hasAttribute("disableServers"))
>+                popup.setAttribute("disableServers",
>+                                   this.getAttribute("disableServers"));
Why do subfolders care about servers?

>+            if (aDisableServers.length > 0 &&
>+                aDisableServers.indexOf(folder.server.key) != -1) {
No need to test the length first.

>       <method name="selectFolder">
Is this the same as v8? (Bugzilla interdiff failed me.)

>-          this._folders = null;
>+          // this._folders = null; // for later optimization
What does this do?

>+    let deferringSetUp = false;
>+    if (gServerSettings.account.incomingServer.isDeferredTo) {
>+      // Some other account already defers to this account
>+      // therefore this one can't be deferred further.
>+      radioGroup.value = "currentAccount";
>+      folderPopup.selectFolder();
>+      radioGroup.disabled = true;
>+      deferringSetUp = true;
>+    }
>+    else if (gFirstDeferredAccount.length)
>     {
>+      // The current account is already deferred...
>       let account = MailServices.accounts.getAccount(gFirstDeferredAccount);
>+      radioGroup.value = "otherAccount";
>+      folderPopup.selectFolder(account.incomingServer.rootFolder);
>+      deferringSetUp = true;
>     }
>     else
>     {
>-      radioGroup.selectedItem = document.getElementById("accountDirectory");
>+      // Current account is not deferred.
>+      radioGroup.value = "currentAccount";
>+    }
> 
>+    if (!deferringSetUp)
>+    {
>+      // If there are no other suitable accounts to defer to,
>+      // then disable the option.
>+      if (!folderPopup.selectFolder()) {
>+        radioGroup.value = "currentAccount";
>+        document.getElementById("deferToOtherAccount").disabled = true;
>+      }
>     }
This is overly complicated; if it's deferred, then it's deferred, otherwise it's not, and maybe it can't be made deferred for some reason.

>+  document.getElementById("deferredServerFolderPicker").disabled = !enablePicker;
>+  document.getElementById('deferGetNewMail').disabled = !enablePicker;
Nit: inconsistent quoting

Comment 68

[:aceman]

(In reply to [e-mail] from comment #67)
> >+              if (this.hasAttribute("disableServers"))
> >+                popup.setAttribute("disableServers",
> >+                                   this.getAttribute("disableServers"));
> Why do subfolders care about servers?
So the subfolders get disabled and can't be selected via selectFolder().

> >       <method name="selectFolder">
> Is this the same as v8? (Bugzilla interdiff failed me.)
Yes, should be no change.

> >-          this._folders = null;
> >+          // this._folders = null; // for later optimization
> What does this do?
Nothing, that is why I commented it out. I'll remove it altogether.

Comment 69

[:aceman]

(In reply to [e-mail] from comment #67)
> >+    let deferringSetUp = false;
> >+    if (gServerSettings.account.incomingServer.isDeferredTo) {
> >+      // Some other account already defers to this account
> >+      // therefore this one can't be deferred further.
> >+      radioGroup.value = "currentAccount";
> >+      folderPopup.selectFolder();
> >+      radioGroup.disabled = true;
> >+      deferringSetUp = true;
> >+    }
> >+    else if (gFirstDeferredAccount.length)
> >     {
> >+      // The current account is already deferred...
> >       let account = MailServices.accounts.getAccount(gFirstDeferredAccount);
> >+      radioGroup.value = "otherAccount";
> >+      folderPopup.selectFolder(account.incomingServer.rootFolder);
> >+      deferringSetUp = true;
> >     }
> >     else
> >     {
> >-      radioGroup.selectedItem = document.getElementById("accountDirectory");
> >+      // Current account is not deferred.
> >+      radioGroup.value = "currentAccount";
> >+    }
> > 
> >+    if (!deferringSetUp)
> >+    {
> >+      // If there are no other suitable accounts to defer to,
> >+      // then disable the option.
> >+      if (!folderPopup.selectFolder()) {
> >+        radioGroup.value = "currentAccount";
> >+        document.getElementById("deferToOtherAccount").disabled = true;
> >+      }
> >     }
> This is overly complicated; if it's deferred, then it's deferred, otherwise
> it's not, and maybe it can't be made deferred for some reason.
If it is deferred but something else defers to it I want to undefer it, that is why it is first branch. I am not sure what is superfluous here. The !deferringSetUp branch is just in case there is not even Local Folders ;)

Comment 70

[[e-mail]]

(In reply to :aceman from comment #69)
> (In reply to [e-mail] from comment #67)
> > >+    let deferringSetUp = false;
> > >+    if (gServerSettings.account.incomingServer.isDeferredTo) {
> > >+      // Some other account already defers to this account
> > >+      // therefore this one can't be deferred further.
> > >+      radioGroup.value = "currentAccount";
> > >+      folderPopup.selectFolder();
> > >+      radioGroup.disabled = true;
> > >+      deferringSetUp = true;
> > >+    }
> > >+    else if (gFirstDeferredAccount.length)
> > >     {
> > >+      // The current account is already deferred...
> > >       let account = MailServices.accounts.getAccount(gFirstDeferredAccount);
> > >+      radioGroup.value = "otherAccount";
> > >+      folderPopup.selectFolder(account.incomingServer.rootFolder);
> > >+      deferringSetUp = true;
> > >     }
> > >     else
> > >     {
> > >+      // Current account is not deferred.
> > >+      radioGroup.value = "currentAccount";
> > >+    }
> > > 
> > >+    if (!deferringSetUp)
> > >+    {
> > >+      // If there are no other suitable accounts to defer to,
> > >+      // then disable the option.
> > >+      if (!folderPopup.selectFolder()) {
> > >+        radioGroup.value = "currentAccount";
> > >+        document.getElementById("deferToOtherAccount").disabled = true;
> > >+      }
> > >     }
> > This is overly complicated; if it's deferred, then it's deferred, otherwise
> > it's not, and maybe it can't be made deferred for some reason.
> If it is deferred but something else defers to it I want to undefer it, that
> is why it is first branch.
That explains some, but not all of the duplication:

let deferringSetUp = false;
if (gServerSettings.account.incomingServer.isDeferredTo) {
  /* ... */
  deferringSetUp = true;
}
else if (gFirstDeferredAccount.length)
{
  /* ... */
  deferringSetUp = true;
}
else
{
  // Current account is not deferred.
  radioGroup.value = "currentAccount";
}

if (!deferringSetUp)
{
  /* ... */
}

The first problem is that there's only one place where deferringSetUp remains false, so you can eliminate it entirely by moving the controlled code:

else
{
  // Current account is not deferred.
  radioGroup.value = "currentAccount";
  // If there are no other suitable accounts to defer to,
  // then disable the option.
  if (!folderPopup.selectFolder()) {
    radioGroup.value = "currentAccount";
    document.getElementById("deferToOtherAccount").disabled = true;
  }
}

And this shows you're setting radioGroup.value twice.

Comment 71

[:aceman]

Attachment: patch v12

OK, that looks reasonable.

Comment 72

[:aceman]

Neil: ping

Comment 73

[:aceman]

Standard8, Neil, can you arrange for somebody to look at this?
Neil already looked over it and the patch fixes the last nits from comment 70.

This would be useful to get into TB24 to prevent users creating unwanted account deferral loops.

Comment 74

[[e-mail]]

Comment on attachment 736413 [details] [diff] [review]
patch v12

r=me based on inspection of the interdiff.

Comment 75

[:aceman]

Comment on attachment 736413 [details] [diff] [review]
patch v12

Thanks!

Comment 76

[Magnus Melin [:mkmelin]]

Comment on attachment 736413 [details] [diff] [review]
patch v12

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

Didn't have a chance to try it out yet, so r- for now

patching file mailnews/base/content/folderWidgets.xml
Hunk #1 FAILED at 12
1 out of 11 hunks FAILED -- saving rejects to file mailnews/base/content/folderWidgets.xml.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 389139.patch

::: mailnews/base/content/folderWidgets.xml
@@ +416,5 @@
> +            let nameCompare = function nameCompare(a, b) {
> +              let sortKey = a.compareSortKeys(b);
> +              if (sortKey)
> +                return sortKey;
> +              return a.prettyName.localeCompare(b.prettyName);

coule be return sortKey || a.prettyName.localeCompare(b.prettyName);

@@ +469,1 @@
>              var node;

make this let while your're there

@@ +690,3 @@
>           - @note If aFolder is not in this popup, but is instead a descendant of
>           -       a member of the popup, that ancestor will be selected.
> +         - @return  True if any usable folder was found, otherwise false.

lowercase true

@@ +709,3 @@
>                  (child.tagName == "menu" &&
> +                 child._folder.isAncestorOf(aFolder)))
> +            {

{ on the previous row seems to be the style here. or is it splinter showing it wrong?

@@ +714,1 @@
>                }

no need for braces

@@ +723,5 @@
> +
> +          if (aFolder) {
> +            // If the caller specified a folder to select and it was not
> +            // found, blow up.
> +            throw "Unable to find folder " + aFolder.prettyName + " in the picker!";

never throw strings. throw new Error("foo");

::: mailnews/base/prefs/content/am-junk.js
@@ +52,5 @@
>    let folder = null;
>    try {
>      folder = GetMsgFolderFromUri(spamActionTargetFolder, true);
>      document.getElementById("actionFolderPopup").selectFolder(folder);
> +  } catch (e) { /* OK for the folder to not exist. */ }

in general /* comments */ for small comments are a bit of a pain, in case you want to comment out blocks later while debugging etc

@@ +58,2 @@
>      folder = GetMsgFolderFromUri(spamActionTargetFolder, false);
>    }

no need for braces

::: mailnews/base/prefs/content/am-server-advanced.xul
@@ +109,3 @@
>            <rows>
>              <row>
> +              <radio value="currentAccount" id="deferToCurrentAccount"

while touching these, make id the first attribute, here and a couple places below

::: mailnews/base/public/nsIMsgIncomingServer.idl
@@ +466,5 @@
>    readonly attribute nsIMsgFilterPlugin spamFilterPlugin;
>  
>    nsIMsgFolder getMsgFolderFromURI(in nsIMsgFolder aFolderResource, in ACString aURI);
>  
> +  // Does any other server have deferred storage to this account ?

Use /// to make it a doxygen recognized documentation format

Comment 77

[:aceman]

(In reply to Magnus Melin from comment #76)
> ::: mailnews/base/prefs/content/am-junk.js
> @@ +52,5 @@
> >    let folder = null;
> >    try {
> >      folder = GetMsgFolderFromUri(spamActionTargetFolder, true);
> >      document.getElementById("actionFolderPopup").selectFolder(folder);
> > +  } catch (e) { /* OK for the folder to not exist. */ }
> 
> in general /* comments */ for small comments are a bit of a pain, in case
> you want to comment out blocks later while debugging etc

I think // could not be on a single line with the closing brace.

Comment 78

[Magnus Melin [:mkmelin]]

yes, but there's no need for it to be a single line

Comment 79

[Magnus Melin [:mkmelin]]

or then use
catch(e) { } // ok for the folder not to exist

Comment 80

[:aceman]

Attachment: patch v13

Comment 81

[Magnus Melin [:mkmelin]]

Comment on attachment 757988 [details] [diff] [review]
patch v13

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

Somthing's wrong with the patch. In "Inbox for different account" i get an empty dropdown, well two blank items. E time i open the popup another empty item is added to the list.

(I don't have a global inbox using account set up. Only "normal" pop non-deferred ones. (Just one actually, i think.)

::: mailnews/base/content/folderWidgets.xml
@@ +346,5 @@
>              folders = aFolders;
>            }
>  
> +          if (excludeServers.length > 0) {
> +            folders = folders.filter(function (aFolder) {

nit: no space after function

@@ +426,5 @@
> +             * want to sort the folders, but rather the accounts to which the
> +             * folders belong.  Since that sorting was already done, we don't need
> +             * to do anything for that case here
> +             */
> +            folders = folders.sort(nameCompare);

might as well inline the nameCompare function, it's only used here right?

::: mailnews/base/public/nsIMsgIncomingServer.idl
@@ +472,5 @@
>    readonly attribute nsIMsgFilterPlugin spamFilterPlugin;
>  
>    nsIMsgFolder getMsgFolderFromURI(in nsIMsgFolder aFolderResource, in ACString aURI);
>  
> +  /// Does any other server have deferred storage to this account ?

end with period, or at least drop the space before ?

Comment 82

[:aceman]

Attachment: patch v14

The patch does not apply cleanly on trunk due to a checkin in the last hours. Maybe you didn't notice one hunk in folderWidgets.xml failing which may have caused this. Please try this one.

Comment 83

[Magnus Melin [:mkmelin]]

That doesn't seem to be the case. I'll try a full rebuild...

Comment 84

[:aceman]

For me, with only one POP3 account in the profile (and Local Folders and News), the dropdown contains only "Global inbox (Local folders)" item.

Comment 85

[Magnus Melin [:mkmelin]]

Comment on attachment 759382 [details] [diff] [review]
patch v14

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

Whatever it was, a full rebuild made it work. Looks good now, r=mkmelin

::: mailnews/base/content/folderWidgets.xml
@@ +431,5 @@
> +             * comparison of names.
> +             */
> +            folders = folders.sort(function nameCompare(a, b) {
> +                                   return a.compareSortKeys(b) ||
> +                                          a.prettyName.localeCompare(b.prettyName);

i think the usual indenting here would be

folders = ...... ({
  return ....
)};

Also the comments should just be // now

Comment 86

[Magnus Melin [:mkmelin]]

I'll note that the "Blogs and News Feeds" rss account is a selectable target. I'm not sure that's really a good idea, and if it will work out well...

Comment 87

[:aceman]

Thanks, see comment 45.

Comment 88

[:aceman]

Attachment: patch v15

Thanks for the review.

Comment 89

[Ryan VanderMeulen [:RyanVM]]

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

Comment 90

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #74)
> Comment on attachment 736413 [details] [diff] [review]
> patch v12
> 
> r=me based on inspection of the interdiff.

That explains why this slipped through:

mail/locales/jar.mn (59): %chrome/messenger/folderWidgets.properties
suite/locales/jar.mn (263): %chrome/mailnews/folderwidgets.properties

The "W" vs. "w" difference leads to a breakage on case-sensitive file systems (e.g. on Linux), like so:

RuntimeError: File "chrome/mailnews/folderwidgets.properties" not found

aceman, can you fix this please?

Comment 91

[:aceman]

Attachment: patch - fix SM path

Sure, thanks for the catch.

Comment 92

[[e-mail]]

Comment on attachment 763093 [details] [diff] [review]
patch - fix SM path

I was testing these patches on Windows so I wouldn't have noticed anyway...

Comment 93

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/15d9ff695542