318495 - Memo labels to distinguish different identities in one account

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, enhancement)

Description

[Raoul Nakhmanson-Kulish]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.7.12) Gecko/20050915
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.7.12) Gecko/20050915

I have two identities for one mail account.

When I compose a letter, I may choose needed identity from a combobox. But if I have two (or more) identities with the same Name and E-mail fields (difference may be in reply-to, signature, SMTP server etc.), I have no way to distinguish them, they look identically.

I would have a field to display in combobox and to distinguish entries with identical names and e-mails.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Mike Cowperthwaite]

I've encountered the same problem; this is a valid request, and I can't find a dupe, so confirming.

Comment 3

[chris]

How would you propose distinguishing the (apparently identical) identities?

Comment 4

[Tom Metro]

(In reply to comment #3)
> How would you propose distinguishing the (apparently identical) identities?

The original requester suggested:

> I would have a field to display in combobox and to distinguish entries with
> identical names and e-mails.

In other words an alias or nickname field would be added to the identity dialog. (One of the other bugs actually mentioned the term "alias.")

I expect in most cases it would be unnecessary and unused, so the code presenting the list of identities would have to handle it being blank, and considered supplemental information to what is displayed now (address, name, account).

Comment 5

[Dave Belfer-Shevett]

I was the originator of 360927.  I suggested an alias, a tag, just a simple write-in field that identified the identity.  It should default to the email address being used, but can be changed.

Comment 6

[Andrea Soster]

Bug #493388 looks a dupe of this, is there any chance of having an alias for different identities with same name/email?

Comment 8

[:aceman]

I think this should be doable for me (depends how hard it gets wiring the new attribute into the backend).

Bwinton, would it be accepted?
The visible changes would be:
1. new "Identity label" field in AM/default identity groupbox
2. the same field in the Settings tab of the dialog that is opened via Edit in the Manage identities list.
3. When composing mail, the From dropdown will show the new Label in addition to Name, Email and Account that already are there. The ordering of the items could be discussed.

Comment 9

[:aceman]

4. the label could be shown in the Manage identities dialog.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

I like it, with the following modifications:

1a) The "Identity label" defaults to "Name <Email Address>", but the user can change it to whatever they want.

3) When composing email, the From dropdown will show "Label <i>account</i>".

If the user wants it to show the name and email address, they can put those in the label.  ;)

Thanks,
Blake.

Comment 11

[:aceman]

So you want to allow the user to remove email address from the display (in composition) if the user wishes so?

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Sure, if the user can see the identity label they chose, that seems like it will be enough information.

Comment 13

[:aceman]

Hopefully it is not too much of a foot gun :)

But OK, it is similar to the account name field.

Comment 14

[:aceman]

I'm going to work on this now.
CCing compose window gyus so they are aware of this. Also the Seamonkey compose window is in /suite so I'll probably not be able to code that part.

Comment 15

[Magnus Melin [:mkmelin]]

I'm not sure i like adding a Label in the Identity settings | Settings. Everything there is things the recipient will see, so a setting for the application only is confusing. Also people having just one identity shouldn't have to set it, it's not something they need so we should hide is best we can.

I'd propose the nickname would be only in the Identity Settings dialog, on top

That could be 

+-------------------------------------------------------------------------------------
|
|  Configure identity settings for Default identity:           [Identity Nickname...]
|
| | Settings | Copies & Folders | Composition & Addressing | Security |
|
|...............


I also think the composition identity picker should remain as is and always show name and address, the nick should only be added there after the account name. Don't let users shoot themselves in the foot - there's always someone changing it to something wrong and then forgets about what it was.

Comment 16

[:aceman]

Bwinton?

Comment 17

[:aceman]

So Magnus's wish seems to be the same as my comment 8. The Label is blank by default (nicely accommodates existing profiles). Only if the user fills it in, it is tacked to the end of the menuitems in the compose window dropdown. Also maybe to the identity list editor.

On the other hand, see this:

http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsIMsgIdentity.idl#26
23    * Overriding display name for this identity. If this pref is not set
24    * then this will return some composed string from the fullname and email.
25    */
26   attribute AString identityName;

and this:
http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgIdentity.cpp#67

So there already is support for identityName, it is just hidden from the UI. It seems various import modules from other clients set it. And it behaves in the way bwinton wants it:
If the name is set, display that. If it is not, display "name <email>".

It look like we just need to expose this pref in the UI...

Comment 18

[Blake Winton (:bwinton) (:☕️)]

I'm not entirely sure what information you need from me.  It sounds like you've got a good way forward, so I think the next step is to whip up a patch that exposes the pref, and ask for ui-review.  :)

Thanks,
Blake.

Comment 19

[:aceman]

The problem is you and Magnus want conflicting results ;) I can probably code any one, but first need decision which one I should try.

Comment 20

[Blake Winton (:bwinton) (:☕️)]

So having it be blank by default seems reasonable, since that gives us the behaviour I'm looking for.  And it seems like the existing behaviour for showing it is what I want, so I suggest going with that.  ;)

(But keep in mind that I might change my mind to match Magnus's opinion after playing with the patch for a while.)

Comment 21

[:aceman]

Attachment: WIP patch

Comment 22

[:aceman]

I do not like that the label field is in the global header in the identity dialog thus making it even higher now. It is already high enough and we don't want it to become higher than the account manager itself. I'd like it to be somewhere in the free space of the Settings tab. But I agree with Magnus that it should be explicitly noted, that this information is not exposed to the recipient.

Comment 23

[:aceman]

Standard8, as you are the peer of the Import module which is the only user of SetIdentityName(), can you tell us if it is strictly necessary to preserve the current functionality, that the identity name found by the import completely overrides the "name <email>" display when retrieving the identityName attribute? We are not sure what content actually comes from the imported data and what gets put into the identityName pref (attribute).

Currently only the import code sets identityName. We now try to expose the attribute to all users via the account manager.
Then suppose all of identityName, fullName and email are set.
In the patch I suggest to rename the real identityName attribute to Label (but keep "identityName" as the pref name for existing users) and use .identityName only as a getter working as .prettyName works for other objects.

The options for result of .identityName are as follows:
1. Label (I think this was what bwinton says and also is the current implementation)
2. name <email> (Label) (I think this is Magnus' preference and I like this too)
3. Label (name <email>)
4. ... (some other permutation)

If label is empty (for most users), then nothing changes and .identityName returns "name <email>".

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 689391 [details] [diff] [review]
WIP patch

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

I think this looks of so far. It's not so nice that the identity dialog gets even taller, but i don't think there's much to do about it if we want the label in there.

::: mailnews/base/prefs/content/am-identity-edit.xul
@@ +47,5 @@
>    <separator class="thin"/>
>  
> +  <hbox align="center">
> +    <label value="Identity label:" control="identity.label" accesskey=""/>
> +    <textbox id="identity.label" size="30" placeholder="(optional)"/>

The placeholder should be the "Foo Bar <[e-mail]>"

Comment 25

[:aceman]

(In reply to Magnus Melin from comment #24)
> It's not so nice that the identity dialog gets
> even taller, but i don't think there's much to do about it if we want the
> label in there.

We can still put it into the first pane.

> The placeholder should be the "Foo Bar <[e-mail]>"

I think that depends on what we make .identityName return.

Comment 26

[Ian Neal]

Comment on attachment 689391 [details] [diff] [review]
WIP patch

To stop the identity dialog getting any taller, can the label just go on the Settings tab?

Comment 27

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 689391 [details] [diff] [review]
WIP patch

I agree that we don't want that top area to get much bigger.

I would be happy with either putting the Alias in the first pane, or with something like https://dl.dropbox.com/u/2301433/Screenshots/IdentityAlias.png

(Other than that, I'm happy with the patch. :)

Thanks,
Blake.

Comment 28

[:aceman]

Attachment: WIP patch v2

So this one has better placement and grouping but we still need answer to comment 23.

Comment 29

[Mark Banner (:standard8)]

(In reply to :aceman from comment #23)
> Standard8, as you are the peer of the Import module which is the only user
> of SetIdentityName(), can you tell us if it is strictly necessary to
> preserve the current functionality, that the identity name found by the
> import completely overrides the "name <email>" display when retrieving the
> identityName attribute? We are not sure what content actually comes from the
> imported data and what gets put into the identityName pref (attribute).

I think you may need to preserve some of that behaviour - it is currently unclear to me as to if the import code does in all cases put the correct email address in the identity, so if we've not got the correct email address, there will be some fake string in there which wouldn't look good.

Comment 30

[:aceman]

OK, so if email is empty, we can still show just "name" (=label) as the .identityName, as it was before.

Comment 31

[:aceman]

I see the import code sometimes puts placeholder string "[e-mail]" as the email. Does anything break if the email would be empty? Or we could special-case [e-mail] in getIdentityName and not show it.

Comment 32

[:aceman]

Attachment: patch v3

Comment 33

[Ian Neal]

Comment on attachment 706679 [details] [diff] [review]
patch v3


>+++ b/mailnews/base/prefs/content/am-identity-edit.xul
>@@ -49,18 +49,21 @@
>   <tabbox>
>     <tabs id="identitySettings">
>       <tab label="&settingsTab.label;"/>
>       <tab label="&copiesFoldersTab.label;"/>
>       <tab label="&addressingTab.label;"/>
>     </tabs>
> 
>     <tabpanels id="identityTabsPanels" flex="1">
>+      <!-- Identity Settings Tab -->
>       <vbox flex="1" name="settings">
>+        <groupbox>
>+          <caption label="Public data"/>
I assume you will be turning this into an entity?
>+
>+        <groupbox>
>+          <caption label="Private settings"/>
And this?

I'm not 100% convinced those are the best names for the two captions or whether captions are needed.
I'll defer to bwinton on that though.
Everything seems to work fine.
r=me

Comment 34

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 706679 [details] [diff] [review]
patch v3

>+++ b/mailnews/base/prefs/content/am-identity-edit.xul
>@@ -49,18 +49,21 @@
>+          <caption label="Public data"/>
>@@ -110,29 +113,42 @@
>+          <caption label="Private settings"/>

So, I kind of like these labels.  I probably wouldn't have come up with them, but they kinda make sense to me…

It would be nice to use the same word in both places, though.

>+          <hbox align="center">
>+            <label value="Identity label:" control="identity.label" accesskey="a"/>
>+            <textbox id="identity.label" size="30" placeholder="(optional alias)"/>
>+          </hbox>

And this isn't quite right, since it's not really a label or an alias anymore, it's more of an extra identifier that gets tacked on to the end.

I still feel like the resulting text is _way_ too long, (see, for example, https://dl.dropbox.com/u/2301433/Screenshots/LongFromLine.png ) and we could easily get away with just showing the label/alias if there is one, and if people wanted all that information they could just type it in to the alias.  If we did that, we could keep using the word "alias".  But having said that, we should make the placeholder whatever the .identityName returns, so we might not show the word "alias" at all…

So, I'm going to say ui-r-, but it will get a successful ui-review if you make the following three changes:
1) Wherever we display the alias, we should only display the alias.  (The compose window, and the identity selector screen are the two I've seen so far.)
2) We make the placeholder text whatever the .identityName would return for an empty label (which I believe to be "name <email>").
3) We change the label's value to "Identity Alias:", to match the capitalization of the other items.

Thanks,
Blake.

Comment 35

[Mark Banner (:standard8)]

(In reply to :aceman from comment #31)
> I see the import code sometimes puts placeholder string
> "[e-mail]" as the email. Does anything break if the email would
> be empty? Or we could special-case [e-mail] in getIdentityName
> and not show it.

I don't have enough knowledge of why this was done, or if it would break anything if it was changed. I guess the best thing would be to test it out and see if there's anything affected.

Comment 36

[Mark Banner (:standard8)]

Comment on attachment 706679 [details] [diff] [review]
patch v3

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

I'm guess I'm fine with the general idea here, but someone really needs to test this on at least outlook and hopefuly Eudora to check this doesn't break anything.

Comment 38

[:aceman]

(In reply to Blake Winton (:bwinton) from comment #34)
> 2) We make the placeholder text whatever the .identityName would return for
> an empty label (which I believe to be "name <email>").
This could be nice, but seem quite hard to do. We ideally pull the string out by calling identity.identityName . But that only works if identity even exists yet (which is not true for new identities being set up) AND the identity already has the correct data stored inside it (which is not true until we click the OK button. So we have to duplicate the logic of creating the "fullname <email>" string in the editor, which I do not like. But let's try it.

Comment 39

[Blake Winton (:bwinton) (:☕️)]

Could we refactor the code a little to call the same logic from both places?
If it's too hard, we can _probably_ do without it, but I'ld like to see how much extra code it takes before we get to that point.

Thanks!  :)

Comment 40

[:aceman]

Refactoring will probably need more code than the duplicated function in JS:) As it would need to live outside of nsIMsgIdentity.

On the other hand I seem to like the "Alias replaces 'name <email>' if set" idea. In that case I think we can leave the setting of identityName (thus alias) in the import code as there would be no more duplicate display (the import code often sets fullname and identityName to the same value). Then we do not need to wonder and test what the import code actually does and why. That will move us forward here :) I prepare a new patch shortly.

Comment 41

[:aceman]

Attachment: patch v4

So this collects all of bwinton's wishes and my further comments.

Comment 42

[Blake Winton (:bwinton) (:☕️)]

Just as a note, I tried ui-reviewing this today, but c-c doesn't compile currently…  :(

Comment 43

[:aceman]

Bug 1152045 will change the identity display in compose window so mark the dependency that I need to fix it up after that.

Comment 44

[:aceman]

Let's not forget some test :)

Comment 45

[:aceman]

Attachment: patch v5 (with test)

This adds some tests whether the identity displays correctly in compose window according to bwinton's specification. (For the tests to work, apply also patch in bug 1152045).

Comment 46

[:aceman]

Attachment: patch v5.1 + test

Some small fixes in the patch.

Comment 47

[:aceman]

Attachment: patch v5.2 + test

Updated some more places that assumed always .identityName returns "name <email>". Does anybody think we should make a new property of nsIMsgIdentity that returns this string? :) It could be handy at some places.

Comment 48

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8596168 [details] [diff] [review]
patch v5.2 + test

So, I'm wondering now if the email address should be part of the identityName in the compose screen.
https://www.dropbox.com/s/0mwu3lk8r5d6tf5/Screenshot%202015-05-03%2015.58.07.png?dl=0 still looks really long, and has needlessly duplicated information…
ui-r=me if we can get it to only show the email address once.  ;)

Comment 49

[:aceman]

If you mean the grey email address in italics, then that is your account name. That one is intentionally there and should not be removed. You can rename it if you do not like the duplication. Actually people using multiple identities per account will probably call the account with a generic name, not with one of their emails from one identity.

Comment 50

[Blake Winton (:bwinton) (:☕️)]

Yes, what I'm suggesting is that we remove the other one from the default if you haven't specified an alias.
(By default, we shouldn't show duplicated information.  I know I can rename it, but that doesn't justify having a bad experience out of the box. ;)

Comment 51

[:aceman]

That may become quite inconsistent for the user, some of the menuitems having the gray text and some not. I'd like to hear what mkmelin thinks about that.

And all this "de-duplication" may become quite expensive and complicated. How do you propose for it to work? Do we need to decide whether the account name is an email address? And then check if that same string appears in the rendered identity name?

Comment 52

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #49)
> If you mean the grey email address in italics, then that is your account
> name. That one is intentionally there and should not be removed. You can
> rename it if you do not like the duplication. Actually people using multiple
> identities per account will probably call the account with a generic name,
> not with one of their emails from one identity.

I agree with aceman that we're not here to remove the account name information.
It's not really duplicated information: the first email address is what the sender is going to see (and I hope it will remain like that even after this bug), the second one tells you which account the sending address belongs to. If your account name happens to be an email address - than that's how it is. That looks like important information because it has an influence on other things like FCC etc. It's also a good indicator if you've started out from the right account or not. If we remove the account name, and perhaps my identity has another email address than the main account address, users will have no way of telling which account they are using for sending.

Comment 53

[Blake Winton (:bwinton) (:☕️)]

Okay, I'm still not convinced that we're doing the right thing for most of our users here, but if anyone else finds it annoying they can file a followup bug.

Comment 54

[:aceman]

(In reply to Blake Winton (:bwinton) from comment #53)
> Okay, I'm still not convinced that we're doing the right thing for most of
> our users here, but if anyone else finds it annoying they can file a
> followup bug.

Yes, the problem you describe is there also without my patch. So it is not a regression. It is a possible improvement but not strictly related to this bug.

Comment 55

[:aceman]

Comment on attachment 8596168 [details] [diff] [review]
patch v5.2 + test

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

mkmelin, please see TB UI and mozmill part.
Iann, please check account manager and SM UI part.
Neil, please check the identity backend and import part.
Thanks.

Comment 56

[[e-mail]]

Comment on attachment 8596168 [details] [diff] [review]
patch v5.2 + test

>+  // An identity with no name or email shows as [key].
>+  if (!params.identity ||
>+      params.identity.identityName == "[" + params.identity.key + "]") {
Why not just test the name and email directly?

>+    let currentAddress = gCurrentIdentity ? MailServices.headerParser.makeMailboxObject(
>+      gCurrentIdentity.fullName, gCurrentIdentity.email).toString() : null;
>+
>+    addRecipientsToIgnoreList(
>+      (currentAddress ? currentAddress + ", " : "") +
>+      msgTo + ", " + msgCC + ", " + msgBCC);
If you use "" instead of null then you can just concatenate everything together.

>+    // This field does not exist for the default identity shown in the am-main.xul pane.
>+    let idLabel = document.getElementById('identity.label')
>+    if (idLabel)
>+      idLabel.value = identity.label;
...
> +  updateAlias();
Only bother updating the alias if the label exists.

>+function updateAlias()
>+{
>+  let identityLabel = document.getElementById("identity.label");
>+  if (!identityLabel || identityLabel.value)
The label should always exist at this point, and its value is irrelevant, the placeholder attribute takes care of that.

>-              <textbox id="identity.fullName" size="30"/>
>+              <textbox id="identity.fullName" size="30" onchange="updateAlias();"/>
>             </row>
>             <row align="center">
>               <label value="&email.label;" control="identity.email" accesskey="&email.accesskey;"/>
>-              <textbox id="identity.email" class="uri-element"/>
>+              <textbox id="identity.email" class="uri-element" onchange="updateAlias();"/>
oninput perhaps?

>+            <textbox id="identity.label" placeholder="" flex="1" onchange="updateAlias();"/>
onchange makes no sense here. Nit: don't need to specify an empty placeholder.

>-  attribute AString identityName;
>+  attribute AString label;
>+
>+  /**
>+   * Pretty display name for this identity. Will return some composed string
>+   * depending on fullname, email and label.
>+   */
>+  readonly attribute AString identityName;
So, if we don't actually use identityName any more, why do we need to rename it?

>+  idName.AssignLiteral("");
You don't need this, but if you did, it would be Truncate().

>+  // If a non-empty label exists, use that as the identity name.
>+  nsString identityName;
>+  nsresult rv = GetLabel(identityName);
>+  if (NS_SUCCEEDED(rv) && !identityName.IsEmpty()) {
>+    idName.Assign(identityName);
>+    return NS_OK;
Nit: you could simply pass idName to GetLabel directly.

>+  if (NS_SUCCEEDED(rv) && email.Equals("[e-mail]"))
Why this check? I'm not exactly sure what import is doing here, but it deletes the identity anyway, so I don't see how it matters.

>+  // If we still found nothing to use, use our key.
Possibly just call ToString?

>-  //BUG 470587. Don't set this: id->SetIdentityName(fullName);
>+  //BUG 470587. Don't set this: id->SetLabel(fullName);
...
>-      id->SetIdentityName(name);
>+      id->SetLabel(name);
Someone should decide what's correct here. (My preference is for bug 470587.)

>+<!ENTITY publicData.label        "Public Data">
>+<!ENTITY privateData.label       "Private Data">
>+<!ENTITY identityAlias.label     "Optional Identity Alias:">
[Not keen on the wording here but then again you already got ui-r.]

Comment 57

[Ian Neal]

Comment on attachment 8596168 [details] [diff] [review]
patch v5.2 + test

>+++ b/mail/components/compose/content/addressingWidgetOverlay.js
>+    addRecipientsToIgnoreList(
>+      (currentAddress ? currentAddress + ", " : "") +
>+      msgTo + ", " + msgCC + ", " + msgBCC);

Would this formatting look better?
>+    addRecipientsToIgnoreList((currentAddress ? currentAddress + ", " : "") +
>+                               msgTo + ", " + msgCC + ", " + msgBCC);

>+<!ENTITY identityAlias.label     "Optional Identity Alias:">
Would "Identity Alias (optional):" be better?

>+++ b/mailnews/base/prefs/content/am-identity-edit.js

>+    // This field does not exist for the default identity shown in the am-main.xul pane.
>+    let idLabel = document.getElementById('identity.label')
Nit: Missing ;

> function saveIdentitySettings(identity)
> {
>   if (identity)
>   {
>+    let idLabel = document.getElementById('identity.label')
Nit: Missing ;

>+++ b/mailnews/base/prefs/content/am-identity-edit.xul

>+        <groupbox>
>+          <caption label="&privateData.label;"/>
>+
>+          <hbox align="center">
>+            <label value="&smtpName.label;" control="identity.smtpServerKey"
>+                   accesskey="&smtpName.accesskey;"/>
Nit: One attribute per line.

>+++ b/suite/mailnews/compose/addressingWidgetOverlay.js

>+    addRecipientsToIgnoreList(
>+      (currentAddress ? currentAddress + ", " : "") +
>+      msgTo + ", " + msgCC + ", " + msgBCC);
Same comment as before about formatting.

Only f+ as there are Neil's comments to address as well.

Comment 58

[:aceman]

(In reply to Ian Neal from comment #57)
> >+<!ENTITY identityAlias.label     "Optional Identity Alias:">
> Would "Identity Alias (optional):" be better?
Even some other fields in the dialog are optional (e.g. organization) and it is not explicitly said in the label, so we could just drop that word.

Other nits appreciated and fixed.

Comment 59

[:aceman]

(In reply to [e-mail] from comment #56)
> >+function updateAlias()
> >+{
> >+  let identityLabel = document.getElementById("identity.label");
> >+  if (!identityLabel || identityLabel.value)
> The label should always exist at this point, and its value is irrelevant,
> the placeholder attribute takes care of that.
If label value is not empty, we can skip updating the placeholder. So it is not irrelevant.

> >-  attribute AString identityName;
> >+  attribute AString label;
> >+
> >+  /**
> >+   * Pretty display name for this identity. Will return some composed string
> >+   * depending on fullname, email and label.
> >+   */
> >+  readonly attribute AString identityName;
> So, if we don't actually use identityName any more, why do we need to rename
> it?
We use it. In compose identity selection and in identity list.

> >-  //BUG 470587. Don't set this: id->SetIdentityName(fullName);
> >+  //BUG 470587. Don't set this: id->SetLabel(fullName);
> ...
> >-      id->SetIdentityName(name);
> >+      id->SetLabel(name);
> Someone should decide what's correct here. (My preference is for bug 470587.)
I don't change this in any way so no need to decide it here.

Thanks, fixed the other problems.

Comment 60

[:aceman]

Attachment: patch v6 + test

Fixed all the nits so far, except those explained.

Comment 61

[:aceman]

Attachment: screenshot of the new identity edit dialog

Comment 62

[:aceman]

Attachment: screenshot of identity list with aliases active

This is also how it will look in compose identity picker (the From menulist). Plus account name appended in italics. I think bwinton posted an image of that.
If the user wants a label and also an email address to be displayed, he can put it all in the alias.

Comment 63

[[e-mail]]

(In reply to aceman from comment #59)
> (In reply to comment #56)
> > >+function updateAlias()
> > >+{
> > >+  let identityLabel = document.getElementById("identity.label");
> > >+  if (!identityLabel || identityLabel.value)
> > The label should always exist at this point, and its value is irrelevant,
> > the placeholder attribute takes care of that.
> If label value is not empty, we can skip updating the placeholder. So it is
> not irrelevant.
But you are duplicating effort that the placeholder attribute already handles, and then adding extra effort to see whether the value becomes empty again. (But I would accept a change back to onchange instead of oninput if you really want to reduce the effort.)

> > So, if we don't actually use identityName any more, why do we need to rename
> > it?
> We use it. In compose identity selection and in identity list.
I'm really tempted to suggest switching the callers to use .identityName || .fullAddress || .toString

> > Someone should decide what's correct here. (My preference is for bug 470587.)
> I don't change this in any way so no need to decide it here.
Well, I wouldn't want you to change incorrect code incorrectly if you could avoid it.

Comment 64

[[e-mail]]

Comment on attachment 8604892 [details] [diff] [review]
patch v6 + test

>+  let idLabel = document.getElementById("identity.label");
>   if (identity)
>   {
>+    // This field does not exist for the default identity shown in the am-main.xul pane.
>+    if (idLabel)
>+      idLabel.value = identity.label;
>     document.getElementById('identity.fullName').value = identity.fullName;
>     document.getElementById('identity.email').value = identity.email;
...
>+  if (idLabel)
>+    updateAlias();
Actually I was thinking you could do all that inside the if (identity) block. (And if you move the label setting code further down, you can put the alias update code in the same if.)

>+  // Try to use "fullname <email>" as the name.
>+  nsAutoString fullAddress;
>+  rv = GetFullAddress(fullAddress);
>+  if (NS_SUCCEEDED(rv)) {
>+    idName.Assign(fullAddress);
>+    fullAddress.StripChars(" <>");
>+    if (!fullAddress.IsEmpty())
>+      return NS_OK;
IMHO GetFullAddress should return the empty string when there is no name or email, instead of checking for " <>" here.

Comment 65

[:aceman]

(In reply to [e-mail] from comment #63)
> > > So, if we don't actually use identityName any more, why do we need to rename
> > > it?
> > We use it. In compose identity selection and in identity list.
> I'm really tempted to suggest switching the callers to use .identityName ||
> .fullAddress || .toString
I would not want to do that. Some caller will surely forget to call some of the properties.

> > > Someone should decide what's correct here. (My preference is for bug 470587.)
> > I don't change this in any way so no need to decide it here.
> Well, I wouldn't want you to change incorrect code incorrectly if you could
> avoid it.
I do not understand what bug 470587 is about (except just somehow distinguish identities with same name). So what is your "preference for" ? The user says something about some patches but none are visible. And then the bug got duped here. So maybe we should just remove the mentions of that bug from the code?

Comment 66

[[e-mail]]

(In reply to aceman from comment #65)
> (In reply to comment #63)
> > > > So, if we don't actually use identityName any more, why do we need to rename
> > > > it?
> > > We use it. In compose identity selection and in identity list.
> > I'm really tempted to suggest switching the callers to use .identityName ||
> > .fullAddress || .toString
> I would not want to do that. Some caller will surely forget to call some of
> the properties.
Maybe we should call the new property .prettyName and keep identityName as the property that tracks the preference.

> > > > Someone should decide what's correct here. (My preference is for bug 470587.)
> > > I don't change this in any way so no need to decide it here.
> > Well, I wouldn't want you to change incorrect code incorrectly if you could
> > avoid it.
> I do not understand what bug 470587 is about (except just somehow
> distinguish identities with same name). So what is your "preference for" ?
> The user says something about some patches but none are visible. And then
> the bug got duped here. So maybe we should just remove the mentions of that
> bug from the code?
I only mentioned bug 470587 because that's what I quoted in comment #56, but basically some of the import wizards set the identity name to the user name, which may not be ideal.

Comment 67

[:aceman]

(In reply to [e-mail] from comment #66)
> Maybe we should call the new property .prettyName and keep identityName as
> the property that tracks the preference.
Sure, that is a possibility. But all callers and extensions would need to be updated.
Will you do that decision?

> > I do not understand what bug 470587 is about (except just somehow
> > distinguish identities with same name). So what is your "preference for" ?
> > The user says something about some patches but none are visible. And then
> > the bug got duped here. So maybe we should just remove the mentions of that
> > bug from the code?
> I only mentioned bug 470587 because that's what I quoted in comment #56, but
> basically some of the import wizards set the identity name to the user name,
> which may not be ideal.
Yes, maybe because that is how the original clients behave? We do not have the expertise now to check that import code. But thew patch here offers the user the possibility to override the alias to whatever he wants. Before it, the user was stuck with what the import code forced on him. So what should I do now? Is there anything missing?

Comment 68

[[e-mail]]

(In reply to aceman from comment #67)
> (In reply to comment #66)
> > Maybe we should call the new property .prettyName and keep identityName as
> > the property that tracks the preference.
> Sure, that is a possibility. But all callers and extensions would need to be
> updated.
> Will you do that decision?
As there may well be extensions out there who assume the identityName is the fullName, I'll agree with what you currently have.

> patch here offers the
> user the possibility to override the alias to whatever he wants. Before it,
> the user was stuck with what the import code forced on him.
Fair enough.

> So what should I
> do now? Is there anything missing?
Just comment #64, I guess.

Comment 69

[:aceman]

Attachment: patch v6.1 + test

Thanks. Then this should be it.

Comment 70

[[e-mail]]

Comment on attachment 8606529 [details] [diff] [review]
patch v6.1 + test

>-  // " <>" is an empty identity, and most likely not valid
>-  if (!params.identity || params.identity.identityName == " <>") {
>+  // An identity with no email is likely not valid.
>+  if (!params.identity || !params.identity.email) {
(Should this use .fullAddress? Probably doesn't make much difference.)

Comment 71

[:aceman]

(In reply to [e-mail] from comment #70)
> (Should this use .fullAddress? Probably doesn't make much difference.)

Would "name <>" or just "name" be usable in any way? Yes, this is our sending address, not the recipients. But will SMTP servers accept it? Is it correct to send out mail with no sender address? We also do not allow empty email in the identity editor.

Comment 72

[Ian Neal]

Comment on attachment 8606529 [details] [diff] [review]
patch v6.1 + test

>+++ b/mailnews/base/prefs/content/am-identity-edit.js

>     document.getElementById('identity.attachVCard').checked = identity.attachVCard;
>     document.getElementById('identity.escapedVCard').value = identity.escapedVCard;
>     initSmtpServer(identity.smtpServerKey);
>+
>+    // This field does not exist for the default identity shown in the am-main.xul pane.
>+    let idLabel = document.getElementById("identity.label");
Nit: ' is used elsewhere in the file rather than "

>+function updateAlias()
>+{
>+  let name = document.getElementById("identity.fullName").value;
>+  let email = document.getElementById("identity.email").value;
>+  let idName = name || email ? MailServices.headerParser.makeMailboxObject(name, email).toString() : "";
>+  document.getElementById("identity.label").setAttribute("placeholder", idName);
Nit: ' is used elsewhere in the file rather than "

Comment 73

[:aceman]

(In reply to Ian Neal from comment #72)
> Nit: ' is used elsewhere in the file rather than "

I am sure mkmelin would tell me to convert to " where possible :) Or does that only count in /mail files?

Comment 74

[Magnus Melin [:mkmelin]]

Comment on attachment 8606529 [details] [diff] [review]
patch v6.1 + test

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

::: mail/test/mozmill/composition/test-drafts.js
@@ +46,5 @@
>    be_in_folder(draftsFolder);
>    let draftMsg = select_click_row(0);
>  
> +  // Wait for the notification with the Edit button.
> +  mc.sleep(0);

needed? looks error prone if so

::: mailnews/base/prefs/content/am-identity-edit.js
@@ +174,5 @@
>  {
>    if (identity)
>    {
> +    let idLabel = document.getElementById('identity.label');
> +    if (idLabel)

maybe comment that this is for the "main page"

@@ +395,5 @@
> +{
> +  let name = document.getElementById("identity.fullName").value;
> +  let email = document.getElementById("identity.email").value;
> +  let idName = name || email ? MailServices.headerParser.makeMailboxObject(name, email).toString() : "";
> +  document.getElementById("identity.label").setAttribute("placeholder", idName);

I think the placeholder functionality isn't quire right. 

Initially placeholders should be descriptions - it should say something like
"Label for this identity in the composition window"

When you click on it to edit it should fill in the full address as default. (And on leaving remove it if you didn't change it.)

Comment 75

[:aceman]

(In reply to Magnus Melin from comment #74)
> > +  // Wait for the notification with the Edit button.
> > +  mc.sleep(0);
> needed? looks error prone if so
Yes, I do not like it either. Any better idea?

> I think the placeholder functionality isn't quire right. 
> 
> Initially placeholders should be descriptions - it should say something like
> "Label for this identity in the composition window"
I wanted for the <label> before the label inputbox to indicate this use. Then there is no need to copy it again into the placeholder. And it isn't only for composition window.

> When you click on it to edit it should fill in the full address as default.
No, we can't do this. We do not want to hardcode a string that will be created automatically if label is empty. Also if the email changes, the hardcoded label would override it to the old outdated value.

Comment 76

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #75)
> (In reply to Magnus Melin from comment #74)
> > > +  // Wait for the notification with the Edit button.
> > > +  mc.sleep(0);
> > needed? looks error prone if so
> Yes, I do not like it either. Any better idea?

I think there's a helper for waiting for notification bars to come up. 

> > I think the placeholder functionality isn't quire right. 
> > 
> > Initially placeholders should be descriptions - it should say something like
> > "Label for this identity in the composition window"
> I wanted for the <label> before the label inputbox to indicate this use.
> Then there is no need to copy it again into the placeholder. And it isn't
> only for composition window.

Where else would it be used?

> > When you click on it to edit it should fill in the full address as default.
> No, we can't do this. We do not want to hardcode a string that will be
> created automatically if label is empty. Also if the email changes, the
> hardcoded label would override it to the old outdated value.

Yes, but I said if not changed from default, on blur you could just reset it.

Comment 77

[Magnus Melin [:mkmelin]]

Some more thoughts that came to mind now:
 1) as is this is a pretty heavy gun for people to shoot themselves in the foot with, and it bound to be problematic for support people (if and whey people change it to something slightly unexpected)
 2) I don't see a use case where the full email wouldn't be included in the label. It's useful only to distinguish identities with similar same emails but different settings. Having a kind of "note" in parenthesis after the full address would serve this purpose. Something like "John Doe <[e-mail]> (with signature)"

Comment 78

[:aceman]

I'd be fine with that. I know you already wanted this behaviour in comment 15. But you'll need to overturn all the UI decisions that requested the behaviour as implemented (starting comment 34).

Comment 79

[:aceman]

(In reply to Magnus Melin from comment #77)
> Some more thoughts that came to mind now:
>  1) as is this is a pretty heavy gun for people to shoot themselves in the
> foot with, and it bound to be problematic for support people (if and whey
> people change it to something slightly unexpected)
>  2) I don't see a use case where the full email wouldn't be included in the
> label. It's useful only to distinguish identities with similar same emails
> but different settings. Having a kind of "note" in parenthesis after the
> full address would serve this purpose. Something like "John Doe
> <[e-mail]> (with signature)"

(In reply to :aceman from comment #78)
> I'd be fine with that. I know you already wanted this behaviour in comment
> 15. But you'll need to overturn all the UI decisions that requested the
> behaviour as implemented (starting comment 34).

Bwinton, what can we do now?

Comment 80

[Magnus Melin [:mkmelin]]

I agree with Blake duplicating info isn't that nice. But this bug isn't making it worse... and fixing it to the limited way having a fully editable label the would fix it creates more problems than it fixes.

Comment 81

[:aceman]

I'm not sure bwinton will respond here anytime soon;)

Comment 82

[Richard Marti (:Paenglab)]

What about a compromise? Let the address always show and the Alias exchanges only the name? Then Magnus's proposed line could look like: "John Doe (with signature) <[e-mail]>", where the user has to fill in the alias field "John Doe (with signature)".

Like this the user can set everything he wants in the alias but still sees with which mail address he's sending.

Comment 83

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #82)
> What about a compromise? Let the address always show and the Alias exchanges
> only the name? Then Magnus's proposed line could look like: "John Doe (with
> signature) <[e-mail]>", where the user has to fill in the alias
> field "John Doe (with signature)".
> 
> Like this the user can set everything he wants in the alias but still sees
> with which mail address he's sending.

Sorry I do not understand the proposal here. The "John Doe" name is public and sent as part of the address in the From header. The "(with signature)" alias is private into set by the user for his needs. How do you mean to merge these?

Comment 84

[Richard Marti (:Paenglab)]

I meant the alias is only shown in the From: field but for sending the real From: address is used. Then the user sees his local alias and the receiver sees the standard From:.

But this could be now more complicated because of the editable From: field.

Comment 85

[:aceman]

Then what is the difference to Magnus' proposal? Only that the alias is after real name, not after the email part?

Comment 86

[Richard Marti (:Paenglab)]

The whole alias is before the email part and it will be exchanged on send with the real name. "John Doe (with signature)" is the alias.

Comment 87

[:aceman]

So you expect the user to type "John Doe (with signature)" (also the brackets) into the alias field, but then use the "Your name" field (also containing "John Doe") in the resulting message?

Comment 88

[Richard Marti (:Paenglab)]

It should show everything the user typed into the "Identity Alias" field, so everything between the "" above, also if it's only "Pokus1". And then use the "Your name" field in the resulting message.

Comment 89

[:aceman]

Before I rewrite this again, would the solution in comment 88 suit everybody?

Comment 90

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #88)
> It should show everything the user typed into the "Identity Alias" field, so
> everything between the "" above, also if it's only "Pokus1". And then use
> the "Your name" field in the resulting message.

I think I understand now. In that case, it will be fine if the Alias is left empty and we do not need to offer any default Alias (name <email>). If Alias is empty, we just use 'Your name' also in the display (will be identical to what is sent out).

Comment 91

[Richard Marti (:Paenglab)]

(In reply to :aceman from comment #90)
> (In reply to Richard Marti (:Paenglab) from comment #88)
> > It should show everything the user typed into the "Identity Alias" field, so
> > everything between the "" above, also if it's only "Pokus1". And then use
> > the "Your name" field in the resulting message.
> 
> I think I understand now. In that case, it will be fine if the Alias is left
> empty and we do not need to offer any default Alias (name <email>). If Alias
> is empty, we just use 'Your name' also in the display (will be identical to
> what is sent out).

Yes, this should be the way.

Comment 92

[Magnus Melin [:mkmelin]]

Sorry I don't like it. The point of my comment 77 was that we don't want to confuse the user with what's actually sent out to people- having the note after the address should make this at least somewhat obvious. If we change the display name this is totally unclear. Also it's unrealistic to expect that users would type in the alias in the form we'd prefer, though of course some may do that.

Comment 93

[Ian Neal]

Isn't the point that the alias will be something meaningful to the user and for different users it will be different things. Could one option be that including the address is a preference (perhaps defaulted to on)?

Comment 94

[:aceman]

(In reply to Magnus Melin from comment #92)
> Sorry I don't like it. The point of my comment 77 was that we don't want to
> confuse the user with what's actually sent out to people- having the note
> after the address should make this at least somewhat obvious. If we change
> the display name this is totally unclear. Also it's unrealistic to expect
> that users would type in the alias in the form we'd prefer, though of course
> some may do that.

The import code uses the feature of replacing whole "full name <email>" with just "full name".
I.e. using the same feature of the "alias" replacing everything, not just being appended to the end of the "full name <email>" string. Maybe users of other clients are used to that.

So do you decide to drop this feature?

Or should I implement another version of alias, that will be public and work as you say. And the old alias used by the import code will still be preserved?

Comment 95

[Magnus Melin [:mkmelin]]

I don't really care about the import of what ns4 may or may not have done with it. We might just as well drop that part.

I think it would be useful the way I suggested, but not high priority...

Comment 96

[:aceman]

In patch https://bug318495.bmoattachments.org/attachment.cgi?id=706679 I actually remove that code from the import modules so you can see where it was done. It is not NS4, but Outlook, WinLiveMail, etc.

Comment 97

[[e-mail]]

(In reply to aceman from comment #89)
> Before I rewrite this again, would the solution in comment 88 suit everybody?

I don't have much interest as to which solution you use since I won't be using identity aliases but perhaps the best compromise would be for the identity alias to appear as the description instead of the account name?

Comment 98

[:aceman]

What is the "description" in this case?

Comment 99

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #98)
> What is the "description" in this case?

alta88, do you know?

Comment 100

[alta88]

(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #99)
> (In reply to :aceman from comment #98)
> > What is the "description" in this case?
> 
> alta88, do you know?

Are you actually asking me, rather than the poster, what the poster meant in comment 97? I don't, do I? ;)

That said, imo, the value of the label field, if it exists, should simply be appended to the nsIMsgIdentity.identityName (perhaps delimited with a '-' or (..) to separate it) , and displayed wherever that is displayed (compose menulist).  Nothing else in nsIMsgIdentity should be altered, that seems unnecessary and asking for trouble.

I don't understand the need for Public/Private. This is unnecessary UI churn. The label field can simply be added in the same line as Account Name; it's obviously separate from the Default Identity group section, which clearly describes 'information other people see'.

Comment 101

[:aceman]

Now I see it is the description attribute on the menuitem, that currently contains the account name. But that one is only displayed in the compose window so is not useful as a solution for other identity lists.

Comment 102

[:aceman]

(In reply to alta88 from comment #100)
> That said, imo, the value of the label field, if it exists, should simply be
> appended to the nsIMsgIdentity.identityName (perhaps delimited with a '-' or
> (..) to separate it) , and displayed wherever that is displayed (compose
> menulist).  Nothing else in nsIMsgIdentity should be altered, that seems
> unnecessary and asking for trouble.

Yes this is the clearest solution that also me and Magnus like.

Jorg, would you have an opinion here?

Comment 103

[Jorg K (CEST = GMT+2)]

I feel honoured by being asked for an opinion here, but I don't really have one since I don't use identities. Also, the bug is from 2005, we're at comment #103, you have a patch which has two r+ by two very rare reviewers, so I'm coming to the party very late. In fact, I contributed to the fact that your patch won't apply since I killed the Eudora(†) import.

That said, I looked at attachment 8604894 [details] and I don't like what I see.

I think others have said that they just want to see the "nicknames" appended somehow. Reading comment #0 (yes, I have read others, too), this is to distinguish identities which otherwise just differ by server/signature/reply-to, etc.

What's wrong with
John Doe <[e-mail]> (reply to John)
John Doe <[e-mail]> (reply to Jill)
John Doe <[e-mail]> (fancy signature) ?

This is what was said in comment #100 and I agree.

If there were a patch, I could play with it.

Comment 104

[:aceman]

Attachment: patch v7 + test

This implements the 'name <email> (label)' version that most of the devs commented about.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=66c82fe1207536a356a34dfbc290e745f1237907

Comment 105

[Richard Marti (:Paenglab)]

Comment on attachment 8749869 [details] [diff] [review]
patch v7 + test

I don't like it 100% but can live with it but I comply with the majority of devs.

Comment 106

[:aceman]

Comment on attachment 8749869 [details] [diff] [review]
patch v7 + test

Thanks.

Comment 107

[:aceman]

Comment on attachment 8749869 [details] [diff] [review]
patch v7 + test

Jorg, could you now play with it?:)

And ping to other reviewers.

Comment 108

[Jorg K (CEST = GMT+2)]

Attachment: patch v7 + test (rebased)

Rebased in suite/mailnews/compose/MsgComposeCommands.js.
Carrying forward Richard's ui-r+.

Comment 109

[Jorg K (CEST = GMT+2)]

Comment on attachment 8781896 [details] [diff] [review]
patch v7 + test (rebased)

Sorry, this has been sitting here for so long that it rotted even more.

Looks like the patch is making identityName readonly and hence removing nsMsgIdentity::SetIdentityName().

That leads to:
c:/mozilla-source/comm-central/mailnews/import/becky/src/nsBeckySettings.cpp(378): error C2039: 'SetIdentityName': is not a member of 'nsIMsgIdentity'

Comment 110

[:aceman]

Thanks, I'll check that.

Please comment on the general appearance in compose window and account settings. I'm referring to comment 103.

Comment 111

[:aceman]

Attachment: patch v7.1 + test

Fixed Becky import so the patch builds again.

Comment 112

[Jorg K (CEST = GMT+2)]

Comment on attachment 8782650 [details] [diff] [review]
patch v7.1 + test

(In reply to :aceman from comment #110)
> Please comment on the general appearance in compose window and account
> settings. I'm referring to comment 103.
It's fine ;-)

(I haven't looked at the code in detail.)

Comment 113

[Jorg K (CEST = GMT+2)]

Is this going to land one day? There are some may superseded patches here, some even with r+ from Neil and IanN so it would be good to finish this off one day.

Comment 114

[:aceman]

I'd love to finish this. But the reviewed patches implement a radically different solution. So we need to take another round of reviews. I'll look at the path if anything needs to be done yet.

Comment 115

[:aceman]

Comment on attachment 8782650 [details] [diff] [review]
patch v7.1 + test

Looks like the patch is ready as is. Let's collect the reviews.

Comment 116

[Magnus Melin [:mkmelin]]

Comment on attachment 8782650 [details] [diff] [review]
patch v7.1 + test

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

::: mail/locales/en-US/chrome/messenger/am-identity-edit.dtd
@@ +15,5 @@
> +
> +<!ENTITY publicData.label        "Public Data">
> +<!ENTITY privateData.label       "Private Data">
> +<!ENTITY identityAlias.label     "Identity Label:">
> +<!ENTITY identityAlias.accesskey "L">

Not much point trying to align these...

::: mailnews/base/util/nsMsgIdentity.cpp
@@ +75,5 @@
> +  // If a non-empty label exists, append it.
> +  nsString label;
> +  rv = GetLabel(label);
> +  if (NS_SUCCEEDED(rv) && !label.IsEmpty())
> +  { // TODO: this should be localizable

i doubt it

Comment 117

[Ian Neal]

Comment on attachment 8782650 [details] [diff] [review]
patch v7.1 + test

>+++ b/mailnews/base/prefs/content/am-identity-edit.xul
>+        <groupbox>
>+          <caption label="&privateData.label;"/>
>+
>+          <hbox align="center">
>+            <label value="&smtpName.label;"
>+                   control="identity.smtpServerKey"
>+                   accesskey="&smtpName.accesskey;"/>
>+            <menulist id="identity.smtpServerKey"
>+                      flex="1">
>+              <menupopup id="smtpPopup">
>+                <menuitem value=""
>+                          label="&smtpDefaultServer.label;"
>+                          id="useDefaultItem"/>
>+                <menuseparator/>
>+                <!-- list will be inserted here -->
>+              </menupopup>
>+            </menulist>
>+          </hbox>
>+
>+          <separator class="thin"/>
>+
>+          <hbox align="center">
>+            <label value="&identityAlias.label;"
>+                   accesskey="&identityAlias.accesskey;"
>+                   control="identity.label"/>
>+            <textbox id="identity.label"
>+                     flex="1"/>
>+          </hbox>
>+        </groupbox>
Nit: style here is that if a multi attribute element can fit within 80 characters then it does not need an attribute per line, so here the <menulist> and <textbox> elements do not need to be across multiple lines

>-nsresult nsMsgIdentity::SetIdentityName(const nsAString& idName) {
>-  return SetUnicharAttribute("identityName", idName);
Are we fairly sure that no extensions make use of this?

>+<!ENTITY publicData.label        "Public Data">
>+<!ENTITY privateData.label       "Private Data">
>+<!ENTITY identityAlias.label     "Identity Label:">
>+<!ENTITY identityAlias.accesskey "L">
This clashes with signatureHtml.accesskey from am-main.dtd, maybe use "b" instead?

>-    addRecipientsToIgnoreList((gCurrentIdentity ? gCurrentIdentity.identityName + ', ' : '') + msgTo + ', ' + msgCC + ', ' + msgBCC);
>+    let currentAddress = gCurrentIdentity ? gCurrentIdentity.fullAddress : "";
>+    addRecipientsToIgnoreList(currentAddress + ", " + msgTo + ", " + msgCC + ", " + msgBCC);
Shouldn't this be:
+    let currentAddress = gCurrentIdentity ? gCurrentIdentity.fullAddress + ", " : "";
+    addRecipientsToIgnoreList(currentAddress + msgTo + ", " + msgCC + ", " + msgBCC);
so as to avoid a leading comma when gCurrentIdentity is null?

r/a=me with those addressed, though you might decide to ask for another review due to the number of changes.

Comment 118

[:aceman]

(In reply to Ian Neal from comment #117)
> >-nsresult nsMsgIdentity::SetIdentityName(const nsAString& idName) {
> >-  return SetUnicharAttribute("identityName", idName);
> Are we fairly sure that no extensions make use of this?

Yes, searching in all addon sources indexed on dxr.mozilla.org shows no callers.

> >+<!ENTITY publicData.label        "Public Data">
> >+<!ENTITY privateData.label       "Private Data">
> >+<!ENTITY identityAlias.label     "Identity Label:">
> >+<!ENTITY identityAlias.accesskey "L">
> This clashes with signatureHtml.accesskey from am-main.dtd, maybe use "b"
> instead?

Thanks.

> >-    addRecipientsToIgnoreList((gCurrentIdentity ? gCurrentIdentity.identityName + ', ' : '') + msgTo + ', ' + msgCC + ', ' + msgBCC);
> >+    let currentAddress = gCurrentIdentity ? gCurrentIdentity.fullAddress : "";
> >+    addRecipientsToIgnoreList(currentAddress + ", " + msgTo + ", " + msgCC + ", " + msgBCC);
> Shouldn't this be:
> +    let currentAddress = gCurrentIdentity ? gCurrentIdentity.fullAddress +
> ", " : "";
> +    addRecipientsToIgnoreList(currentAddress + msgTo + ", " + msgCC + ", "
> + msgBCC);
> so as to avoid a leading comma when gCurrentIdentity is null?

OK, I have reworked this to handle any of the recipient types being empty.

Comment 119

[:aceman]

Attachment: patch v7.2 + test

Comment 120

[:aceman]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=38307b14c56444fec52a471f90f28c7f7914bba4

Comment 121

[:aceman]

I have landed it for the string changes:
https://hg.mozilla.org/comm-central/rev/d2c25fee0909d3f8122a97688e390915961029f2

We can flesh out any code cosmetics in the next days after branching.

Comment 122

[Ian Neal]

Comment on attachment 8808042 [details] [diff] [review]
patch v7.2 + test

r/a=me