1288363 - When no account can be determined on reply or forward (for example to BCC'ed message) first account is used instead of default

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

This is a continuation from bug 1287338.

Basically, when TB can't determine a suitable account on reply, for example to a BCC'ed message, the first account from "mail.accountmanager.accounts" is used, of course skipping "Local Folders".

The default account should be used instead since the default account is also used when starting a new message when a local folder is active.

Code is here:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2359
and here for SM:
https://dxr.mozilla.org/comm-central/source/suite/mailnews/compose/MsgComposeCommands.js#1086

This is particularly confusing to the user when first account and default account differ. The default account always shows up first in the folder pane and the account settings, however, it is not chosen if another account comes first in the account list. This account list is not visible to the user, it is only visible in prefs.js.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: WIP: debug patch

Debug is:
Jorg Knobloch <[e-mail]>=== params.identity.identityName
=== params already supplied
id1=== params.identity.key

That's wrong. The default account is account4 and its only identity is id4.

id1 belonging account1 to is the first in
user_pref("mail.accountmanager.accounts", "account1,account4,account5,account7,account8,account6,account3,account2");

My assessment from comment #0 was wrong. The parameters are already wrong earlier on. Needs more debugging. Aceman, do you have an idea, you're busily ripping out some of this stuff in bug 454064.

Comment 2

[Jorg K (CEST = GMT+2)]

OK, dumping out like this:
    catch(ex) { dump("ERROR with parameters: " + ex + "\n"); }
    dump(params.identity.identityName + "=== params.identity.identityName\n");
    dump(params.identity.key + "=== params.identity.key\n");
further up gives the wrong identity as well. So the window is already called with wrong parameters.

In comparison a new message dumps out:
Jorg Knobloch (Mail) <[e-mail]>=== params.identity.identityName
=== params already supplied
id4=== params.identity.key

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: WIP: debug patch (v2).

OK, unlike "new message", a reply takes this path:
1) ComposeMessage() at mailCommands.js (272) calls getIdentityForHeader()
2) getIdentityForHeader() at mailCommands.js (126) calls getBestIdentity()
3) getBestIdentity() at the end of the function doesn't know better and selects the first from the list.

Debug:
=== picking first
== JS stack> chrome://messenger/content/mailCommands.js (41)
== JS stack> chrome://messenger/content/mailCommands.js (126)
== JS stack> chrome://messenger/content/mailCommands.js (272)
== JS stack> chrome://messenger/content/mailWindowOverlay.js (1597)
== JS stack> chrome://messenger/content/mailWindowOverlay.js (1617)
== JS stack> chrome://messenger/content/messenger.xul (1)

So now we know why it doesn't work, how do we fix it?

Should the last desperate act in getBestIdentity() be to return the default instead of the first one?

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: proposed solution (v1a).

Hi guys, simple solution. One review will do, whoever is interested.

Comment 5

[rsx11m]

Comment on attachment 8773249 [details] [diff] [review]
proposed solution (v1a).

> /**
>  * Get the identity that most likely is the best one to use, given the hint.
>  * @param identities    nsIArray<nsIMsgIdentity> of identities
>  * @param optionalHint  string containing comma separated mailboxes
>+ * @param optionalHint  default identity of default account
>  */
>-function getBestIdentity(identities, optionalHint)
>+function getBestIdentity(identities, optionalHint, defaultIdentity = null)

The new comment should probably refer to "@param defaultIdentity" instead.

Comment 6

[:aceman]

Comment on attachment 8773249 [details] [diff] [review]
proposed solution (v1a).

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

::: mail/base/content/mailCommands.js
@@ +14,2 @@
>   */
> +function getBestIdentity(identities, optionalHint, defaultIdentity = null)

Why do you need to pass the default identity in? Can't it be fetched inside the function (and only when needed)? Or do you want to control when the function is allowed to fall back to default identity? That can still be done with a boolean argument.

Comment 7

[Jorg K (CEST = GMT+2)]

(In reply to rsx11m from comment #5)
> The new comment should probably refer to "@param defaultIdentity" instead.
Oops ;-(

(In reply to :aceman from comment #6)
> Why do you need to pass the default identity in? Can't it be fetched inside
> the function (and only when needed)? Or do you want to control when the
> function is allowed to fall back to default identity?
Yes. There are only two call sites. This one
https://dxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#46
does *not* want the default identity from the default account.
This one, the one I changed
https://dxr.mozilla.org/comm-central/source/mail/base/content/mailCommands.js#122
does.

> That can still be done with a boolean argument.
Yes, but the function doesn't get passed the account manager, so it would have to retrieve it whereas the caller already has it. Surely you could pass the account manager as a flag, so if the argument is non-zero, it's the account manager that can be used to get the default identity from the default account. Ugly, huh? So can you accept my solution or what would you prefer?

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: proposed solution (v2a.)

Oops, looks like I've just written nonsense. Here the boolean solution.

Comment 9

[:aceman]

Comment on attachment 8773354 [details] [diff] [review]
proposed solution (v2a.)

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

Please adjust the commit message to mention when we use the default identity.

It is quite non-standard that there is accountManager variable defined (it is in another file). But even if it wouldn't you would use Mailservices.accounts directly (mailservices.js is included in this file).

::: mail/base/content/mailCommands.js
@@ +38,5 @@
>    }
>  
> +  if (useDefault) {
> +    let defaultAccount = accountManager.defaultAccount;
> +    if (defaultAccount && defaultAccount.defaultIdentity)

Nice that you check for defaultAccount, but currently accountManager.defaultAccount will throw an exception if there is no default account. Please enlose in try..catch.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 8773354 [details] [diff] [review]
proposed solution (v2a.)

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

Especially with only two callers, I don't know why you bother this method with a default value, that's kind of non standard. What you typically do is return null if you find nothing, then handle that in the caller

Comment 11

[Jorg K (CEST = GMT+2)]

Well, that adds more complication. The caller then would have to extract the first one from the list but check that the list is not empty. I think the way we have it is simpler.

Comment 12

[Magnus Melin [:mkmelin]]

Well, not first one, the default. And that .defaultAccount can throw is just silly, even if it is the xpcom way.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: proposed solution (v2b).

Added try/catch.

Comment 14

[:aceman]

(In reply to Magnus Melin from comment #12)
> Well, not first one, the default. And that .defaultAccount can throw is just
> silly, even if it is the xpcom way.

Bug 342632.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: proposed solution (v3a).

OK, let's do this the Magnus way ;-)

Comment 16

[Jorg K (CEST = GMT+2)]

Haste makes waste. Yes, I'll fix the spaces.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: proposed solution (v3b).

Actually, that wasn't right. This should be better. A little complicated.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: proposed solution (v3c).

This should be right, hopefully ;-)

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 8773446 [details] [diff] [review]
proposed solution (v3c).

Magnus: This resulted in a whole lot more code and changes than solution v2c in attachment 8773423 [details] [diff] [review]. Do you still want it your way?

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8773446 [details] [diff] [review]
proposed solution (v3c).

Magnus, I've thought about it for a while. I can understand your approach, but let me argue in favour of mine:

getBestIdentity() gets passed a list of identities and its job is to pick the "best" one. It doesn't make sense to pick none and leave it to the caller, who in turn has to check the length of the list passed in.

In case of passing in "all identities" it's fair to tell the function via flag that its last desperate measure should be to pick the default and not the first.

I will land this now with the comment slightly adjusted. Formally I have r+ from Aceman to do so.

Comment 21

[Jorg K (CEST = GMT+2)]

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

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8773423 [details] [diff] [review]
proposed solution (v2b).

[Approval Request Comment]
Regression caused by (bug #): No regression, was always wrong.
User impact if declined: Confusing behaviour. Sometime the default account/identity is used, other times the first one in the list. And the list is not visible to the user, they'd have to inspect prefs.js.
Testing completed (on c-c, etc.): Manual testing.
Risk to taking this patch (and alternatives if risky):
Low, a few lines of JS to retrieve the default identity of the default account.

Comment 23

[Jorg K (CEST = GMT+2)]

TB 49 (Aurora):
https://hg.mozilla.org/releases/comm-aurora/rev/cf2e3d47bb06

Comment 24

[Jorg K (CEST = GMT+2)]

Also baked in TB 49 beta.

Comment 25

[Kent James (:rkent)]

Comment on attachment 8773423 [details] [diff] [review]
proposed solution (v2b).

http://hg.mozilla.org/releases/comm-esr45/rev/e19ff9cd9311