Closed Bug 1288363 Opened 3 years ago Closed 3 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set

Tracking

(thunderbird47 wontfix, thunderbird48 wontfix, thunderbird49 fixed, thunderbird50 fixed, thunderbird_esr4549+ fixed)

RESOLVED FIXED
Thunderbird 50.0
Tracking Status
thunderbird47 --- wontfix
thunderbird48 --- wontfix
thunderbird49 --- fixed
thunderbird50 --- fixed
thunderbird_esr45 49+ fixed

People

(Reporter: jorgk, Assigned: jorgk)

Details

Attachments

(1 file, 7 obsolete files)

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.
Attached patch WIP: debug patch (obsolete) — Splinter Review
Debug is:
Jorg Knobloch <jorgk@jorgk.com>=== 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.
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) <mail@jorgk.com>=== params.identity.identityName
=== params already supplied
id4=== params.identity.key
Attached patch WIP: debug patch (v2). (obsolete) — Splinter Review
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?
Attachment #8773236 - Attachment is obsolete: true
Attached patch proposed solution (v1a). (obsolete) — Splinter Review
Hi guys, simple solution. One review will do, whoever is interested.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8773249 - Flags: review?(mkmelin+mozilla)
Attachment #8773249 - Flags: review?(acelists)
Attachment #8773248 - Attachment is obsolete: true
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 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.
(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?
Attached patch proposed solution (v2a.) (obsolete) — Splinter Review
Oops, looks like I've just written nonsense. Here the boolean solution.
Attachment #8773249 - Attachment is obsolete: true
Attachment #8773249 - Flags: review?(mkmelin+mozilla)
Attachment #8773249 - Flags: review?(acelists)
Attachment #8773354 - Flags: review?(acelists)
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.
Attachment #8773354 - Flags: review?(acelists) → review+
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
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.
Well, not first one, the default. And that .defaultAccount can throw is just silly, even if it is the xpcom way.
Added try/catch.
Attachment #8773354 - Attachment is obsolete: true
Attachment #8773423 - Flags: review+
(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.
Attached patch proposed solution (v3a). (obsolete) — Splinter Review
OK, let's do this the Magnus way ;-)
Attachment #8773423 - Attachment is obsolete: true
Attachment #8773426 - Flags: review?(mkmelin+mozilla)
Haste makes waste. Yes, I'll fix the spaces.
Attached patch proposed solution (v3b). (obsolete) — Splinter Review
Actually, that wasn't right. This should be better. A little complicated.
Attachment #8773426 - Attachment is obsolete: true
Attachment #8773426 - Flags: review?(mkmelin+mozilla)
Attachment #8773439 - Flags: review?(acelists)
Attached patch proposed solution (v3c). (obsolete) — Splinter Review
This should be right, hopefully ;-)
Attachment #8773439 - Attachment is obsolete: true
Attachment #8773439 - Flags: review?(acelists)
Attachment #8773446 - Flags: review?(acelists)
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?
Attachment #8773446 - Flags: review?(mkmelin+mozilla)
Attachment #8773423 - Attachment is obsolete: false
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.
Attachment #8773446 - Attachment is obsolete: true
Attachment #8773446 - Flags: review?(mkmelin+mozilla)
Attachment #8773446 - Flags: review?(acelists)
https://hg.mozilla.org/comm-central/rev/bb570cd6e2f5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
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.
Attachment #8773423 - Flags: approval-comm-esr45?
Attachment #8773423 - Flags: approval-comm-aurora+
Also baked in TB 49 beta.
Flags: needinfo?(rkent)
Comment on attachment 8773423 [details] [diff] [review]
proposed solution (v2b).

http://hg.mozilla.org/releases/comm-esr45/rev/e19ff9cd9311
Flags: needinfo?(rkent)
Attachment #8773423 - Flags: approval-comm-esr45? → approval-comm-esr45+
You need to log in before you can comment on or make changes to this bug.