474721 - messagereader: message header should not prefer the address book over the message headers as a source of display names

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

In my neck of the woods, Evite is a very widely used event invitation system, and it sends out mails with headers like this:

From: Bill Schlep <[e-mail]>
Reply-To: Bill Schlep <[e-mail]>

They also include this string in the body of the message: "Did this email go to your junk/bulk folder?  Add [e-mail] to your address book to ensure that you receive future Evite Invitations in your Inbox." which explains the motivation (though they have a bug in that they changed the hostname their mail came from without updating the body text to match).

This means that currently, all evites that I get appear, in the compact header view, as though they came from whatever name is associated with [e-mail], which happens to be whoever I got the evite from when I starred that address.  Very confusing.

pingg, an up-and-coming web 2.0 invite site does something similar.

If an email has a display in the From header and we prefer that to anything from the address book, that fixes this problem.

Assigning to clarkbw in the hopes of ui-review+ on the proposed solution.

Comment 1

[Ronald Killmer]

All of the E-commerce sites I get notices and newsletters from use separate Reply To: addresses, since the sending server is outbound only. The confusion noted above can happen with these cases too.  Particularly if sale notices and a newsletter are provided from the same company, i.e. HP or JCPenny.

Comment 2

[Pete Riley]

> If an email has a display in the From header and we prefer
> that to anything from the address book, that fixes this problem.

This bug seems to be for a special case that I hope won't change Thunderbird's general behavior (always using the address book's display name instead of that in the From header).

I propose that for this special case (when an email has a Reply-To header) that it supersedes the From header and Thunderbird gets the display name from the address book based on the Reply-To header instead of the From header.  Of course, if the Reply-To person doesn't have an entry in the address book then Thunderbird could get the full name from the headers.

Otherwise, please make the behavior proposed in this bug optional.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #2)
> 
> This bug seems to be for a special case that I hope won't change Thunderbird's
> general behavior (always using the address book's display name instead of that
> in the From header).

I think there are other reasons to want this behavior, but it's not clear to me whether they're common enough to be compelling here.  People sometimes change what they like to be called (eg some people change their last names upon getting married).  I could imagine people sharing an email account using different names at different times, though I have no idea whether this actually happens.

> Otherwise, please make the behavior proposed in this bug optional.

Why?  What's the down side of always preferring the headers?

Comment 4

[Bryan Clark (DevTools PM) [:clarkbw]]

A couple of things seem wrong here; these are my thoughts so far.

One is that we should be showing the email address of a sender you don't have in your address book.  You essentially got tricked into starring invite.com as the email address for this person because we didn't show the relationship correctly.

I'm not sure about the Reply-To superseding the From header, initially it seems like a good idea; however I don't like the silent behavior which I'll get to more in a second.

As far as display I think the assumptions we made in the previous bug that defaults to the address book are partially incorrect.

I think part of the problem here is our silent (silent to the user) swap of names between the name provided and the name we have.  I think the provided header information is the correct display information, but we should also use the contact display name when it makes sense.

Here are a couple of situations for a contact we have as "Alice Bee".

Examples of the From: header

1) <[e-mail]>
2) Alice B. <[e-mail]>
3) Alice Bee <[e-mail]>
4) Alice Bee <[e-mail]>

Examples of how I think we want to display each

1) Alice Bee
2) Alice B. (Alice Bee)
3) Alice Bee
4) Alice Bee <[e-mail]>

The difference I'm pointing out here is that we should really only use the Address Book display name when no name is provided or it matches the given header name exactly (doesn't matter).  If the given header name is different from the Address Book name I think we need to default to the header information, but include our known name as the alternate; but the sender is likely more "correct" about their name even if we have our own version.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

In general, this seems reasonable to me.  The proposed display for 2) feels a little weird, but maybe that's ok.  There's still possibly a problem with Reply-To in the compact view, however, in that the context menu items for "Copy Email Address" and "Compose Email To" may want to act on different email addresses, which seems like somewhat surprising behavior.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Although without knowing what the copied email address is intended to be used for, it's a little hard to infer which address to pick.

Comment 7

[Pete Riley]

> 2) Alice B. (Alice Bee)

While I would prefer to only see "Alice Bee" from the address book (to reduce clutter), I also think that Bryan's compromise is reasonable, in lieu of a pref to let users decide which display name we want Thunderbird to use.

Comment 8

[Ronald Killmer]

While the focus here has been on Mail, a reminder that News also displays the Posters address (Often munged) and some do use a valid Reply To: which it would be useful to see in the display.

Comment 10

[Ben Bucksch (:BenB)]

I whole agree to comment 4 and I think it's a great idea and proposal.

I don't think Reply-To should override From in display. They are entirely distinct things, they might even be two different persons.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

As per <http://weblogs.mozillazine.org/dmose/archives/2009/06/thunderbird_compact_header_mov.html>, the compact header is no longer part of the Thunderbird core, so this bug is no longer valid.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Reopening, as I believe this happens in the regular header view as well.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

This shouldn't be assigned to me, I'm giving this over to dmose for assessing implementation.  Comment #4 has a break down of what I feel is a good solution to this problem, let me know if there are pieces still required of me.

Adding blake to this even though blake is currently busy with other blockers.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

While we'd very much like to get this, if this were the last bug standing, we wouldn't block on it, as it feels like low-percentage use case; marking blocking-thunderbird3-.  Patches very much invited, however.  If anyone reading this is interested in poking at it, I can point you to the right pieces of code...

Comment 15

[Jim Porter (:squib)]

Attachment: A simple patch

This is about as simple as it gets for this bug. I think having a hidden pref to enable/disable the "Provided Name (Address Book Name)" would be good. Probably a 3-way option.

Comment 16

[Mark Banner (:standard8)]

(In reply to comment #15)
> This is about as simple as it gets for this bug. I think having a hidden pref
> to enable/disable the "Provided Name (Address Book Name)" would be good.
> Probably a 3-way option.

We already have a pref: Prefs -> Advanced -> Reading & Display -> Show only display name for people in my address book.

Comment 17

[Jim Porter (:squib)]

(In reply to comment #16)
> We already have a pref: Prefs -> Advanced -> Reading & Display -> Show only
> display name for people in my address book.

Hm, looking at that option (I've never used it before), it seems that it always relies on the information provided in the header if you disable it. So if a display name isn't provided in the email, it just shows the address. Is that what we want?

I don't really have a strong preference either way, but my instinct would be to have two options: 1) show/hide email address for people in my addr book, 2) what display name to use when there's a conflict in display names.

I'm not sure what I'd want to do when email addresses are shown, there's no display name in the email, and there is one in the address book. Currently it would just show [e-mail], but maybe it's appropriate to add the display name from the address book?

Comment 18

[Jim Porter (:squib)]

Attachment: Add preference to determine how conflicting display names are resolved

Ok, this adds a hidden pref called "mail.conflictingDisplayName" (this could use a better name if someone has an idea) that determines how to format the display name when the email header and the address book conflict. The options are:

0 - Email Name (Abook Name)
1 - Email Name
2 - Abook Name

I'm not sure this is quite review-ready, since there seems to be some debate about how to control this preference, but the patch will at least give an idea of how it could work.

Comment 19

[Ben Bucksch (:BenB)]

Compare bug 457304, which describes a case where I want to know - for my own address only - whether the sender knew my real name (esp. for spam).

(I've also sometimes played pranks by addressing with a funny real name like "Black hole <[e-mail]>" or "Big brother <[e-mail]>", but I guess that's really an edge case.)

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

I'm not a big fan of adding preferences unless we _really_ need to, since it adds code complexity and makes the testing & support matrix bigger (eg the first patch adds 4 lines of code, the second patch adds 43 including a bunch of conditionals).

One thing I do like about the second patch is that by adding a FormatDisplayName function, it creates a hook that could easily be overridden in an extension to do whatever the extension felt was appropriate.

One thought about the proposed solution "Alice B. (Alice Bee)":  It offers the user two conflicting versions of the name in question.  Since it's not really clear what the source of each name is and why there are two, I would expect a fair number of users to simply be puzzled about what exactly Thunderbird is showing and why.  It's possible that this won't matter, in that people may not stop to resolve any confusion most of the time, but will (happilY) have a good idea who's emailing them.

I'm wondering if there some sort of other visual representation or affordance we could use here to help the above issue.

I don't feel like I have complete picture of what the typical mental model is likely to be as far as how much people typically resolve the various objects here: "contact in the addressbook", "starred person", "who this mail is from", "who this mail claims to be from", "email address", "real name", "real name as given in email", etc.

Bryan, do you have any thoughts here?

I suspect this belongs in a spinoff bug, but it feels like a moderate amount of the time (eg if somebody has gotten married, or changed the way they refer to themselves), what the user would _really_ want to do is update the addressbook card in some way.

Comment 21

[Jim Porter (:squib)]

(In reply to comment #20)
> I'm not a big fan of adding preferences unless we _really_ need to, since it
> adds code complexity and makes the testing & support matrix bigger (eg the
> first patch adds 4 lines of code, the second patch adds 43 including a bunch
> of conditionals).

I agree with the general theme of this, but the main reason there are more code changes in the second patch is that in the first one, I forgot to handle the case where the user edits a contact with the inline popup. I don't have any strong desire to keep the preference around, though. My initial reasoning for a preference was that there didn't seem to be one clear right answer, so I just tried to avoid that question by making it a setting. Granted, that's usually a bad idea unless there really is no clear answer.

> One thing I do like about the second patch is that by adding a
> FormatDisplayName function, it creates a hook that could easily be overridden
> in an extension to do whatever the extension felt was appropriate.

The best solution might be to pick a reasonable formatting and then let extension writers handle the other cases. The other benefit of the FormatDisplayName function is that it could be used in the multi-message summary and the "From" field in the thread pane. It seems a bit strange to me that for a single mail header field, there are two ways that it gets rendered, depending on what's rendering it:

1) From field in thread pane/multi-message summary: header display name or address
2) From field in the message header/faceted search: address book display name or full-address*

* the faceted search seems to have some weird caching issues, which I'll file a bug on when I figure out the details

> "contact in the addressbook", "starred person"

I remember there being a bug suggesting that the address book stars be turned into little people icons, like in the address book.

> "who this mail is from", "who this mail claims to be from", "email address",
> "real name", "real name as given in email", etc.

One of these could be put in the first row of the menu when you click the email. Right now it just replicates the text of whatever you clicked.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #21)
> My initial reasoning for a
> preference was that there didn't seem to be one clear right answer, so I just
> tried to avoid that question by making it a setting. Granted, that's usually a
> bad idea unless there really is no clear answer.

A reasonable first cut, to be sure.

> The best solution might be to pick a reasonable formatting and then let
> extension writers handle the other cases.

Agreed.  Since Bryan's the UX owner, his opinion is really the one that counts here, so I'd suggest going with his proposal so that we at least do better than the current state of the tree for Tb3.  We can continue to iterate on it as time goes on...

> The other benefit of the
> FormatDisplayName function is that it could be used in the multi-message
> summary and the "From" field in the thread pane. It seems a bit strange to me
> that for a single mail header field, there are two ways that it gets rendered,
> depending on what's rendering it:
> 
> 1) From field in thread pane/multi-message summary: header display name or
> address
> 2) From field in the message header/faceted search: address book display name
> or full-address*

From a UX and generic code standpoint, that sounds exactly right.  My suspicion is that that might require a bit of refactoring, because the threadpane From is probably generated from an nsIMsgDbView implemented in C++.

Comment 23

[Ben Bucksch (:BenB)]

I agree with comment 4, apart from 2).
I quite often change the display name in the address book, because I don't like the way the sender spells himself.
Esp. in an international context, where France e.g. uses "LASTNAME Firstname" (incl. uppercase) or "Firstname LASTNAME", but I want to see "Firstname Lastname", everything else hurts my eyes and takes me longer to process.

In other cases, I give a person a nickname (as "Display name"), e.g. "gozer", and I want to see that instead of the full name.

Both would be dwarfed by the proposal in comment 4. I think 2) should also be rendered as "Alice Bee", i.e. always prefer the display name in the personal address book over the From name.

Bryan? What do you think?

Comment 24

[Ben Bucksch (:BenB)]

Sorry, I ignored the initial description.

One way to resolve would be to store (in the address book, in a new field) what the From name was when the address was stored. If it changes, we display both (*1).
If the user edits the contact again, the new From is stored, and again only the address book display name is displayed.

*1 IMHO as "Address Book name (From name)", not the other way around

Comment 25

[Ben Bucksch (:BenB)]

(Another example, which happened a moment ago: The company I work for has Froms like "HQ-Finance-2-12 Lastname Firstname Fullmiddlename", which I shorten to "Firstname Lastname". I don't want to see the full From at all, because it costs too much space and only distracts me.)

Comment 26

[Tom Parker]

I second Ben's suggestion above, about how to address this issue.

Comment 27

[David :Bienvenu]

not blocking, wanted. I guess we need a new patch?

Comment 28

[Jim Porter (:squib)]

Since there's no clear answer to which name you'd want (even for a given user), maybe it would be best to have a checkbox under the display name field in the address book card that says "Always prefer display name over message header" (maybe with better wording). I can certainly see wanting to override the display name for some senders and not for others.

Comment 29

[Jim Porter (:squib)]

Attachment: Add option in address book card to prefer adress book name vs. header name

This isn't quite review-ready (there are no tests, and it'll almost certainly break Seamonkey), but this patch adds a per-contact setting to determine whether to use the AB name or the header name when there's a conflict. It also automatically sets the setting to prefer the AB name if you edit the display name in the little star popup. Finally, it also has the disambiguation of "You" from bug 478466, since these touch the same code.

It also removes a couple of inconsistencies when updating address book entries. Before, updates didn't use the same display name formatting, so they didn't display "You". Now they do. Another benefit of this is that msgHdrViewOverlay.js is actually *shorter* now, since I coalesced a couple of similar functions.

Comment 30

[Jim Porter (:squib)]

Attachment: The new setting in the edit card dialog

Comment 31

[Ben Bucksch (:BenB)]

I think this is overdoing it, and way too prominent in a common UI (address book entry) instead of just prefs.
If I wanted to use the sender's name, I could just use that in my address book, no?

Can we instead do what I suggested in comment 24 and forget about the per-contact setting and UI?

Comment 32

[Jim Porter (:squib)]

(In reply to comment #31)
> I think this is overdoing it, and way too prominent in a common UI (address
> book entry) instead of just prefs.
> If I wanted to use the sender's name, I could just use that in my address book,
> no?

That, in and of itself, wouldn't address comment 0, and that's really where we are pre-patch. As for the feature being overkill, keep in mind that there are already 14 input boxes of some kind on that tab alone (and this is after refactoring that dialog to split stuff out). Granted, I realize that this justification could easily lead to death by a thousand cuts, but I don't think there's a lot of mental overhead in a single checkbox that directly relates to the field above it.

> Can we instead do what I suggested in comment 24 and forget about the
> per-contact setting and UI?

Adding a hidden property that's not directly editable in the address book would just add to the confusion. Then the behavior of the message header would be based on a value *totally invisible* to the user. Taking the example in comment 0, if you manually edited the display name, the only way to get back to preferring the header name would be to delete the contact and start over.

If there's going to be a per-contact way of setting this behavior, it really needs to be visible *somewhere*.

Comment 33

[Jim Porter (:squib)]

Comment on attachment 427448 [details] [diff] [review]
Add option in address book card to prefer adress book name vs. header name

clarkbw, do you mind taking a look at this to see what you think about the new behavior?

Comment 34

[Mark Banner (:standard8)]

I'm trying to be open-minded about the per-contact way. The significant issue in my mind is that this is going to be a pain to configure for users with more than a few contacts.

Comment 35

[Jim Porter (:squib)]

I'm not sure there's an easy way to do it, but what I'd like would be for existing contacts to default to "use display name" (since that's how it works pre-patch), and for newly-added contacts to default to "use header name". In the patch, if you edit the name in the inline popup, it'll automatically switch to "use display name", since I figure that if you're changing the display name, you (probably) want to see it.

Incidentally, this behavior is the same as the suggestion in comment 24, but with a UI for the setting.

Comment 36

[Magnus Melin [:mkmelin]]

I'd too prefer it to be a global option. Would fit right in in the pref ui under Advanced | Reading & Display where we already have a related pref.

Comment 37

[Jim Porter (:squib)]

(In reply to comment #36)
> I'd too prefer it to be a global option. Would fit right in in the pref ui
> under Advanced | Reading & Display where we already have a related pref.

That's actually the second patch I wrote (sans UI), but things kind of fizzled out after that.

Comment 38

[Jim Porter (:squib)]

To motivate more discussion on this, here's the exact behavior in my patch (PDN = "prefer display name"):

1: from someone else
 1.1: header is "[e-mail]"
  1.1.1: not in AB                    -> "[e-mail]"
  1.1.2: in AB as "Ted Danson", PDN=0 -> "Ted Danson"
  1.1.3: in AB as "Ted Danson", PDN=1 -> "Ted Danson"
 1.2: header is "Danson, Ted <[e-mail]>"
  1.2.1: not in AB                    -> "Danson, Ted <[e-mail]>"
  1.2.2: in AB as "Ted Danson", PDN=0 -> "Danson, Ted"
  1.2.3: in AB as "Ted Danson", PDN=1 -> "Ted Danson"

2: from you
 2.1: header is "[e-mail]"; one identity in TB
  2.1.1: not in AB, one identity         -> "You"
  1.1.2: in AB as "Carl Sagan", PDN=0    -> "You"
  1.1.3: in AB as "Carl Sagan", PDN=1    -> "Carl Sagan"
 2.2: header is "[e-mail]"; two identities in TB
  2.2.1: not in AB, one identity         -> "You <[e-mail]>"
  1.2.2: in AB as "Carl Sagan", PDN=0    -> "You <[e-mail]>"
  1.2.3: in AB as "Carl Sagan", PDN=1    -> "Carl Sagan"
 2.3: header is "Sagan, Carl <[e-mail]>"; one identity in TB
  * as 2.1
 2.4: header is "Sagan, Carl <[e-mail]>"; two identities in TB
  * as 2.2

Comment 39

[Jim Porter (:squib)]

Haha oops, replace "[e-mail]" with "[e-mail]". Forgot to change that (Peter Cushing caused linewrap :)).

Comment 40

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 427448 [details] [diff] [review]
Add option in address book card to prefer adress book name vs. header name

Ok, looks good.  I tried to rework the string used because it seems awkward but then quickly learned that everything seems awkward.

Here's my best take:

"Always use this display name in message headers"


With this change it seems like bug 478466 is unnecessary as they do the same thing.

Comment 41

[Jim Porter (:squib)]

(In reply to comment #40)
> With this change it seems like bug 478466 is unnecessary as they do the same
> thing.

Yeah, they touched the same code, so I just simplified matters and put it all into this patch, which did other stuff too.

Comment 42

[Jim Porter (:squib)]

Oh, I've been thinking about this and behaviorally, I think I have the logic of the "always use display name" backwards. Currently, it defaults to false, but I think it should default to true for backwards compatibility and for read-only address books (e.g. LDAP). Wanting to use the display name is probably more common, so I shouldn't make that impossible for LDAP users. Then I'd just have to explicitly set "always use display name" to false when you first click the star for a new contact in order to maintain the behavior in that case.

Comment 43

[Ben Bucksch (:BenB)]

In fact, you might want to make this a tri-state checkbox: default (neither checked nor unchecked) is to use a global pref.

Comment 44

[Ben Bucksch (:BenB)]

And yes, using display names for me is critical. mail reader gets pretty unusable without this.
Just to show why, this is how my employer sends emails:

From: ".MOBILE HEADQERS-2-IT-5-67 Flintstone, Fred"
 <[e-mail]>

I just make that "Fred Flintstone".
And that is not exaggerated in the slightest, but in fact one of the shorter addresses of that organization. There are people called "Katherina Stehli-Jaspersonheimer".

Note also my preference of first name last name vs. last name, first name in the header.

Comment 45

[Jim Porter (:squib)]

It looks like the first time you click a star, the address book is updated via nsIAbAddressCollector::CollectSingleAddress: <http://mxr.mozilla.org/comm-central/source/mailnews/addrbook/src/nsAbAddressCollector.cpp#175>. However, this doesn't give me the opportunity to get the "always use display name" flag to false (see comment 42). I could add code in that function to do it, but then I'd be affecting Seamonkey. Any ideas?

Comment 46

[Jim Porter (:squib)]

Oh, also:

(In reply to comment #40)
> (From update of attachment 427448 [details] [diff] [review])
> Ok, looks good.  I tried to rework the string used because it seems awkward 
> but then quickly learned that everything seems awkward.

Maybe adding a tooltip to explain what the checkbox means in more detail would help? The "Allow remote content" checkbox does this already...

Comment 47

[Jim Porter (:squib)]

Attachment: Default to preferring address book name

Ok, this addresses my comments in comment 42. I think this patch is probably done modulo testing. Where would the tests for this go?

Comment 48

[Dan Mosedale (:dmosedale, :dmose)]

test-message-header.js

Comment 49

[Ben Bucksch (:BenB)]

card.getProperty("PreferDisplayName", 1) != 0
cardproperty.getProperty("PreferDisplayName", true) != false

1/0 or true/false? doesn't it matter?

Comment 50

[Jim Porter (:squib)]

(In reply to comment #49)
> card.getProperty("PreferDisplayName", 1) != 0
> cardproperty.getProperty("PreferDisplayName", true) != false
> 
> 1/0 or true/false? doesn't it matter?

Honestly, I just copied that from the code above it for AllowRemoteContent. I'm not actually sure if it's necessary or not. :)

Comment 51

[Jim Porter (:squib)]

Attachment: Add mozmill tests

Ok, this patch has Mozmill tests for all the cases in comment 38 (excluding 2.3 and 2.4, which are redundant). I also addressed the "PreferDisplayName" property inconsistency in comment 49; it's always true/false now.

Comment 52

[Jim Porter (:squib)]

Attachment: Remove stowaway files

Oops, another Mozmill test managed to stow away in that last patch. Here's a patch that should have the right files.

Comment 53

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

Comment on attachment 439643 [details] [diff] [review]
Remove stowaway files

>+++ b/mail/base/content/msgHdrViewOverlay.js	Fri Apr 16 18:14:04 2010 -0500
>@@ -1093,39 +1093,22 @@
>+function FormatDisplayName(emailAddress, documentNode)
> {
>+  var displayName;
>+  var identity = getBestIdentity(accountManager.allIdentities);
>+  var card = documentNode.cardDetails.card;
> 
>   // If this address is one of the user's identities...
>+  if (!displayName && emailAddress == identity.email) {

Since you only just declared displayName up above, and you haven't set it to anything, isn't it guaranteed to be undefined here?

>@@ -1137,39 +1120,38 @@
>+  if (card) {
>+    if(!displayName)
>+      displayName = documentNode.getAttribute("displayName");

I'm not sure if this test should be in the "if (card)" block, either, for a couple of reasons.  First, it doesn't seem to depend on the card existing.  Second, it leaves us open to the possibility that we might return undefined, which seems a little odd.  (It may not matter, but I wanted to raise it as a concern.)

>+    if (!displayName || card.getProperty("PreferDisplayName", true) != false)

Isn't that last bit the same as "card.getProperty("PreferDisplayName", true)"?  Oh, okay, I just found the comment in abCardOverlay.js:
>+    // getProperty may return a "1" or "0" string, we want a boolean
Maybe we want to say that up here, too.  (Also, you might think that you could use the "!!" operator, but apparently not.)

>+++ b/mail/test/mozmill/message-header/test-display-names.js	Fri Apr 16 18:14:04 2010 -0500
>@@ -0,0 +1,215 @@
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Messaging, Inc.

I've been told that this should be "The Mozilla Foundation." because they actually own all the copyrights to Mozilla Messaging code.

>+/*
>+ * Test that we can open and close a standalone message display window from the
>+ *  folder pane.
>+ */
>+var MODULE_NAME = 'test-identities';

It's probably best if the module name matches the file name.

>+var RELATIVE_ROOT = '../shared-modules';
>+var MODULE_REQUIRES = ['folder-display-helpers'];

Also, could you please change the 's to "s here, and in the other places it occurs?

>+  collectedAddresses = abManager.getDirectory("moz-abmdbdirectory://history.mab");

Would it be worth using kCollectedAddressbookURI from mail/components/addrbook/content/abCommon.js here?
>+
>+  let bundle = Cc["@mozilla.org/intl/stringbundle;1"]
>+      .getService(Ci.nsIStringBundleService).createBundle(
>+          "chrome://messenger/locale/messenger.properties");

Two-space indents, please.

>+function ensure_single_identity() {
>+  if (localAccount.identities.Count() > 1)
>+    localAccount.removeIdentity(secondIdentity);

Should that be "while (localAccount.identities…"?

>+function help_test_display_name(message, field, expectedValue) {
>+  be_in_folder(decoyFolder);
>+  be_in_folder(folder);

So, I don't really get why you're using the decoyFolder here.  Please remove it, or add a comment explaining why you're using it.

>+function _test_single_identity_in_abook_no_pdn() {
>+function _test_multiple_identities_in_abook() {

Why aren't we testing these?

>+++ b/mailnews/addrbook/content/abCardOverlay.xul	Fri Apr 16 18:14:04 2010 -0500
>@@ -126,6 +126,14 @@
>+              <hbox class="CardEditWidth">
>+                <checkbox id="preferDisplayName" label="&preferDisplayName.label;"

I think we can break this before the label, to keep it under 80 columns.

>+                          accesskey="&preferDisplayName.accesskey;"/>
>+++ b/mailnews/addrbook/src/nsAbAddressCollector.cpp	Fri Apr 16 18:14:04 2010 -0500
>@@ -310,6 +310,9 @@
>+  if (modifiedCard)
>+    aSenderCard->SetPropertyAsBool("PreferDisplayName", false);

So, why don't we want to prefer the display name if we've modified the card?

So, there are a bunch of things I've mentioned, but they're all pretty minor, so I'm going to go ahead and give it the r=me with the things fixed or explained.

Thanks,
Blake.

Comment 54

[Jim Porter (:squib)]

I'll be attaching an updated patch right after this. In addition to the stuff I've addressed from the review, I've also changed the argument list for FormatDisplayName to make it usable in other places (e.g. the multi-message summary).

(In reply to comment #53)
> >   // If this address is one of the user's identities...
> >+  if (!displayName && emailAddress == identity.email) {
> 
> Since you only just declared displayName up above, and you haven't set it to
> anything, isn't it guaranteed to be undefined here?

Fixed.

> >@@ -1137,39 +1120,38 @@
> >+  if (card) {
> >+    if(!displayName)
> >+      displayName = documentNode.getAttribute("displayName");
> 
> I'm not sure if this test should be in the "if (card)" block, either, for a
> couple of reasons.  First, it doesn't seem to depend on the card existing. 
> Second, it leaves us open to the possibility that we might return undefined,
> which seems a little odd.  (It may not matter, but I wanted to raise it as a
> concern.)

I've changed it to return null instead and added some comments about why I do that. Basically, if there's no card, then we don't want to try to format a display name, since we might lose information. There are a variety of ways of handling the "I don't know this person" case: for here, we show the raw value from the email's header. For the multi-message summary, we show the display name from the mail header, or if that's not there, the email address. So returning null means "go ahead and fall back to whatever method you prefer".

> >+    if (!displayName || card.getProperty("PreferDisplayName", true) != false)
> 
> Isn't that last bit the same as "card.getProperty("PreferDisplayName", true)"? 
> Oh, okay, I just found the comment in abCardOverlay.js:

I copied the comment over.

> >+++ b/mail/test/mozmill/message-header/test-display-names.js	Fri Apr 16 18:14:04 2010 -0500
> >@@ -0,0 +1,215 @@
> >+ * The Initial Developer of the Original Code is
> >+ * Mozilla Messaging, Inc.
> 
> I've been told that this should be "The Mozilla Foundation." because they
> actually own all the copyrights to Mozilla Messaging code.

Fixed.

> >+var MODULE_NAME = 'test-identities';
> 
> It's probably best if the module name matches the file name.

Oops, I forgot to change that when I moved the test over from bug 478466.

> >+var RELATIVE_ROOT = '../shared-modules';
> >+var MODULE_REQUIRES = ['folder-display-helpers'];
> 
> Also, could you please change the 's to "s here, and in the other places it
> occurs?

Done.

> >+  collectedAddresses = abManager.getDirectory("moz-abmdbdirectory://history.mab");
> 
> Would it be worth using kCollectedAddressbookURI from
> mail/components/addrbook/content/abCommon.js here?

Maybe. I'm not sure how to import that though.

> >+  let bundle = Cc["@mozilla.org/intl/stringbundle;1"]
> >+      .getService(Ci.nsIStringBundleService).createBundle(
> >+          "chrome://messenger/locale/messenger.properties");
> 
> Two-space indents, please.

Fixed. Though I also lined up the ".getService" with "Cc" since I do that elsewhere.

> >+function ensure_single_identity() {
> >+  if (localAccount.identities.Count() > 1)
> >+    localAccount.removeIdentity(secondIdentity);
> 
> Should that be "while (localAccount.identities…"?

Not in this case. I ensure that there are only ever 1 or 2 identities, and then I delete the secondary one. It was easier than trying to delete all but the primary identity. :)
 
> >+function help_test_display_name(message, field, expectedValue) {
> >+  be_in_folder(decoyFolder);
> >+  be_in_folder(folder);
> 
> So, I don't really get why you're using the decoyFolder here.  Please remove
> it, or add a comment explaining why you're using it.

I added a comment. I need to ensure that the message gets refreshed between tests (some tests use the same message), and switching folders seemed like the easiest way to do that.

> >+function _test_single_identity_in_abook_no_pdn() {
> >+function _test_multiple_identities_in_abook() {
> 
> Why aren't we testing these?

Because I forgot to re-enable them after I fixed a bug. They're back on now. :)

> >+++ b/mailnews/addrbook/content/abCardOverlay.xul	Fri Apr 16 18:14:04 2010 -0500
> >@@ -126,6 +126,14 @@
> >+              <hbox class="CardEditWidth">
> >+                <checkbox id="preferDisplayName" label="&preferDisplayName.label;"
> 
> I think we can break this before the label, to keep it under 80 columns.

Done.

> >+  if (modifiedCard)
> >+    aSenderCard->SetPropertyAsBool("PreferDisplayName", false);
> 
> So, why don't we want to prefer the display name if we've modified the card?

That only happens when we've automatically pulled out a display name during card collection (e.g. sending a mail, clicking the star for the first time). In those cases, the user hasn't explicitly set the display name, so I assume that he's ok with whatever is in the email header, since that's what he got.

Comment 55

[Jim Porter (:squib)]

Attachment: Address review comments

Flagging this for review again just in case...

Comment 56

[Jim Porter (:squib)]

Attachment: Once more, without selectionsummaries.js

Sigh... I accidentally attached a related in-progress patch for the multi-message summary. This version doesn't have it.

Comment 57

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

Comment on attachment 441634 [details] [diff] [review]
Once more, without selectionsummaries.js

>+++ b/mail/base/content/msgHdrViewOverlay.js	Mon Apr 26 17:39:55 2010 -0500
>@@ -1110,83 +1110,80 @@
>+function FormatDisplayName(aEmailAddress, aHeaderDisplayName, aContext, aCard)
[snip…]
>+  var card = aCard ? aCard : getCardForEmail(aEmailAddress).card;

Isn't this the same as "aCard || getCardForEmail(aEmailAddress).card"?

>+++ b/mail/test/mozmill/message-header/test-display-names.js	Mon Apr 26 17:39:55 2010 -0500
>@@ -0,0 +1,217 @@
[snip…]
>+  let bundle = Cc["@mozilla.org/intl/stringbundle;1"]
>+               .getService(Ci.nsIStringBundleService).createBundle(
>+                 "chrome://messenger/locale/messenger.properties");

Two more spaces (or one more level) of indent on the previous two lines, please.

Other than that, seems great to me!

Later,
Blake.

Comment 58

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

Attachment: (The interdiff between the two versions I reviewed.)

(Just cause it's hard to get from Bugzilla, and it seemed useful.)

Comment 59

[Jim Porter (:squib)]

Attachment: Fix nits

This addresses the latest review comments. I forgot about the a || b syntax (I've been writing a lot of C), and I fixed the indentation in the test file.

Note for sr: this does make a change to mailnews/ so it'll need a Seamonkey version of the rest of the code. That's probably doable in a separate bug, though.

Comment 61

[Jim Porter (:squib)]

Just a reminder (to myself, more than anything): take some of the code from the tests for bug 563612 and use them in the tests for this bug.

Other than that, and possibly fixing some bitrot, this patch should be good (I hope!).

Comment 62

[Jim Porter (:squib)]

Attachment: Pull out address book test helpers

The only changes in this patch (aside from some offsets being different) is that I put the address book helper functions in their own module so they can be used by this and bug 563612.

Comment 63

[Jim Porter (:squib)]

So it looks like this is being held back by the fact that the patch will break Seamonkey in one (or two, depending on how you look at it) places:

1) The edit contact dialog (abCardOverlay.xul) would have an extra field that does nothing and has no translation
2) Adding new contacts (e.g. via clicking the star) would have an extra property set: "PreferDisplayName", which would be useless to Seamonkey as it doesn't handle that property, but could cause weird behavior if they ever started handling it.

(1) can be fixed by forking abCardOverlay.xul, which I think makes sense, since all the related files are already forked.

(2) might not need fixed, but I guess you could fix it with #ifdefs?

Comment 64

[Jim Porter (:squib)]

Anything I can do to help get this checked in?

Comment 65

[Mark Banner (:standard8)]

(In reply to comment #64)
> Anything I can do to help get this checked in?

Sorry Jim, I've been fighting the tree this week to get it open again. Thankfully, we are almost there now.

I've put sorting this out into a slightly more noticeable place for me to remember it, but if I haven't done anything by Monday, feel free to ping me direct.

Comment 66

[Jim Porter (:squib)]

Attachment: Update for forked abCardOverlay.xul

This handles the forked abCardOverlay.xul from bug 584026 so now this patch doesn't break Seamonkey (except for point (2) in comment 63). Hopefully, this is the last thing that needed to be fixed. :)

Comment 67

[Mark Banner (:standard8)]

Jim, I've been trying to run the MozMill tests, but I'm getting:

TEST-UNEXPECTED-FAIL | e:\buildbot\tryserver-win32-opt-unittest-mozmill\build\mozmill\message-header\test-message-header.js | setupModule
  EXCEPTION: no message!
    at: nonesuch line 0

any ideas?

Comment 68

[Jim Porter (:squib)]

I'm looking into it. Unfortunately, I can't seem to actually run the latest trunk build of Thunderbird, since every single window (main, address book, compose, etc) pops up and then the whole program quits a few seconds later. I'll mess with it a bit more; maybe this is related to an issue I had with the mozmill test suites earlier...

Comment 69

[Jim Porter (:squib)]

Here's the stuff from the error console if anyone cares:

Warning: Warning: Ignoring obsolete chrome registration modifier 'xpcnativewrappers=yes'.
Source File: /home/porterj/comm-central/objdir-tb-release/mozilla/dist/bin/chrome/toolkit.manifest
Line: 3

[Above repeated a bunch of times for other files...]

Could not read chrome manifest file '/home/porterj/comm-central/objdir-tb-release/mozilla/dist/bin/extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}/chrome.manifest'.

Error: this._updateVisibleText is not a function
Source File: chrome://messenger/content/search.xml
Line: 140

Error: Gloda.myContact is null
Source File: file:///home/porterj/comm-central/objdir-tb-release/mozilla/dist/bin/components/glautocomp.js
Line: 283

2010-08-11 15:55:16	gloda.ds.qfq	ERROR	Exception: TypeError: Gloda.myContact is null

Warning: Expected color but found 'dialog'.  Error in parsing value for 'background-color'.  Declaration dropped.
Source File: chrome://messenger/skin/accountCreation.css
Line: 229

Comment 70

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

I think that might be due to a bug Standard8 just pushed a fix for.  Try getting latest, blowing away your objdir, and rebuilding everything.  (Or ask in #maildev for smaller steps, if you don't feel like waiting for a full rebuild. ;)

Later,
Blake.

Comment 71

[Jim Porter (:squib)]

Ok, everything works now. Thanks, Blake.

Standard8, I'm not getting the error you have. Everything works for both test-message-header.js and test-display-names.js, though when the tests finish, I get a Python error from runtest.py. I'm using the latest Mozmill (1.4.2b3) since the tests were hanging on startup using 1.4 and I have the most recent trunk build of Thunderbird fully cleaned and all that good stuff.

Comment 72

[Mark Banner (:standard8)]

Jim: were you running the tests individually or as a set in one directory?

It is when you're running them as a set (which is what make mozmill does - run each folder in one startup/shutdown cycle) that they fail.

I've tracked it down to the fact that test-display-names.js is creating the MessageWindowA folder which test-message-headers.js has already been using, and so now when test-message-headers.js comes along, the folder is already created and it gets confused.

Doing this change resolves the failure:

-  folder = create_folder("MessageWindowA");
-  decoyFolder = create_folder("MessageWindowB");
+  folder = create_folder("MessageWindowB");
+  decoyFolder = create_folder("MessageWindowC");

I've pushed this to our try server to verify its working on all platforms, but it worked fine on my machine like this.

Comment 73

[Jim Porter (:squib)]

Ah ha, that'll do it. I had always thought running the tests independently was identical to running them together. Guess not.

Comment 74

[Jim Porter (:squib)]

(In reply to comment #72)
> I've pushed this to our try server to verify its working on all platforms, but
> it worked fine on my machine like this.

Did it work on the try server? I looked for it and thought I found it, but I'm not 100% sure what qualifies as "working".

Comment 75

[Mark Banner (:standard8)]

Comment on attachment 464126 [details] [diff] [review]
Update for forked abCardOverlay.xul

Sorry for all the delays in this bug.

This looks good, and I've checked it in with the mozmill fix applied:

http://hg.mozilla.org/comm-central/rev/82a5f5faa5b1

Thanks for all your work on this Jim.

Comment 76

[Mark Banner (:standard8)]

(wontfix for 3.1.x because the fix contains string changes)

Comment 77

[Mark Banner (:standard8)]

Some of the tests were failing on Windows, so I've disabled them for now:

http://hg.mozilla.org/comm-central/rev/746fa9d763e5

Jim, can you file a follow-up bug to fix those please?

Tinderbox log: http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1282734469.1282740617.17017.gz

TEST-UNEXPECTED-FAIL | e:\buildbot\win32-comm-central-check\build\mail\test\mozmill\message-header\test-display-names.js | test_single_identity
  EXCEPTION: got 'You <[e-mail]>' but expected 'You'
    at: test-display-names.js line 119
       Error("got 'You <sender  0
       help_test_display_name(0,"to","You") test-display-names.js 119
       test_single_identity() test-display-names.js 125
            frame.js 474
            frame.js 530
            frame.js 573
            frame.js 417
            frame.js 579
            server.js 164
            server.js 168
TEST-PASS |  test_single_identity_in_abook
TEST-UNEXPECTED-FAIL | e:\buildbot\win32-comm-central-check\build\mail\test\mozmill\message-header\test-display-names.js | test_single_identity_in_abook_no_pdn
  EXCEPTION: got 'You <[e-mail]>' but expected 'You'
    at: test-display-names.js line 119
       Error("got 'You <sender  0
       help_test_display_name(0,"to","You") test-display-names.js 119
       test_single_identity_in_abook_no_pdn() test-display-names.js 137
            frame.js 474
            frame.js 530
            frame.js 573
            frame.js 417
            frame.js 579
            server.js 164
            server.js 168

Comment 78

[Jim Porter (:squib)]

Thanks for getting this in!

I filed bug 590700 for comment 77. Not sure how much I'll be able to do, since I don't have a Windows build environment and I'm also going on vacation soon.

Comment 79

[Thomas D. (:thomas8)]

Having cross-read the comments, a lot of behaviour and layout variants have been discussed, and it's not very clear (to me) which of these variants are actually implemented by this patch.

Jim, could you post a screenshot and a description of the intended behaviour after your patch?

Comment 80

[Thomas D. (:thomas8)]

http://gsfn.us/t/1c62p

Comment 81

[Jim Porter (:squib)]

Comment 38 should describe everything. Not sure a screenshot will help much since this only changes the text and nothing else.

Comment 82

[Jim Porter (:squib)]

Oh, this also makes all* display name handling run through the JS function FormatDisplayName. It'd probably be worth adding a relnote for extension authors if they want to override this.

* So far, this is only in the message header. Still needed:
  * Thread pane (bug 312821)
  * Multi-message summary (bug 563062)
  * Gloda (bug 591730)

Comment 84

[Seungbeom Kim]

(In reply to Ben Bucksch (:BenB) from comment #43)
> In fact, you might want to make this a tri-state checkbox: default (neither
> checked nor unchecked) is to use a global pref.

It's probably too late, but I think this should have been off by default, or at least a tri-state checkbox. All the entries in my address book now have PDN=1, and there's no easy way to turn them off, even if the *default* behaviour I want is to show what exactly the message says. (Or is there an easy way?)

There have already been enough examples showing why PDN=1 can be bad, but to add another: you may use your name in different scripts depending on the language of the message or the recipients (especially when you're in a community of people living in a foreign country, e.g. Koreans in the United States). It's really awkward if a person's name in one script is automatically added to the address book and, later when the name in another script should be used, one overrides the other.

This "overriding" is a nontrivial behaviour which needs an explicit approving action by the user, something stronger than what happens "just silently".

Comment 85

[Jim Porter (:squib)]

(In reply to Seungbeom Kim from comment #84)
> (In reply to Ben Bucksch (:BenB) from comment #43)
> > In fact, you might want to make this a tri-state checkbox: default (neither
> > checked nor unchecked) is to use a global pref.
> 
> It's probably too late, but I think this should have been off by default, or
> at least a tri-state checkbox. All the entries in my address book now have
> PDN=1, and there's no easy way to turn them off, even if the *default*
> behaviour I want is to show what exactly the message says. (Or is there an
> easy way?)

The reason we chose this behavior is that it matched the behavior prior to this patch. If you'd like to disable this entirely, you can go to Tools > Options > Advanced > Reading & Display and uncheck "Show only display name for people in my address book".