226580 - fix backend multiple identity support

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect)

Description

[Alec Flett]

Back when I made multiple identities, I never did the final work of parsing out
the mail.account.idX.identities array.. duh! 

thanks to david greaves for pointing this out.. I've got a fix and am attaching
a patch momentarily

Comment 1

[Alec Flett]

Attachment: parse out the identity keys

.. and here it is. Now I just need some reviewers.

Comment 2

[Alec Flett]

Comment on attachment 136175 [details] [diff] [review]
parse out the identity keys

..its been so long, I forget who needs to review this..

Comment 3

[Scott MacGregor]

oops meant to add that the sr is for post 1.6b freeze :)

Comment 4

[Ben Bucksch (:BenB)]

Same as bug 48757?

Would be *really* great, if we had working support for multiple identities, even
without account management UI.

I tried the patch and it works great for me (with 1.4.1), works as expected.
Thanks a lot, finally a bit cleanup in my accounts! :-)

Comment 5

[Ben Bucksch (:BenB)]

Unfortunately, the account manager destroys the glory, open it, close it, and
your multiple identities are gone :-(, so don't use it. Creating a new account
using the UI (File|New|Account) seems to work, though.

Comment 6

[Alec Flett]

thanks for testing this, ben. sounds like we're not writing out the excess
identities. hmm... I'll continue investigating.

Comment 7

[Peter Lairo]

Will fixing this bug include some UI? Either way, could a *brief* pointer of
where/how to set up multiple identities be posted in this bug, so we (those
CC'd) can know and explain it to others?

Comment 8

[Ben Bucksch (:BenB)]

> Will fixing this bug include some UI?

No, that's what "backend" means.

> Either way, could a *brief* pointer of
> where/how to set up multiple identities be posted in this bug

Bug 48757 (see above) and
<http://www.mozilla.org/mailnews/arch/accountmanager.html>. Further questions
about that please to the newsgroup.

Comment 9

[Alec Flett]

No - no UI changes come out of this bug. That would be bug 44863.

Look at http://www.mozilla.org/mailnews/arch/accountmanager.html for some low
level information. Until we get UI, maybe someone can write up a nice doc on
this stuff.

Comment 10

[Alec Flett]

ok, I figured out the problem. Its not actually the opening or closing of the
account manager that blows it away.

the problem is that nsMsgAccount::AddIdentity() rewrites the pref every time.
That worked fine when there was only one identity, but now I have to figure out
a smart way to add the identity without upsetting the pref.

I think I need to make an AddIdentityInternal() or something, that adds the
identity without modifying the pref.

Patch forthcoming.

Comment 11

[Ben Bucksch (:BenB)]

Alecf, I already filed bug 226585 about the account manager problem. Do you want
to deal with that problem there or keep it all here?

Comment 12

[Alec Flett]

Attachment: parse identity keys, and save them correctly

ok, here's an updated version. I switched to using nsCRT::strtok, because we
use that all over. I basically ripped off the pattern of using that and
StripWhitespace() from other places in the code.

Someone want to try this patch before I go to get it reviewed again? I won't be
able to test this until later today...

Comment 13

[Ben Bucksch (:BenB)]

With the first patch, I noticed that quitting the app also removes the added
identities.

This is fixed with the second patch, as well as the account manager problem.
Thanks :-).

I now have a strange problem, though, with both patches. If I start from KDE's
menu, using KDE su, I don't see any identity (only "<>") for the account with
multiple identities. If I start from a shell (under that OS account), it works.
No idea what the reason is, might be something on my end.

Comment 14

[Alec Flett]

oh, and yeah I'd rather just deal with it here - get the right patch in all at
once (especially since I now have the right patch)

Comment 15

[Ben Bucksch (:BenB)]

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

Comment 16

[Holger Mauermann]

Second patch works great! Steps to create two additional identities: add

   user_pref("mail.identity.id11.fullName", "My Name");
   user_pref("mail.identity.id11.useremail", "[e-mail]");
   user_pref("mail.identity.id12.fullName", "My Name");
   user_pref("mail.identity.id12.useremail", "[e-mail]");

and append both ids to your main account

   user_pref("mail.account.account1.identities", "id1,id11,id12");

in prefs.js. You may also add fcc_folder, sig_file etc.

However, creating a new _account_ with the existing UI doesn't work anymore.
Clicking on "Finish" in the Account Wizard crashes Thunderbird...

Comment 17

[Alec Flett]

oh! ack. I'll look into that. thanks a ton for testing..

Comment 18

[timeless]

my bet is that the crash is fixed and doesn't relate to alecf's work, when you
report crashes (even of modified builds) it helps to indicate your build id.

i could be wrong, and if you crash w/ a build from today then i am wrong, a
stack would be good (even if it's just module names, my bet is that your crash
was nspr<appshell)

Comment 19

[Holger Mauermann]

> my bet is that the crash is fixed and doesn't relate to alecf's work, when
> you report crashes it helps to indicate your build id.

"Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6b) Gecko/20031125
Thunderbird/0.4a" build from CVS, checkout started today at 09:30 +0100 (CET).
It crashes only with the patch applied, a build without it does not crash.

Comment 20

[Ben Bucksch (:BenB)]

I wrote:
> I now have a strange problem, though, with both patches.

I was indeed confused, the KDE menu entry started an unpatched build, duh.
It seems that unpatched builds don't show *any* identity for an account, if
there are several ones.

I can confirm (using a 1.4 branch seamonkey build with the second patch) that
creating a new account crashes the app. This didn't happen with the first patch,
IIRC.

Comment 21

[Alec Flett]

Attachment: parse identity keys, and really save them correctly

ok, the previous patch had two flaws in it
1) it would pass empty identity key strings to nsCRT::strtok(), which would
assert and crash on an empty string. Fixed.
2) It would fail to save the identity key of new accounts. I had mixed up
"newIdentityKey" with "identityKey" towards the end.

I tested composition with mulitiple identities, and created some accounts,
without any problems.

I'm now looking for reviewers.. this patch should be ready to go. Anyone want
to test it some more too?

Comment 22

[Alec Flett]

Comment on attachment 136801 [details] [diff] [review]
parse identity keys, and really save them correctly

Here's the updated version of the patch, hopefully the last one.. scott, do you
mind reviewing? obviously this would be for moz1.7

Comment 23

[Holger Mauermann]

Patch is working for me without problems! I think it's ready for check in :-)

Comment 24

[Alec Flett]

oh, also see bug 18906, which is about automatically choosing your identity
based on the currently displayed message. This becomes more relevant when we
have multiple identities per account working.

Comment 25

[Scott MacGregor]

Comment on attachment 136801 [details] [diff] [review]
parse identity keys, and really save them correctly

plussing for 1.7 trunk. 

I may be more agressive about taking this into thunderbird on a private branch
assuming I can get some UI cribbed up.

Comment 26

[Alec Flett]

I feel pretty good about the patch, I guess I'd be slightly concerned about
callers of nsIMsgAccount::GetIdentities() in case they are counting on just one
identity. But it would probably be easy just to find those by search/inspection.

Comment 27

[Christian Eyrich]

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

Comment 28

[Scott MacGregor]

Alec, just so I understand what this gives us. This means I can hack my prefs
such that if I have a single account that gets mail from two addresses such as:

[e-mail]
and
[e-mail]

I can now add an identity for each one. Now in the mail compose window, the drop
down list will automatically show all the identies for all accounts. So I will
now have an option for selecting both of these identites instead of just the one
identity I normally associate with the account?

That is very cool!

And as you said the next step would be to get Bug #18906 going next so the user
does not have to manually pick the identity.

I'll try putting this in the next tbird nightly so more folks can try it out.

If someone writes together a nice blurb summarizing all of this I'll make sure
it shows up on the help site (http://texturizer.net/thunderbird/)

Comment 29

[Scott MacGregor]

FYI: I just posted a test win32 thunderbird build with this fix and a fix i made
up for Bug #18906 which scans through the email addresses of the message being
replied/forwarded and if it finds an email address that matches one of your
multiple identities for the account, it makes the compose window use that
identity by default.

You can find this build here:

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/2003-12-16-trunk/

Comment 30

[Alec Flett]

yep, that's exactly what this bug gets you.. as soon as the tree opens for 1.7,
I'll land this.

Comment 31

[Scott MacGregor]

Alec, I hope you don't mind, I just took the liberty of checking this in along
with my fix for Bug #18906. 

Initial feedback from my test build of thunderbird with this patch has been great!