Memo labels to distinguish different identities in one account

RESOLVED FIXED in Thunderbird 52.0

Status

enhancement
RESOLVED FIXED
14 years ago
3 years ago

People

(Reporter: raoul, Assigned: aceman)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
Thunderbird 52.0
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 13 obsolete attachments)

43.07 KB, image/png
Details
31.32 KB, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
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:
OS: Windows Server 2003 → All
Hardware: PC → All
Version: unspecified → Trunk
I've encountered the same problem; this is a valid request, and I can't find a dupe, so confirming.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 360927
How would you propose distinguishing the (apparently identical) identities?
(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).
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.
Assignee: mscott → nobody
Bug #493388 looks a dupe of this, is there any chance of having an alias for different identities with same name/email?
Duplicate of this bug: 493388
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.
Assignee: nobody → acelists
Product: Thunderbird → MailNews Core
4. the label could be shown in the Manage identities dialog.
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.
So you want to allow the user to remove email address from the display (in composition) if the user wishes so?
Sure, if the user can see the identity label they chose, that seems like it will be enough information.
Hopefully it is not too much of a foot gun :)

But OK, it is similar to the account name field.
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.
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.
Bwinton?
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...
Flags: needinfo?(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.
Flags: needinfo?(bwinton)
The problem is you and Magnus want conflicting results ;) I can probably code any one, but first need decision which one I should try.
Flags: needinfo?(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.)
Flags: needinfo?(bwinton)
Posted patch WIP patch (obsolete) — Splinter Review
Attachment #689391 - Flags: ui-review?(bwinton)
Attachment #689391 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Attachment #689391 - Flags: feedback?(iann_bugzilla)
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.
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 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 <foo.bar@example.org>"
Attachment #689391 - Flags: feedback?(mkmelin+mozilla) → feedback+
(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 <foo.bar@example.org>"

I think that depends on what we make .identityName return.
Flags: needinfo?(mbanner)
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?
Attachment #689391 - Flags: feedback?(iann_bugzilla) → feedback+
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.
Attachment #689391 - Flags: ui-review?(bwinton) → ui-review+
Posted patch WIP patch v2 (obsolete) — Splinter Review
So this one has better placement and grouping but we still need answer to comment 23.
Attachment #689391 - Attachment is obsolete: true
Attachment #698336 - Flags: ui-review?(bwinton)
Attachment #698336 - Flags: feedback?(iann_bugzilla)
(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.
Flags: needinfo?(mbanner)
OK, so if email is empty, we can still show just "name" (=label) as the .identityName, as it was before.
Flags: needinfo?(mbanner)
I see the import code sometimes puts placeholder string "import@import.service" as the email. Does anything break if the email would be empty? Or we could special-case import@import.service in getIdentityName and not show it.
Posted patch patch v3 (obsolete) — Splinter Review
Attachment #698336 - Attachment is obsolete: true
Attachment #698336 - Flags: ui-review?(bwinton)
Attachment #698336 - Flags: feedback?(iann_bugzilla)
Attachment #706679 - Flags: ui-review?(bwinton)
Attachment #706679 - Flags: feedback?(mbanner)
Attachment #706679 - Flags: review?(iann_bugzilla)
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
Attachment #706679 - Flags: review?(iann_bugzilla) → review+
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.
Attachment #706679 - Flags: ui-review?(bwinton) → ui-review-
(In reply to :aceman from comment #31)
> I see the import code sometimes puts placeholder string
> "import@import.service" as the email. Does anything break if the email would
> be empty? Or we could special-case import@import.service 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.
Flags: needinfo?(mbanner)
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.
Attachment #706679 - Flags: feedback?(mbanner) → feedback+
Duplicate of this bug: 470587
(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.
Flags: needinfo?(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!  :)
Flags: needinfo?(bwinton)
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.
Posted patch patch v4 (obsolete) — Splinter Review
So this collects all of bwinton's wishes and my further comments.
Attachment #706679 - Attachment is obsolete: true
Attachment #8591246 - Flags: ui-review?(bwinton)
Just as a note, I tried ui-reviewing this today, but c-c doesn't compile currently…  :(
Bug 1152045 will change the identity display in compose window so mark the dependency that I need to fix it up after that.
Depends on: 1152045
Let's not forget some test :)
Flags: in-testsuite?
Posted patch patch v5 (with test) (obsolete) — Splinter Review
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).
Attachment #8591246 - Attachment is obsolete: true
Attachment #8591246 - Flags: ui-review?(bwinton)
Attachment #8594248 - Flags: ui-review?(bwinton)
Posted patch patch v5.1 + test (obsolete) — Splinter Review
Some small fixes in the patch.
Attachment #8594248 - Attachment is obsolete: true
Attachment #8594248 - Flags: ui-review?(bwinton)
Attachment #8594909 - Flags: ui-review?(bwinton)
Attachment #8594909 - Flags: review?(mkmelin+mozilla)
Posted patch patch v5.2 + test (obsolete) — Splinter Review
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.
Attachment #8594909 - Attachment is obsolete: true
Attachment #8594909 - Flags: ui-review?(bwinton)
Attachment #8594909 - Flags: review?(mkmelin+mozilla)
Attachment #8596168 - Flags: ui-review?(bwinton)
Attachment #8596168 - Flags: review?(mkmelin+mozilla)
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.  ;)
Attachment #8596168 - Flags: ui-review?(bwinton) → ui-review+
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.
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. ;)
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?
(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.
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.
(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 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.
Attachment #8596168 - Flags: review?(neil)
Attachment #8596168 - Flags: review?(iann_bugzilla)
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("import@service.invalid"))
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 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.
Attachment #8596168 - Flags: review?(iann_bugzilla) → feedback+
(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.
(In reply to neil@parkwaycc.co.uk 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.
Posted patch patch v6 + test (obsolete) — Splinter Review
Fixed all the nits so far, except those explained.
Attachment #8596168 - Attachment is obsolete: true
Attachment #8596168 - Flags: review?(neil)
Attachment #8596168 - Flags: review?(mkmelin+mozilla)
Attachment #8604892 - Flags: review?(neil)
Attachment #8604892 - Flags: review?(mkmelin+mozilla)
Attachment #8604892 - Flags: review?(iann_bugzilla)
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.
(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 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.
(In reply to neil@parkwaycc.co.uk 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?
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
(In reply to neil@parkwaycc.co.uk 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?
Flags: needinfo?(neil)
(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.
Flags: needinfo?(neil)
Posted patch patch v6.1 + test (obsolete) — Splinter Review
Thanks. Then this should be it.
Attachment #8604892 - Attachment is obsolete: true
Attachment #8604892 - Flags: review?(neil)
Attachment #8604892 - Flags: review?(mkmelin+mozilla)
Attachment #8604892 - Flags: review?(iann_bugzilla)
Attachment #8606529 - Flags: review?(neil)
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.)
Attachment #8606529 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk 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.
Attachment #8606529 - Flags: review?(mkmelin+mozilla)
Attachment #8606529 - Flags: review?(iann_bugzilla)
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 "
Attachment #8606529 - Flags: review?(iann_bugzilla) → review+
(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 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.)
Attachment #8606529 - Flags: review?(mkmelin+mozilla)
(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.
(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.
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 <john@example.com> (with signature)"
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).
(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
> <john@example.com> (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?
Flags: needinfo?(bwinton)
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.
Blocks: 1166482
I'm not sure bwinton will respond here anytime soon;)
Flags: needinfo?(richard.marti)
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) <john@example.com>", 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.
Flags: needinfo?(richard.marti)
(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) <john@example.com>", 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?
Flags: needinfo?(richard.marti)
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.
Flags: needinfo?(richard.marti)
Then what is the difference to Magnus' proposal? Only that the alias is after real name, not after the email part?
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.
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?
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.
Before I rewrite this again, would the solution in comment 88 suit everybody?
Flags: needinfo?(neil)
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(iann_bugzilla)
(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).
Flags: needinfo?(richard.marti)
(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.
Flags: needinfo?(richard.marti)
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.
Flags: needinfo?(mkmelin+mozilla)
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)?
Flags: needinfo?(iann_bugzilla)
(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?
Flags: needinfo?(mkmelin+mozilla)
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...
Flags: needinfo?(mkmelin+mozilla)
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.
Depends on: 1238264
(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?
Flags: needinfo?(neil)
What is the "description" in this case?
Flags: needinfo?(neil)
(In reply to :aceman from comment #98)
> What is the "description" in this case?

alta88, do you know?
Flags: needinfo?(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'.
Flags: needinfo?(alta88)
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.
(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?
Flags: needinfo?(neil) → needinfo?(mozilla)
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 <john@doe.com> (reply to John)
John Doe <john@doe.com> (reply to Jill)
John Doe <john@doe.com> (fancy signature) ?

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

If there were a patch, I could play with it.
Flags: needinfo?(mozilla)
Posted patch patch v7 + test (obsolete) — Splinter Review
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
Attachment #8604894 - Attachment is obsolete: true
Attachment #8606529 - Attachment is obsolete: true
Flags: needinfo?(bwinton)
Attachment #8749869 - Flags: ui-review?(richard.marti)
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.
Attachment #8749869 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 8749869 [details] [diff] [review]
patch v7 + test

Thanks.
Attachment #8749869 - Flags: review?(mkmelin+mozilla)
Attachment #8749869 - Flags: review?(iann_bugzilla)
Blocks: 58342
Comment on attachment 8749869 [details] [diff] [review]
patch v7 + test

Jorg, could you now play with it?:)

And ping to other reviewers.
Attachment #8749869 - Flags: feedback?(jorgk)
Posted patch patch v7 + test (rebased) (obsolete) — Splinter Review
Rebased in suite/mailnews/compose/MsgComposeCommands.js.
Carrying forward Richard's ui-r+.
Attachment #8749869 - Attachment is obsolete: true
Attachment #8749869 - Flags: review?(mkmelin+mozilla)
Attachment #8749869 - Flags: review?(iann_bugzilla)
Attachment #8749869 - Flags: feedback?(jorgk)
Attachment #8781896 - Flags: ui-review+
Attachment #8781896 - Flags: review?(mkmelin+mozilla)
Attachment #8781896 - Flags: review?(iann_bugzilla)
Attachment #8781896 - Flags: feedback?(jorgk)
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'
Attachment #8781896 - Flags: feedback?(jorgk)
Thanks, I'll check that.

Please comment on the general appearance in compose window and account settings. I'm referring to comment 103.
Posted patch patch v7.1 + test (obsolete) — Splinter Review
Fixed Becky import so the patch builds again.
Attachment #8781896 - Attachment is obsolete: true
Attachment #8781896 - Flags: review?(mkmelin+mozilla)
Attachment #8781896 - Flags: review?(iann_bugzilla)
Attachment #8782650 - Flags: feedback?(jorgk)
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.)
Attachment #8782650 - Flags: feedback?(jorgk) → feedback+
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.
Flags: needinfo?(acelists)
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.
Flags: needinfo?(acelists)
Comment on attachment 8782650 [details] [diff] [review]
patch v7.1 + test

Looks like the patch is ready as is. Let's collect the reviews.
Attachment #8782650 - Flags: review?(mkmelin+mozilla)
Attachment #8782650 - Flags: review?(iann_bugzilla)
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
Attachment #8782650 - Flags: review?(mkmelin+mozilla) → review+
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.
Attachment #8782650 - Flags: review?(iann_bugzilla) → review+
(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.
Attachment #8782650 - Attachment is obsolete: true
Attachment #8808042 - Flags: review?(iann_bugzilla)
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.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
Comment on attachment 8808042 [details] [diff] [review]
patch v7.2 + test

r/a=me
Attachment #8808042 - Flags: review?(iann_bugzilla) → review+
You need to log in before you can comment on or make changes to this bug.