228593 - identity drop-down gets confused by shared identities

Status: ASSIGNED

(MailNews Core :: Composition, defect)

Description

[Christian Eyrich]

When sharing identities between accounts, the identities drop-down menu gets
confused. The key in the value attribute is identity.key. If you're using an
identity in multiple accounts, the popup entry selected will always the first
one using an identity.

So you have
    [e-mail] via newserver1.example.org
    [e-mail] via news.mozilla.org
    [e-mail] via news.blahblah.com

Selecting news.mozilla.org in your folder pane, clicking Compose always
sets you at [e-mail] via newserver1.example.org.

That's the same for identites shared by mail accounts. But while it's a merely
cosmetic problem there, it's a real one for news.
That's because the outgoing NNTP server is bound to the account, not to the
identity. So the user sees [e-mail] as From: and thinks it's the one of
news.mozilla.org but it's newserser1.example.org. And the mail goes out through
this ...

So maybe we've to change the drop-down menu a little and build a
combined key consisting of "idkey accountkey" to make it unique. That
requires dealing with split(" ") when accessing the key in the value
attribute but should be future-proof.

The changes for building the drop-down (in FillIdentityListPopup(),
MsgComposeCommands.js) and extracting it (in getCurrentIdentity()) is simple. My
only problem is getting the accounts key into ComposeStartup()'s through the
aParams parameter.

Comment 1

[Christian Eyrich]

Attachment: example patch

This is the js code to generate and handle the combined key. The code in
ComposeStartup() assumes, the account will be provided in aParams.

The function getCurrentAccountKey() is a modified version from bug 228597. That
also shows that carrying the account key with us is also a good deed outside of
this bug.

Comment 2

[Christian Eyrich]

Attachment: proposed patch

This is again one of the patches doing nearly nothing except pushing the
account through dozends of functions. All real work is described in comments #0
and #1. The cpp code is for providing the account in the params.

Comment 3

[Scott MacGregor]

Christian, when we go to find the default identity for the compose window, we
call getIdentityForServer which trys to pick the right identity to choose.

This code enumerates over just the identities for the current server and then
picks the right one to bring up the compose window with. 

Is something broken there that is causing us to look an identity for an account
that is not even the currently selected account?

Comment 4

[Christian Eyrich]

No, the problem isn't the code that looks up the identity. But if an identity is
assigned to multiple accounts, its identity key is the value for multiple
entries in the identity drop-down.

So if an identity is selected (identityList.value = identity.key) it just
chooses the first entry with this value (key). I tried to make the values uniqe
by composing them from identity.key + account.key.
It's a huge amount of code to change, maybe it can be done another way too. I'd
be happy if so.

Comment 5

[Jim]

*** Bug 250364 has been marked as a duplicate of this bug. ***

Comment 6

[Jim]

Bug 250364 is a duplicate of this bug


User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2)
Gecko/20040705
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2)
Gecko/20040705

Compose/Reply does not select the correct Identity for the currently selected
account.  It always defaults to the first account identity.  For example I have
2 mail accounts.  While in mail account 2 I select reply or compose.  The
compose comes up but the from address Identity is not the identity of account 2.
 It is the identity of account 1.  In earlier versions (April/May 2004) this
worked as expected.

Reproducible: Always
Steps to Reproduce:
1.Setup 2 or more mail accounts.
  Make sure the itentity (Name, from address, etc) are different.
2.Select compose or reply while in the second mail account.
3.Notice the from address at the top.  It will default to the first account and
not the account you are replying from.

Actual Results:  
The from address is not the from address defined in the current account.

Expected Results:  
The from address (identity) should have defaulted to the identity of the current
account.

Comment 7

[Christian Eyrich]

Jim, I read your other bug yesterday but couldn't find why it's a dupe to this one.
If you don't share one identity (the id, not only the data) between your two
accounts, it shouldn't be this bug. Additionally this bug exists since last
December but you wrote April/May version work as expected.

There's something wrong.

Comment 8

[Jim]

Reading this bug sounded like what I was experiencing.  You are right that this
bug is not exactly like the one I reported.  

Comment 9

[Mike Cowperthwaite]

Christian Eyrich, is there a reason to keep this bug open?  In the compose window now, the identity is appended with an account, which seems to address what you were talking about here.

There are still problems with uniquely identifying identities -- for instance, I have two identities with the same username and email address associated with the same account, one of which uses a sig and the other doesn't, but they appear identically in the dropdown.  But the issue of knowing which news-server will be used appears to be solved.

Comment 10

[Christian Eyrich]

> Christian Eyrich, is there a reason to keep this bug open?

I think yes.
The issue may be theoretical for two reasons: 1. The very same identity (i.e. mail.identity.idx) can only be assigned manually, not via the UI. 2. The user has the chance to notice the automatically picked up identity is the wrong once since it's displayed in grey behind the address.

But from what I see and tested, the problem is still valid.

Comment 11

[Wayne Mery (:wsmwk)]

Christian, can you check that this patch still applies and request new review? I'll reset review request next.

(I wonder if this is one of my current problems)

Comment 12

[Wayne Mery (:wsmwk)]

Comment on attachment 143327 [details] [diff] [review]
proposed patch

canceling mscott review

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

As part of patchlove, David, is this 4.5 year old patch still relevant?

(Btw, the patch has already bitrotted.)

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 143327 [details] [diff] [review]
proposed patch

patch has bitrotted.

Comment 15

[Magnus Melin [:mkmelin]]

I assume it's per se valid, but since it's mainly a theoretical issue (you have to manually go into the prefs and make the identity shared - can't in the UI, as per comment 11), I don't think it's worth doing.

Comment 16

[Karsten Düsterloh]

Definitely not WONTFIX, especially since the UI inability to create this is the actual problem. ;-) Just low priority.

Comment 17

[Magnus Melin [:mkmelin]]

wanted‑thunderbird3- as this is very much an edge case.

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Christian, any chance of a new patch?

Comment 19

[Christian Eyrich]

Not from me for the next months.

Comment 20

[WADA:World Anti-bad-Duping Agency]

> "by shared identities" in bug summary.

How can identity be shared, without manual modification of prefs.js?

Identity is already created as slave of account always, even when news account.
  mail.account.accountA.identities = idB, ..., idZ
Identity defines usable email adress as From:.
  mail.identity.idB.useremail = [e-mail]-B
  mail.identity.idZ.useremail = [e-mail]-Z
And any dentity can have any usermail value.
So multiple identities of same email address is possible.
However, if current Tb's UI is used for identity definition, "shared identities" is impossible.

As Magnus says in comment #15, to force the impossible "shared identities", manual prefs.js change is currently needed.
  mail.account.accountX.identities = idZ
  mail.account.accountY.identities = idZ
But this is manual corruption of prefs.js for current Tb.
If protection from "shared identities by manual corruption of prefs.js" is needed, I think "remove double booking of identity at start up" is sufficient, as done on unused server.serverN to avoid incorrect account.accountN.server value.

Comment 21

[roger21]

this bug should be closed, probably as fixed:

the problem as described is: "a possible confusion in the identity drop down list when an identity has been manually shared between two or more accounts"

it is impossible to be confused for three reasons:

1/ as said previously, the server name is now clearly displayed in grey next to the identity email/name

2/ the default identity selected is always related to what i'm responding to or what i'm composing: the default mail identity is selected if i'm composing or responding to a mail and the default newsgroup identity is selected if i'm composing or responding to a newsgroup, so, by default, even if the shared identity is the default one it will be selected accordingly to what i'm doing

3/ if i intentionally choose the wrong identity, the one linked to the mail account instead of the one linked to the newsgroup account and i try to send to a newsgroup, i will have an alert expliciting that i'm not using the "right" identity (and in the opposite case, newsgroup account identity to mail recipient, there is no difference with using the mail account identity since it is the same identity - same mail, same smtp, same data)


what possibly remains is:

(In reply to WADA from comment #20)
> If protection from "shared identities by manual corruption of prefs.js" is
> needed, I think "remove double booking of identity at start up" is
> sufficient

and

(In reply to Mike Cowperthwaite from comment #9)
> There are still problems with uniquely identifying identities -- for
> instance, I have two identities with the same username and email address
> associated with the same account, one of which uses a sig and the other
> doesn't, but they appear identically in the dropdown.  But the issue of
> knowing which news-server will be used appears to be solved.

as another bug since it is about two different but similar account

Comment 22

[roger21]

(In reply to roger21 from comment #21)
> as another bug since it is about two different but similar account

i meant identities not accounts