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

RESOLVED FIXED in Thunderbird 50.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 50.0

Thunderbird Tracking Flags

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

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

9 months ago
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.
(Assignee)

Comment 1

9 months ago
Created attachment 8773236 [details] [diff] [review]
WIP: debug patch

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

Comment 2

9 months ago
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
(Assignee)

Comment 3

9 months ago
Created attachment 8773248 [details] [diff] [review]
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?
Attachment #8773236 - Attachment is obsolete: true
(Assignee)

Comment 4

9 months ago
Created attachment 8773249 [details] [diff] [review]
proposed solution (v1a).

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

Updated

9 months ago
Attachment #8773248 - Attachment is obsolete: true

Comment 5

9 months ago
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

9 months ago
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.
(Assignee)

Comment 7

9 months ago
(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?
(Assignee)

Comment 8

9 months ago
Created attachment 8773354 [details] [diff] [review]
proposed solution (v2a.)

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 9

9 months ago
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 10

9 months ago
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
(Assignee)

Comment 11

9 months ago
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

9 months ago
Well, not first one, the default. And that .defaultAccount can throw is just silly, even if it is the xpcom way.
(Assignee)

Comment 13

9 months ago
Created attachment 8773423 [details] [diff] [review]
proposed solution (v2b).

Added try/catch.
Attachment #8773354 - Attachment is obsolete: true
Attachment #8773423 - Flags: review+

Comment 14

9 months ago
(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.
(Assignee)

Comment 15

9 months ago
Created attachment 8773426 [details] [diff] [review]
proposed solution (v3a).

OK, let's do this the Magnus way ;-)
Attachment #8773423 - Attachment is obsolete: true
Attachment #8773426 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 16

9 months ago
Haste makes waste. Yes, I'll fix the spaces.
(Assignee)

Comment 17

9 months ago
Created attachment 8773439 [details] [diff] [review]
proposed solution (v3b).

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

Comment 18

9 months ago
Created attachment 8773446 [details] [diff] [review]
proposed solution (v3c).

This should be right, hopefully ;-)
Attachment #8773439 - Attachment is obsolete: true
Attachment #8773439 - Flags: review?(acelists)
Attachment #8773446 - Flags: review?(acelists)
(Assignee)

Comment 19

9 months ago
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)
(Assignee)

Updated

9 months ago
Attachment #8773423 - Attachment is obsolete: false
(Assignee)

Comment 20

9 months ago
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)
(Assignee)

Comment 21

9 months ago
https://hg.mozilla.org/comm-central/rev/bb570cd6e2f5
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-thunderbird47: --- → wontfix
status-thunderbird48: --- → wontfix
status-thunderbird49: --- → affected
status-thunderbird50: --- → fixed
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
(Assignee)

Comment 22

9 months ago
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+
(Assignee)

Comment 23

9 months ago
TB 49 (Aurora):
https://hg.mozilla.org/releases/comm-aurora/rev/cf2e3d47bb06
status-thunderbird49: affected → fixed
(Assignee)

Comment 24

7 months ago
Also baked in TB 49 beta.
Flags: needinfo?(rkent)

Comment 25

7 months ago
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+

Updated

7 months ago
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → 49+
You need to log in before you can comment on or make changes to this bug.