243631 - Availability to show the name out of the addressbook for a display name

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sometimes I receive mails from people whose using terrible names assigned to
their email addresses. Within my addressbook I updated all added contacts to the
original name. So I see them by composing a new mail. What I miss is the feature
to overwrite the display name for incoming mails. I can only see the name inside
the from address which this person has set.

Within 'msgHdrViewOverlay.js' in function OutputEmailAddresses the DisplayName
is taken from the from address every time:

  address.displayName = names.value[index];

Could we expand this with a pref that it is also possible to show the saved
display name from the addressbook?

Comment 1

[Ben]

Maybe it does not have to be a pref.

In the address book, there is a menu option (View > Show Name As) where the user
can set its preference about displaying names. In my opinion, this display
setting should be used everywhere: in message headers, in the message list, etc.

Perhaps this option should be copied to the Options window?

Comment 2

[Misty]

I don't know if this should be a different bug or not, but we use Tbird at 
work & in Tools>Options in the Advanced section, "show only display name for 
people in my address book" is checkmarked......but it doesn't seem to work 
consistently.  Emails we receive come in sometimes how our display name is in 
the address book, sometimes as the right name but maybe with an initial in the 
middle or something, & sometimes as the display name & the entire email 
address.  This bug is the closest I could find to that problem because it 
deals with display names on incoming emails.  Most of our company is on 
verion .8 still, but I'm on version 0.9 (20041103) & mine's having this 
problem too.

Comment 3

[Ben]

(In reply to comment #2)
Perhaps you have address collection turned on and TB takes the display names
from the Collected Addresses book. Try emptying the Collected Addresses book and
see what happens.

Maybe this is another bug: the Collected Addresses book should not be used for
display name replacements...

Comment 4

[Misty]

I do not use the collected address book or have it set to save addresses.

Comment 5

[E. Hietbrink]

Thunderbird 1.06 UK

Still problems with "Show only display name":

1) Address books
"Show only Display name for people in my address book" setting fails to ready
any but the Personal Address Book in my case. I split up one huge address book
over multiple address books (Family, Friends, Work, etc.) and now it will only
try to match names from the Personal Address Book.

2) Uppercase/lowercase
The address matching is done case sensitive, so if i receive an email with
[e-mail] CC'd, and I have an entry for [e-mail] in my
address book, it will *not* match! Very annoying. The closest related bug I
could find is maybe #129393.

3) Address book name/Sender name
Whenever there is a match, it will display the name *used by the sender of the
email* instead of the "Display name" used in my address book entry! To
clearify: I have a display name of "Some Name" for the earlier email in my
address book. I receive an email where that person is CC'd but with in the
raw email body:
Cc:  "Some 1337 h4x0r Name" <[e-mail]>
And then the name used is "Some 1337 h4x0r Name" instead of the one from the
address book.

Comment 6

[Mark]

I submitted a bug a couple of years ago with a good rational for making this
feature, and particularly what is described in comment #5 work.  Im not sure if
I should close that bug #165249 or not since it is on Mozilla.  (Which I no
longer use in favor of TB.)  But I think it may be informative to add here the
reason why I think this would be valuable corporate users to get this working.

When I enter a name/email in my address book, I typically put a prefix on
certain display names for the company.  For example "IBM-Fred Suzuki". This
gives me instant awareness of the "group" I am talking to so I am more or less
careful about what I say to them in email.  This is also especially useful when
I have many contacts with similar names.  If I accidently select the wrong name
from the autocompletion list, it is immediately obvious.  (One IBM-xxx in among
4 other FUJITSU-xxx names will stand out.)

I have seen many others, especially in Japan do this.  But one problem is that
this never agrees with what the individual uses in their own display name.  So
whenever I receive email from them, the display name looks wrong.  Even though
it isnt wrong, it loses the information I added.

It sounds like the feature/fix being discussed here will solve this problem and
make many Japanese users happy.

Comment 7

[Mike Cowperthwaite]

The symptom at comment 2 is bug 279290 -- which was WontFix'd for unexplained 
reasons.

Comment 8

[Robert Kaiser]

I think completely replacing the display name is no good idea, prefixing the addressbook entry and putting the sent one in brackets if different, or something in this direction may be better.
This is especially because spam and virus mails love to mix known names with known addresses.
Say, some sort of malware sends out a mail with "Laura Bush <[e-mail]>" and you happen to have that address on file in your address book. I believe this should not be displayed as just "Hillary Clinton" in your mail app, displaying something like "Hillary Clinton (Laura Bush)" would make it much more clear that something's wrong here.

[Disclaimer: I have no personal interest in the mentioned people, I just took publicly known names for explaining this cause. Replace then with any names you do or don't know while parsing this comment.]

Comment 9

[Pete Riley]

I believe that the implementation of this bug would improve security, not decrease it.

First, though, I should say the obvious.  Regardless if Thunderbird shows the display name from the email (current behavior) or from our address books (as per this bug), the legitimacy of an email can not be determined from the display name or the email address in the FROM header.  Both are easily spoofed and should not be trusted at all.

Second, this bug only affects people in our address books:  friends, family, & possibly some business contacts.  Personally I don't have any internationally known celebrities in my address books and I doubt that most other people do either.  It seems extremely unlikely that someone who is sending a fraudulent email to millions of people would be able to guess a FROM email address that happens to be in my address book.

Third, even if the malicious person is lucky and is able to guess a FROM email address that's in my address book, I'm still safe.  For example, suppose that I have these two people in my address book:

  Bill Gates <[e-mail]>
  Scott McNealy <[e-mail]>

If Mr. Gates sends me an email and wants me to think that it's from Mr. McNealy, he might use "Scott McNealy <[e-mail]>".

In this case, the implementation of this bug would actually *improve* security because it would change the display name to the true identity.  In the headers pane I would simply see "Bill Gates" instead of "Scott McNealy <[e-mail]>", and the deception is foiled.

Another nice side effect of this bug is that it would show the display names that we've defined for people who haven't specified their display names in their own email programs (we currently only see their email address when we receive emails from such people).

Comment 10

[Pete Riley]

For the benefit of users who want this functionality today, I've discovered an extension called "UserChrome.js.xpi" that will show the display names from your address book in the headers pane.  It's at
http://forums.mozillazine.org/viewtopic.php?t=397735

Any questions regarding the extension should probably be posted in that forum discussion.  The extension isn't trivial to configure but is straightforward if you have enough computer experience.  It's great but isn't perfect because (1) it's not a core part of Thunderbird, (2) it only seems to affect the headers pane but not the Sender column of the message-list pane, and (3) it's not clear to me if it only uses the primary address book or if it searches all address books.

After you've installed the extension you add the necessary code to your new UserChrome.js file, save it, and restart Thunderbird.  The code to add is at
http://kb.mozillazine.org/UserChrome.js/Mail#Display_name_from_address_book

IMO this is a great temporary workaround.

Comment 11

[Boying Lu]

Attachment: Look up display name in personal address book if it is not in the message header

Comment 12

[Pete Riley]

Boying, thanks for working on this.  I don't understand the source code so I can't verify if you're already doing the following things, but would you please consider:

1) Always show Display Name from AB if it exists in AB (regardless if
    it exists in the FROM header).  Comment 0 explains why.

2) Ignore case in email addresses.

3) If it's in AB, show only Display Name to user (according to user pref),
   but tooltip could show the original FROM header's Display Name & email
   address.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We should also keep in mind bug 381555 which let us search in each address book. Perhaps that should be fixed before this issue.

Comment 14

[Boying Lu]

Thanks a lot for your feedback. I'm making a new patch according to your suggestion.
> 
> 2) Ignore case in email addresses.

I don't quite understand this. Can you explain more detailed? Thanks 

Comment 15

[Pete Riley]

(In reply to comment #14)
> > 2) Ignore case in email addresses.
> 
> I don't quite understand this. Can you explain more detailed? Thanks 

Sorry, I meant to say:  use case-insensitive text comparisons.  That is, if the FROM header contains [e-mail], that should find [e-mail] or [e-mail] in the address book.

Comment 16

[Boying Lu]

Attachment: lookup in addressbook first and ignore case in email address

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Boying, thank you for the patch. I build my debug version with your patch applied. There are some issues and questions:

* Why you have added the same code to msgHdrViewOverlay.js and nsMsgDBView.cpp? Second one to use the display name in the sender column of the thread pane? David, is that what we want? At the moment we still show the senders name and with the patch applied it's really broken at the moment (added '<' and '>' around the address and empty if no name is given e.g. [e-mail]). If we want this we should IMO separate this part into another bug.

* Display name is always used when pref is enabled - even if the email address is not listed inside any of the address books.

* Tooltips over display name dont't work anymore in header pane.

Comment 18

[Boying Lu]

Sorry for late reply, I was on national holiday last week. Please see my comments below.

(In reply to comment #17)
> Boying, thank you for the patch. I build my debug version with your patch
> applied. There are some issues and questions:
> 
> * Why you have added the same code to msgHdrViewOverlay.js and nsMsgDBView.cpp?

The code in js file is to handle message headers e.g. "From","To" fields
The code in cpp file is to handle "From" field in thread panel

> Second one to use the display name in the sender column of the thread pane?
Yes.

> David, is that what we want? At the moment we still show the senders name and
> with the patch applied it's really broken at the moment (added '<' and '>'
> around the address and empty if no name is given e.g. [e-mail]). If we want
> this we should IMO separate this part into another bug.

Can you explain further about this? I don't quite understand.
> 
> * Display name is always used when pref is enabled - even if the email address
> is not listed inside any of the address books.

Yes, any other suggestions?
> 
> * Tooltips over display name dont't work anymore in header pane.
> 

I built the latest thunderbird trunk build without this patch, The "tooltips" doesn't work. So I think this is a separated bug.

Comment 19

[the gladiator]

Fantastic news !!! Boing, thanks a lot for your patch. IMHO we would need just two additional actions to close the loop:
1)could you (or some other guru) write a short guide on how to use your patch ?
2)could you exactly say which are the expected results of your patch ?

With  these information we could double check and enjoying the patch knowing that we have correctly implemented it. Thanks in advance

Comment 20

[the gladiator]

Fantastic news !!! Boying, thanks a lot for your patch. IMHO we would need just two additional actions to close the loop:
1)could you (or some other guru) write a short guide on how to use your patch ?
2)could you exactly say which are the expected results of your patch ?

With  these information we could double check and enjoying the patch knowing that we have correctly implemented it. Thanks in advance

Comment 21

[Boying Lu]

(In reply to comment #20)
> Fantastic news !!! Boying, thanks a lot for your patch. IMHO we would need just
> two additional actions to close the loop:
> 1)could you (or some other guru) write a short guide on how to use your patch ?

To use the patch:
0. save the patch into some direcotry say foo/patch.diff
1. cd to mozilla directory
2. gpatch -p0 < foo/patch.diff
3. build thunderbird

> 2)could you exactly say which are the expected results of your patch ?

I did following testing on this patch

test case 0:

check "Edit"-->" Prefernce" --> "Advanced" --> "General" --> "Show only display name for people in my address book"

expected result: 
In "From" column in the thread panel and "From", "To", "CC" and "cc" fields, display name should only be shown instead of e-mail addresses. e.g. Brian Lu <[e-mail]> should 
be shown as "Brian Lu" instead of "Brian Lu <[e-mail]>"

If uncheck it, e-mail addresses should also be shown.

test case 1:
0. create a new address book
1. add my another e-mail address [e-mail] (whose display name is set to "Eagle Lu") into it and set the display name to "Hawk Lu"
2. check the check box

expected result:
Those fields of e-mails from [e-mail] are shown as "Hawk Lu" instead of "Eagle Lu" 
or "Eagle Lu <[e-mail]>"

This case shows that the patch also fix the bug 381555

Hope this can be helpful to you.  Thanks

Comment 22

[the gladiator]

Boying thanks a lot for your prompt reply.

Expected results and test case are very well stated and clear.

I suppose that the patch implementation you're referring to is for a Linux system (at least I hope !). What about a Windows OS ? 

Comment 23

[David :Bienvenu]

If I'm reading the patch correctly, it looks like for every address we try to display in the message header, you iterate over all the local AB's trying to look it up, even if the gShowCondensed pref is false - that sounds like a bad performance hit - didn't we recently have a performance problem that was caused by similar behavior?

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

David, could you please have a look at comment 17 where pointed out two other things in relation to the latest patch?

Comment 25

[Mark Banner (:standard8)]

Looks like I got cc'ed on this bug for some comments...

David is right in comment 23 - you shouldn't iterate over all the local abs if the gShowCondensed pref is false - its just a waste of time even if the perf impact is small.

Some comments about the patch in general:

+      // search through all of local address books looking for a match.
+      var parentDir = RDF.GetResource("moz-abdirectory://").QueryInterface(Components.interfaces.nsIAbDirectory);
+      var enumerator = parentDir.childNodes;
+      var cardForEmailAddress;
+      var addrbook;
+
+      while (!cardForEmailAddress && enumerator.hasMoreElements())
+      {
+        addrbook = enumerator.getNext();
+        if (addrbook instanceof Components.interfaces.nsIAbMDBDirectory)
+          cardForEmailAddress = addrbook.cardForEmailAddress(address.emailAddress);
+
+        if (cardForEmailAddress)
+        {
+          /* find the first one,jump out of the loop */
+          address.displayName = cardForEmailAddress.displayName;
+          break;
+        }
+      }

parentDir should be initialised outside of the loop, otherwise you're unnecessarily reinitialising it each time.

Please initialise cardForEmailAddress to null. Its clearer what is happening.

As you are checking cardForEmailAddress as the loop parameter you don't need to do the additional check for if (cardForEmailAddress) within the loop. Just exit the loop then do the check. You can also drop the break then, which makes things a lot clearer.

You would then be able to simplify the if statements after the loop as well.

A subset of these comments apply to the c++ code as well.

I'm starting to thing that we could do with the search for any address function into the nsIAddressBook interface as there are now (or will be) several places where we are using this pattern.

Comment 26

[Boying Lu]

Attachment: patch v1

Thanks all for the comments. 
I re-made the patch according to your comments and the "while" loop in my previous  patch (in msgHdrViewOverlay.js) is incorrect and I fixed it in this patch

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

There are still some unresolved issues:

1. Pref disabled:

* All the headers inside the message pane only show the display name from the given mail address and not the whole address. Even if the person is not in my address book I only see the display name.

* The from column within the thread pane only showed the Senders name without the mail address before. Now anytime the address is added within '<>'. Some of them looks like 'Foo Bar <[e-mail]>' and others like '"Foo Bar" <[e-mail]>'. David, I don't know if these changes are what we want. But if it is so they should be displayed with an identical look.

2. Pref enabled:

* All the headers within the message pane display the name which is given by the mail address instead of using the local display name from the address book. I sent a message to "test <[e-mail]>" and assigned Foobar as display name. But test is shown.

* Same issue as above with '"'.

3. All:

* Tooltips aren't displayed anymore.

* Recipient column of thread pane is not affected by the pref change. It still shows the given recipients name.

David, it would be nice if you could have a look at the current patch and my given concerns. An official statement would be helpful at this stage I think.

Comment 28

[David :Bienvenu]

I really don't like searching through all the users address books in code that's essentially executed from the thread pane paint code, which nsMsgDBView::FetchAuthor is.  The performance impact could be bad, especially for things like scrolling. The issues you mention, Henrik, sound bad, but I'll let Boying address those.

Comment 29

[Boying Lu]

(In reply to comment #28)
> I really don't like searching through all the users address books in code
> that's essentially executed from the thread pane paint code, which
> nsMsgDBView::FetchAuthor is.  The performance impact could be bad, especially
> for things like scrolling. The issues you mention, Henrik, sound bad, but I'll
> let Boying address those.
> 
ok, i'll continue work on this

Comment 30

[the gladiator]

Just a few lines to give my help:

(In reply to comment #27)
> There are still some unresolved issues:
> 
> 1. Pref disabled:
> 
> * All the headers inside the message pane only show the display name from the
> given mail address and not the whole address. Even if the person is not in my
> address book I only see the display name.
>

Actually this is the behavior for the current official release. An extension is
available if you want to display the email address (
https://addons.mozilla.org/en-US/thunderbird/addon/3492 ). I believe the patch
doesn't have any impact and works properly on this point.   

> * The from column within the thread pane only showed the Senders name without
> the mail address before. Now anytime the address is added within '<>'. Some of
> them looks like 'Foo Bar <[e-mail]>' and others like '"Foo Bar"
> <[e-mail]>'. David, I don't know if these changes are what we want. But
> if it is so they should be displayed with an identical look.
>

No, this is not what we expect (we would like to see "Foo Bar" without " in
your example). What we expect is listed in Boying's comment #21.

> 2. Pref enabled:
> 
> * All the headers within the message pane display the name which is given by
> the mail address instead of using the local display name from the address book.
> I sent a message to "test <[e-mail]>" and assigned Foobar as display
> name. But test is shown.
> 
> * Same issue as above with '"'.
> 
> 3. All:
> 
> * Tooltips aren't displayed anymore.
>

Personally I don't care.

> * Recipient column of thread pane is not affected by the pref change. It still
> shows the given recipients name.
> 

Henrick, if you send me the patched version of TB (any platform) I can help
with some test.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #30)
> > * All the headers inside the message pane only show the display name from the
> > given mail address and not the whole address. Even if the person is not in my
> > address book I only see the display name.
> 
> Actually this is the behavior for the current official release. An extension 

No. It is not. I talk about the message pane and not about the thread pane. Only the thread pane hides the mail address and shows the senders name (not display name) at the moment. But I have to correct myself: The message pane doesn't show the display name but the senders name. There should be the whole address visible.

> Henrick, if you send me the patched version of TB (any platform) I can help
> with some test.

Sorry, its a local debug build and too big for sending via mail. I'm currently away from home that's why I cannot offer it as download.

Comment 32

[Boying Lu]

(In reply to comment #27)
> There are still some unresolved issues:
> 
> 1. Pref disabled:
> 
> * All the headers inside the message pane only show the display name from the
> given mail address and not the whole address. Even if the person is not in my
> address book I only see the display name.

Yes, I saw these problems. I'll work on these issues

> 
> * The from column within the thread pane only showed the Senders name without
> the mail address before. Now anytime the address is added within '<>'. Some of
> them looks like 'Foo Bar <[e-mail]>' and others like '"Foo Bar"
> <[e-mail]>'. David, I don't know if these changes are what we want. But
> if it is so they should be displayed with an identical look.

I don't quiet understand this. Can you explain further?
> 
> 2. Pref enabled:
> 
> * All the headers within the message pane display the name which is given by
> the mail address instead of using the local display name from the address book.
> I sent a message to "test <[e-mail]>" and assigned Foobar as display
> name. But test is shown.

I can't re-produce this issue. Can you try with a fresh profile?
> 
> * Same issue as above with '"'.
> 
> 3. All:
> 
> * Tooltips aren't displayed anymore.

I think this is a separated issue. tooltips don't work on current trunk code.

> 
> * Recipient column of thread pane is not affected by the pref change. It still
> shows the given recipients name.

I saw this and will work on this

> 
> David, it would be nice if you could have a look at the current patch and my
> given concerns. An official statement would be helpful at this stage I think.
> 

Thanks a lot

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #32)
> > * The from column within the thread pane only showed the Senders name without
> > the mail address before. Now anytime the address is added within '<>'. Some of
> > them looks like 'Foo Bar <[e-mail]>' and others like '"Foo Bar"
> > <[e-mail]>'. David, I don't know if these changes are what we want. But
> > if it is so they should be displayed with an identical look.
> 
> I don't quiet understand this. Can you explain further?

The issue that I had with the <> I cannot see anymore. After a clobber it is gone. But the quotes are still shown for some addresses. This will be happen if the header contains quotes e.g. 'To: "Foobar" <[e-mail]>'. This quotes should be eleminated.

> > * All the headers within the message pane display the name which is given by
> > the mail address instead of using the local display name from the address book.
> > I sent a message to "test <[e-mail]>" and assigned Foobar as display
> > name. But test is shown.
> 
> I can't re-produce this issue. Can you try with a fresh profile?

Sorry my fault. I can't reproduce it even on my machine.

> > * Tooltips aren't displayed anymore.
> 
> I think this is a separated issue. tooltips don't work on current trunk code.

No, running the latest nightly build shows me the tooltips when using the same profile. But I found the issue why they are not shown with your patch. Normally what's current on trunk it only searches for display names within the first address book. So tooltips are always shown when a display name is used. But now we are searching through each address book. If a display name from another address book is used no tooltip is shown. I think it's also limited to the first address book and should be fixed within this bug.

Comment 34

[Boying Lu]

Comment on attachment 289806 [details] [diff] [review]
patch v1

I made a new patch. After some testing, I'll post it here

Comment 35

[Boying Lu]

Comment on attachment 289806 [details] [diff] [review]
patch v1

I made a new patch. After some testing, I'll post it here

Comment 36

[Boying Lu]

Attachment: new patch

I've addresses all issues in Comment #27

Comment 37

[Mark Banner (:standard8)]

Comment on attachment 292379 [details] [diff] [review]
new patch 

I've just taken another look at this and picked up on a few more things:

+      var enumerator = parentDir.childNodes;
+      var cardForEmailAddress = null;
+      var addrbook;
+
+      while (gShowCondensedEmailAddresses && enumerator.hasMoreElements())

parentDir.childNodes can be expensive, so drop the gShowCondensedEmailAddress from the while loop and wrap the var definitions and while in one if (gShowCondensedEmailAddres) statement.

+        addrbook = enumerator.getNext();
+        if (addrbook instanceof Components.interfaces.nsIAbMDBDirectory)
+          cardForEmailAddress = addrbook.cardForEmailAddress(address.emailAddress);
+
+        if (cardForEmailAddress)
+        {
+          /* find the first one,jump out of the loop */
+          address.displayName = cardForEmailAddress.displayName;
+          break;
+        }

+       nsCOMPtr<nsIAbCard> cardForAddress = nsnull;

You don't need the nsnull here - a nsCOMPtr automatically initialises to null.

+         if (mdbDirectory)
+           mdbDirectory->CardForEmailAddress(emailAddress.get(), getter_AddRefs(cardForAddress));
+
+         if (cardForAddress)
+           cardForAddress->GetDisplayName(localDisplayName);

It may make the code a little more indented, but why not do:

if (mdbDirectory)
{
  mdbDirectory->CardForEmailAddress(...

  if (cardForAddress
  {
     cardForAddress->GetDisplayName(...
     if (!localDisplayName.isEmpty(...
     {
        ...
     }
  }
}

As then you'll avoid the unnecessary checks if you've not got an mdbDirectory.

+            names += NS_LITERAL_CSTRING(",")+name;

We're converting to the frozen API so that we'll eventually be able to build with libxul. This isn't compliant to that interface, please could you change it to:

{
  names.AppendLiteral(",");
  names.Append(name);
}

(ditto in other location).

+    }else {

missing space before else.

I haven't tried it out yet - david may have some more comments first.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #37)
> I haven't tried it out yet - david may have some more comments first.

I hope that I have time to build a patched version this evening. So I could give information about possible visible issues.

But what I'm also interested in: Why I get an empty result when I diff the latest two patches?

https://bugzilla.mozilla.org/attachment.cgi?oldid=289806&action=interdiff&newid=292379&headers=1

Comment 39

[Boying Lu]

Attachment: patch

First thanks very much for Mark's comment, I re-made the patch based on his comment

Comment 40

[Boying Lu]

(In reply to comment #37)
> +         if (mdbDirectory)
> +           mdbDirectory->CardForEmailAddress(emailAddress.get(),
> getter_AddRefs(cardForAddress));
> +
> +         if (cardForAddress)
> +           cardForAddress->GetDisplayName(localDisplayName);
> 
> It may make the code a little more indented, but why not do:
> 
> if (mdbDirectory)
> {
>   mdbDirectory->CardForEmailAddress(...
> 
>   if (cardForAddress
>   {
>      cardForAddress->GetDisplayName(...
>      if (!localDisplayName.isEmpty(...
>      {
>         ...
>      }
>   }
> }
> 
> As then you'll avoid the unnecessary checks if you've not got an mdbDirectory.
> 
I can't quite understand this comment, even I move these codes, I still need to check if mdbDirectory is NULL or not.

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #39)
> Created an attachment (id=292580) [details]
> patch

This patch doesn't compile:

/mailnews/base/src/nsMsgDBView.cpp:432: error: no matching function for call to 'nsDerivedSafe<nsIAbMDBDirectory>::CardForEmailAddress(const char*, nsGetterAddRefs<nsIAbCard>)'

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 292580 [details] [diff] [review]
patch

>--- mozilla/mail/base/content/msgHdrViewOverlay.js.old	2007-12-11
>+ mdbDirectory->CardForEmailAddress(emailAddress.get(), getter_AddRefs(cardForAddress));

Should be mdbDirectory->cardForEmailAddress.

Please test your patch before attaching it here. Also please have a look at word wrapping. You have some too long lines.

And a note: could you also please do a p0 diff directly from within the mozilla directory? That would be nice.

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #42)
> Should be mdbDirectory->cardForEmailAddress.

Err, please forget this comment. That's not the solution.

In bug 407564 this was moved to frozen API, right? Now the email address is a nsACString.

Comment 44

[Mark Banner (:standard8)]

(In reply to comment #43)
> In bug 407564 this was moved to frozen API, right? Now the email address is a
> nsACString.

No it wasn't moved to frozen API (both the old and previous versions could have been used with frozen API). The interface was improved to handle non-pure ASCII addresses better which is why it was changed to a AString (in c++ code this is nsACString).

This line:
+           mdbDirectory->CardForEmailAddress(emailAddress.get(), getter_AddRefs(cardForAddress));

just needs to drop the .get() after the emailAddress.

Comment 45

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 292580 [details] [diff] [review]
patch

>+       PRBool aBool;

Not used anymore.

>+       for ( int i = 0; i < numAddresses; i++) 

Isn't it better to also work with PRUint32 for i here? Exists twice in that patch.

Mark, thanks for clearifying.

Comment 46

[Boying Lu]

Attachment: patch again

This patch is made according to Hernik and Marks' comments and based on latest trunk code and did with p0 diff. Thanks for all comments

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I applied the patch and found following remaining issues:

* If you open a message from a person you don't have in your address book the header pane only displays the senders name. Shouldn't we always display the whole address here? Further within the thread pane the whole address is shown. Both should be exchanged. I'll attach a screenshot.

* If you open a message from a person you don't have in your address book and add the email address to the address book, the display name within the thread pane is updated but not within the header pane. Reopening the message shows the correct display name. Do we need to reload the message here?

* Same happens when you remove an address from the address book. The thread pane gets updated while the header pane still shows the display name of the non-existing contact.

* Searching for the display name of an email address within the OS X system address book doesn't work. The display name isn't shown inside the header and thread pane while the address book itself still shows the correct display name.

Sorry Boying that it is a longer list again.

Comment 48

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Display names for not existing contacts between TB2 and TB3

This image shows the different showing of the display name between Thunderbird 2 (above) and Thunderbird 3. The complete address should be displayed within the header pane and not within the thread pane. With the latest patch we change the current behavior.

Comment 49

[Boying Lu]

(In reply to comment #48)
> Created an attachment (id=294542) [details]
> Display names for not existing contacts between TB2 and TB3
> 
> This image shows the different showing of the display name between Thunderbird
> 2 (above) and Thunderbird 3. The complete address should be displayed within
> the header pane and not within the thread pane. With the latest patch we change
> the current behavior.
> 
THanks a lot for your comments. I'll address on these issues and post a new patch

Comment 50

[Boying Lu]

Attachment: patch v2

The patch fixes issues in comment #47 (except the last one).

Comment 51

[Boying Lu]

> * Searching for the display name of an email address within the OS X system
> address book doesn't work. The display name isn't shown inside the header and
> thread pane while the address book itself still shows the correct display name.
> 
Sorry I don't have OS X system. I'll try to find one.

Comment 52

[Mark Banner (:standard8)]

(In reply to comment #51)
> > * Searching for the display name of an email address within the OS X system
> > address book doesn't work. The display name isn't shown inside the header and
> > thread pane while the address book itself still shows the correct display name.
> > 
> Sorry I don't have OS X system. I'll try to find one.

It won't work because of:

+       nsCOMPtr<nsIAbMDBDirectory> mdbDirectory;
...
+       while (NS_SUCCEEDED(enumerator->HasMoreElements(&hasMore)) && hasMore && !cardForAddress)
+       {
+         rv = enumerator->GetNext(getter_AddRefs(supports));
+         NS_ENSURE_SUCCESS(rv, rv);
+         mdbDirectory = do_QueryInterface(supports);
+         if (mdbDirectory)
+           mdbDirectory->CardForEmailAddress(emailAddress,getter_AddRefs(cardForAddress));

So the code will only work with Mork-based local address books and not anything else (OSX/Outlook/LDAP). This really needs bug 359352 fixing as then you'd be able to do it on a implementation-independent basis.

One additional thought, cardForEmailAddress checks against the second email address as well as the primary, so you'll get both matches, I assume that's ok here?

Comment 53

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #52)
> One additional thought, cardForEmailAddress checks against the second email
> address as well as the primary, so you'll get both matches, I assume that's ok
> here?

Could that happen if the first and the second address are the same for a card? Otherwise we stop after the first card found?

So if bug 359352 needs to be fixed we can still rely on the current patch here and check it in ASAP. The remaining patch on bug 359352 will automatically fix the missing feature?

Comment 54

[Mark Banner (:standard8)]

(In reply to comment #53)
> Could that happen if the first and the second address are the same for a card?
> Otherwise we stop after the first card found?

cardForEmailAddress will always find the first card that matches either first or second email address - it doesn't check for two the same.

> So if bug 359352 needs to be fixed we can still rely on the current patch here
> and check it in ASAP. The remaining patch on bug 359352 will automatically fix
> the missing feature?

I would expect the remaining patch on bug 359352 would fix the missing feature, but it would need to do some more changes to this code.

Comment 55

[Boying Lu]

I see the patch for bug 359352 has been passed review. Can anyone check it in?
So I can make a new base based on it.

Comment 56

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Boying, the work on bug 359352 is only done half at the moment. The move of the function cardForEmailAddress from nsIAbMDBDirectory to nsIAbDirectory isn't done yet. So the question is checking in your patch meanwhile gives us the change to test it with local databases. We can leave this bug open and do the last part later after bug 359352 is fixed.

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #56)
> Boying, the work on bug 359352 is only done half at the moment. The move of the
> function cardForEmailAddress from nsIAbMDBDirectory to nsIAbDirectory isn't
> done yet. So the question is checking in your patch meanwhile gives us the
> change to test it with local databases. We can leave this bug open and do the
> last part later after bug 359352 is fixed.

Mark, what do you think about?

Comment 58

[Mark Banner (:standard8)]

(In reply to comment #57)
> (In reply to comment #56)
> > Boying, the work on bug 359352 is only done half at the moment. The move of the
> > function cardForEmailAddress from nsIAbMDBDirectory to nsIAbDirectory isn't
> > done yet. So the question is checking in your patch meanwhile gives us the
> > change to test it with local databases. We can leave this bug open and do the
> > last part later after bug 359352 is fixed.
> Mark, what do you think about?

I don't mind too much either way. I think by TB 3.0a1 (4 weeks) we'll probably have that function moved (by bug 413260), but that doesn't mean to say we can't go ahead and do this patch anyway, especially as there's other areas that are currently specific to mdb anyway.

Comment 59

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

This looks great!  I'm hopeful we can get something like this into TB.  And I'd like to try turning this behavior on by default if we can get it in for an alpha release and get some further feedback.

Here's my best summary of the situations we want to see, repeating much of the discussion above.

* Any message containing persons not in your address book should show their entire name+email display in any of the address fields.  While only the display name should be shown for people who are in your address book.
** From: Person Not In AddressBook <[e-mail]>
** To: Person Not In AddressBook <[e-mail]>
** CC: Person In AddressBook, Person Not In AddressBook <[e-mail]>

In the future I'd like to think about adding an icon, likely similar to the ff3 bookmarks icon, along side contacts to indicate if a contact is in the address book or not.

Comment 60

[Neil]

I'm happy to test this on OS X (though I might need a little build help).
And I'd love to see this make it into the Alphas if at all possible.

Comment 61

[Serge Gautherie (:sgautherie)]

Comment on attachment 295066 [details] [diff] [review]
patch v2

>+      while (gShowCondensedEmailAddresses && enumerator.hasMoreElements())

At least this part of comment 37 wasn't addressed...

Comment 62

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #58)
> I don't mind too much either way. I think by TB 3.0a1 (4 weeks) we'll probably
> have that function moved (by bug 413260), but that doesn't mean to say we can't
> go ahead and do this patch anyway, especially as there's other areas that are
> currently specific to mdb anyway.

Joshua, do we need bug 449618 now before we can fix this issue? If yes it should be marked as depends.

Comment 63

[Joshua Cranmer [:jcranmer]]

As far as I can tell, this feature is partially implemented in the UI. The actual nsIAbCard is the primary bitrotting factor; the change in bug 449618 merely moves a function up a level, so that, from the JS point of view, the only thing that has to happen is that QueryInterface's have to be changed or removed.

So, in short, this is orthogonal (AIUI) of bug 449618. Although people might rather make some changes so that it isn't only the Personal Address Book that is queried, but that's a different issue.

Comment 64

[Mark Banner (:standard8)]

I have already said in comment 58 that this could go ahead either side of implementing bug 359352/bug 4449618.

As others comment 59 and comment 61 have pointed out, the patch still needs some work.

Comment 65

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Boying are you willed to update your patch? Or are you time limited at the moment?

Comment 66

[Boying Lu]

(In reply to comment #65)
> Boying are you willed to update your patch? Or are you time limited at the
> moment?
> 
Sorry for late reply.  I'm making a new patch and will post it here soon

Comment 67

[Boying Lu]

Attachment: new patch based on the latest trunk (comm-central) code

Comment 69

[Mark Banner (:standard8)]

Comment on attachment 333196 [details] [diff] [review]
new patch based on the latest trunk  (comm-central) code

>diff -r 9d26fe956df7 mailnews/base/src/nsMsgDBView.cpp
>@@ -374,28 +379,79 @@
> nsresult nsMsgDBView::FetchAuthor(nsIMsgDBHdr * aHdr, nsAString &aSenderString)
> nsresult nsMsgDBView::FetchRecipients(nsIMsgDBHdr * aHdr, nsAString &aRecipientsString)

I'm concerned about the performance aspect here.

I have just loaded an address book with 5000 addresses, as I now scroll a message view (9 messages showing, 22 total) it is noticeably sluggish, and CPU is climbing...

For the code I've already looked at above which just does the message headers, its not too bad, I don't think the lag is that perceivable, though we could really do with a way to measure it.


>diff -r 9d26fe956df7 mail/base/content/msgHdrViewOverlay.js

>+    address.showOnlyDisplayName = false;
>+    
>+    // search through all of local address books looking for a match.
>+    var enumerator = abManager.directories;
>+    var cardForEmailAddress = null;
>+    var addrbook;
>+    
>+    if (gShowCondensedEmailAddresses)
>+    {
>+      while (enumerator.hasMoreElements())
>+      {
>+        addrbook = enumerator.getNext();
>+        if (addrbook instanceof Components.interfaces.nsIAbMDBDirectory)
>+	  cardForEmailAddress = addrbook.cardForEmailAddress(address.emailAddress);

nit: There's tabs here, which I think is causing the indentation to be wrong.

>+
>+        if (cardForEmailAddress && cardForEmailAddress.displayName)
>+        {
>+          /* find the first one,jump out of the loop */

nit: missing space after comma.

>+          address.displayName = cardForEmailAddress.displayName;
>+          address.showOnlyDisplayName =  true;

nit: Only one space between = and true please.

>diff -r 9d26fe956df7 mail/components/addrbook/content/addressbook.js
>--- a/mail/components/addrbook/content/addressbook.js	Thu Aug 07 08:53:45 2008 +0100
>+++ b/mail/components/addrbook/content/addressbook.js	Mon Aug 11 15:11:55 2008 +0800
>@@ -119,16 +119,25 @@
> 
> function OnUnloadAddressBook()
> {
>   Components.classes["@mozilla.org/abmanager;1"]
>             .getService(Components.interfaces.nsIAbManager)
>             .removeAddressBookListener(gAddressBookAbListener);
> 
>   CloseAbView();
>+
>+  // refresh message pane
>+  var windowManager = Components.classes['@mozilla.org/appshell/window-mediator;1'].getService();
>+  var windowManagerInterface = windowManager.QueryInterface(nsIWindowMediator);
>+  var topWindow = windowManagerInterface.getMostRecentWindow( "mail:3pane");
>+
>+  if (topWindow)
>+    topWindow.ReloadMessage();
>+

What is this for? If it is refreshing the mail message window then I'm not sure its the right way to go around it. The Mail Window should listen to changes in the address book. If necessary, I'm happy to look at this in a follow-up bug.

>diff -r 9d26fe956df7 mailnews/base/resources/content/mailWidgets.xml
>--- a/mailnews/base/resources/content/mailWidgets.xml	Thu Aug 07 08:53:45 2008 +0100
>+++ b/mailnews/base/resources/content/mailWidgets.xml	Mon Aug 11 15:11:55 2008 +0800
>@@ -256,17 +256,19 @@
>       </method>
> 
>       <!-- updateEmailAddressNode: private method used to set properties on an address node -->
>       <method name="updateEmailAddressNode">
>         <parameter name="aEmailNode"/>
>         <parameter name="aAddress"/>
>         <body>
>           <![CDATA[
>-            if (aEmailNode.parentNode.useShortView && aAddress.displayName)
>+	    if ((aAddress.showOnlyDisplayName ||
>+	         aEmailNode.parentNode.useShortView) && aAddress.displayName
>+               )

nit: no tabs please. The bracket should also be on the end of the line like it was originally.

>   if (mHeaderParser)
>   {
>-    nsCString name;
>-    rv = mHeaderParser->ExtractHeaderAddressName("UTF-8", NS_ConvertUTF16toUTF8(unparsedAuthor).get(), getter_Copies(name));
>-    if (NS_SUCCEEDED(rv) && !name.IsEmpty())
>-    {
>+    nsCString name,displayName,emailAddress;
>+    PRUint32 numAddresses;
>+       
>+    /* get display name and mailbox from message header */
>+    mHeaderParser->ParseHeaderAddresses("UTF-8", NS_ConvertUTF16toUTF8(unparsedAuthor).get(), 
>+    getter_Copies(displayName),getter_Copies(emailAddress), &numAddresses); 

This is wrong. ParseHeaderAddresses will return a list of null-terminated addresses, so you'll only get the first one here and I think you will leak memory when it comes to freeing it. parseHeadersWithArray may be a better option here as could we cut down on some of the conversions from UTF16 to UTF8?

>+    rv = mHeaderParser->MakeFullAddressString(displayName.get(),emailAddress.get(),
>+                                              getter_Copies(name));

I don't see why you're doing this here (the MakeFullAddressString). Surely we only want to do it in the !ShowCondensedEmailAddress case?

>+       nsCOMPtr<nsISupports> supports;
>+       nsCOMPtr<nsIAbMDBDirectory> mdbDirectory;
>+       nsCOMPtr<nsIAbCard> cardForAddress;
>+       PRBool hasMore;
>+       nsString localDisplayName;
>+
>+       /* first look up the diaplay name in all address books
>+        * return the first one 
>+        */
>+       while (NS_SUCCEEDED(enumerator->HasMoreElements(&hasMore)) && hasMore && !cardForAddress)
>+       {
>+         rv = enumerator->GetNext(getter_AddRefs(supports));
>+	 NS_ENSURE_SUCCESS(rv, rv);
>+	 mdbDirectory = do_QueryInterface(supports);

nit: tabs again.

>+   
>+      nsCOMPtr<nsISimpleEnumerator> enumerator;
>+      abManager->GetDirectories(getter_AddRefs(enumerator));
>+      NS_ENSURE_SUCCESS(rv, rv);

You've missed assigning to rv.

>+	 while (NS_SUCCEEDED(enumerator->HasMoreElements(&hasMore)) && hasMore && !cardForAddress)
>+	 {
>+	   rv = enumerator->GetNext(getter_AddRefs(supports));
>+	   NS_ENSURE_SUCCESS(rv, rv);
>+	   mdbDirectory = do_QueryInterface(supports);
>+	   NS_ENSURE_SUCCESS(rv, rv);

You don't need this NS_ENSURE_SUCCESS here, it may fail, and in that case we just want to continue.

>+	   if (mdbDirectory)
>+	     mdbDirectory->CardForEmailAddress(nsCString(pAddresses), getter_AddRefs(cardForAddress));
>+	    
>+	   if (cardForAddress)
>+	     cardForAddress->GetDisplayName(displayName);
>+	    
>+	   if (!displayName.IsEmpty())
>+	   {

if cardForAddress is null, there's no point in checking the !displayName.IsEmpty.

>+	 /* if the recipient is not in the address book
>+	  * use default  
>+	  */
This can go on one line and just use the // style please.

Comment 70

[Boying Lu]

Mark, thanks a lot for review. I'm making a new patch and will post it here when ready.

Comment 71

[Mark Banner (:standard8)]

Boying, any chance you could separate this patch into two parts, one for the message header display and one for the view changes?

I'm concerned about the performance for the view changes, but I could also really do with having the message header display fixes in (where I think there isn't an issue).

We can therefore drive the header display changes in, and address the view changes in slower time.

Comment 72

[Boying Lu]

Attachment: patch for message header disaplay

Comment 73

[Boying Lu]

Attachment: patch for update whole message window

I'll give out a better solution

Comment 74

[Mark Banner (:standard8)]

(In reply to comment #72)
> Created an attachment (id=333893) [details]
> patch for message header disaplay

I'm really sorry for not having noticed this before. mailWidgets.xml calls AddExtraAddressProcessing() in msgHdrViewOverlay.xul, this function already checks to see if the card is in the address book (via the useDisplayNameForAddress function) if gShowCondensedEmailAddresses is set.

Therefore I think all you really need to do is in AddExtraAddressProcessing(), capture the return value of useDisplayNameForAddress (maybe rename that function), and then if that card has a display name, change the displayName in the address node.

Here's the lines I'm talking about marked up against the old cvs code (hg hasn't got this facility yet, but that code hasn't changed):

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mail/base/content/msgHdrViewOverlay.js&rev=1.109&mark=942,953,955#940

Don't worry about useDisplayNameForAddress only matching against one address book, I can fix that up in a separate bug - as there is at least one other bug touching that function at the moment.

Could you take a look and see what you think?

Comment 75

[Boying Lu]

Attachment: new patch using useDisplayNameForAddress()

I had thought about using  useDisplayNameForAddress(), but it only checks "Personal address book". Now I modified the patch to use it. 

Please add my e-mail address to the CC list when you file a bug against  useDisplayNameForAddress().

Comment 76

[Mark Banner (:standard8)]

Comment on attachment 333915 [details] [diff] [review]
new patch using  useDisplayNameForAddress()

+    var card = useDisplayNameForAddress(emailAddress); 
+    var displayName = card.displayName;

useDisplayNameForAddress may return false (or possibly even null), hence we're going to throw an error on the second line here. So I think you should detect that and just return out of the function.

Apart from that, it looks good.

Comment 77

[Boying Lu]

Attachment: new patch

Comment 78

[Mark Banner (:standard8)]

Comment on attachment 334239 [details] [diff] [review]
new patch

Two minor nits:

+    var card = useDisplayNameForAddress(emailAddress); 

No space on the end of this line please.

+    if (card && card.displayName)

You don't need to check card.displayName here, because you're checking it further down anyway (and you're not setting displayName to anything else).

Comment 79

[Mark Banner (:standard8)]

(In reply to comment #77)
> Created an attachment (id=334239) [details]
> new patch

Also, as this patch is in mail/ only you won't need sr for it.

Comment 80

[Boying Lu]

Attachment: [checked in] patch for message header display

Mark, if possible, can you help me to check in the patch? I don't have permission to do this.

Comment 81

[Mark Banner (:standard8)]

Comment on attachment 334253 [details] [diff] [review]
[checked in] patch for message header display

Thanks Boying, patch checked in. Changeset id: 127:a7881593c05a.

Comment 82

[Boying Lu]

Attachment: update mail window when addressbook is changed

Comment 83

[Boying Lu]

Attachment: update mail window when addressbook is changed

Comment 84

[Mark Banner (:standard8)]

Comment on attachment 336302 [details] [diff] [review]
update mail window when addressbook is changed

I'm really sorry about this, I forgot to tell you that I have a patch proposed on bug 450724 which includes updating the message header if the address book changes.

The patch has the advantage that it only updates the required emails and doesn't reload the whole message.

Sorry that we wasted time here.

Comment 85

[David Ascher (:davida)]

Mark, it sounds like this bug should be closed at this point?

Comment 86

[Boying Lu]

(In reply to comment #85)
> Mark, it sounds like this bug should be closed at this point?
No, the patch only fix the message header part, thread pane still has this problem.

Comment 87

[the gladiator]

I must admit that I've lost the point where we are but...I've found by chance this TB add-on https://addons.mozilla.org/en-US/thunderbird/addon/5296 : doesn't it solve the bug ?

Comment 89

[Jim Porter (:squib)]

It sounds like what remains in this bug is the same as bug 312821. I'd really love to get this fixed, but I'm not sure what the best way would be in order to maintain sane performance. Maybe caching the mapping from header values to display names would be enough? (Also, I'd really like to just do the display name processing in Javascript, but I'm not sure how feasible it is to call JS from C++.)

Comment 90

[Boying Lu]

Attachment: new patch for thread pane

Comment 91

[Jim Porter (:squib)]

Is it possible to pull the display name out from Javascript? That would let us use FormatDisplayName from bug 474721, which would be more consistent.

Comment 92

[Boying Lu]

(In reply to comment #91)
> Is it possible to pull the display name out from Javascript? That would let us
> use FormatDisplayName from bug 474721, which would be more consistent.

So you mean to use FormatDisplayName() in the threadPane.js?

Comment 93

[Mark Banner (:standard8)]

Comment on attachment 484634 [details] [diff] [review]
new patch for thread pane

I've got a lot on at the moment, so redirecting this to David.

My biggest concern here is the performance of the message list - as we're going to be pulling a lot of items out of the address book.

Comment 94

[Boying Lu]

> My biggest concern here is the performance of the message list - as we're going
> to be pulling a lot of items out of the address book.

I don't think the performance is an issue. My patch saves the results in "sender_name" and "recipient_names". As long as user didn't change the "Show only display name for people in my address book", (User seldom changes this I think), the values save in "sender_name" and "recipient_names" are used directly no further computation is needed.

Comment 95

[Jim Porter (:squib)]

(In reply to comment #92)
> (In reply to comment #91)
> > Is it possible to pull the display name out from Javascript? That would let us
> > use FormatDisplayName from bug 474721, which would be more consistent.
> 
> So you mean to use FormatDisplayName() in the threadPane.js?

Yeah. Well... probably, assuming threadPane.js is the logical place for that. I've been trying to get more places to use that method since the old way had problems, and consistency in the UI is a good thing.

Comment 96

[David :Bienvenu]

Comment on attachment 484634 [details] [diff] [review]
new patch for thread pane

Thx for the patch, sorry for the delay in reviewing it.

You've got a few whitespace errors and long lines - see http://beaufour.dk/jst-review/

typo - should be "display"
+      // if the e-mail address is in the address book, use the dispaly name as the result

no space between nsCOMPtr and <
+      nsCOMPtr <nsIMimeConverter> mimeConverter = do_GetService(NS_MIME_CONVERTER_CONTRACTID);

should be "computed"
+  // if recipients have already been compulated, use it

should be "compute"
+      // compuate its display name 

should be "display"
+          // if the e-mail address is in the address book, use the dispaly name as the result

should be "showCondensedAddresses" - lower case 's'
+    let ShowCondenseAddresses = pref.getBoolPref("mail.showCondensedAddresses");


The bigger problem is that changing the display name of a card doesn't ever change what we display in the thread pane, because of the caching. Even repairing a folder doesn't cause the cached display name to get rebuilt because we copy the old cached value from the existing db.  This could be fixed by changing the code here:

http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#1245

to clear the cached values, if they exist. As it is now, the user would have to delete the .msf files to get the cached value to update. I think that's going to be frustrating for users.

In theory, the item property changed code should cause us to update the headers in the current view if the folder is open when the display name is changed, but that didn't work for me. And it doesn't do anything about all the other folders in the profile. I'm not sure what the best way to solve this, short of using gloda.  Perhaps standard8 has some thoughts. I believe that searches for e-mail addresses for which there is no card will cause a full scan of all address books, so that's the case we really want to avoid doing over and over again.

Comment 97

[Boying Lu]

(In reply to comment #96)
> Comment on attachment 484634 [details] [diff] [review]
> new patch for thread pane
> 
> Thx for the patch, sorry for the delay in reviewing it.
> 
> You've got a few whitespace errors and long lines - see
> http://beaufour.dk/jst-review/
> 
> typo - should be "display"
> +      // if the e-mail address is in the address book, use the dispaly name as
> the result
> 
> no space between nsCOMPtr and <
> +      nsCOMPtr <nsIMimeConverter> mimeConverter =
> do_GetService(NS_MIME_CONVERTER_CONTRACTID);
> 
> should be "computed"
> +  // if recipients have already been compulated, use it
> 
> should be "compute"
> +      // compuate its display name 
> 
> should be "display"
> +          // if the e-mail address is in the address book, use the dispaly
> name as the result
> 
> should be "showCondensedAddresses" - lower case 's'
> +    let ShowCondenseAddresses =
> pref.getBoolPref("mail.showCondensedAddresses");
> 
I'll correct these in new patch. Thanks David.
> 
> The bigger problem is that changing the display name of a card doesn't ever
> change what we display in the thread pane, because of the caching. Even

I didn't see this issue in my environment. Did you check the 'Always prefer display name over message header' in the contract card window and check the 'Show only display name for people in my address book' (in the Preference->Advanced->Reading & Display pane)?

Can you give me a test case?

> repairing a folder doesn't cause the cached display name to get rebuilt because
> we copy the old cached value from the existing db.  This could be fixed by
> changing the code here:
> 
> http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsParseMailbox.cpp#1245
> 
> to clear the cached values, if they exist. As it is now, the user would have to
> delete the .msf files to get the cached value to update. I think that's going
> to be frustrating for users.
> 
> In theory, the item property changed code should cause us to update the headers
> in the current view if the folder is open when the display name is changed, but
> that didn't work for me. And it doesn't do anything about all the other folders
> in the profile. I'm not sure what the best way to solve this, short of using
> gloda.  Perhaps standard8 has some thoughts. I believe that searches for e-mail
> addresses for which there is no card will cause a full scan of all address
> books, so that's the case we really want to avoid doing over and over again.

Comment 98

[David :Bienvenu]

(In reply to comment #97)
> I didn't see this issue in my environment. Did you check the 'Always prefer
> display name over message header' in the contract card window and check the
> 'Show only display name for people in my address book' (in the
> Preference->Advanced->Reading & Display pane)?

Yes, both of those were checked.

> 
> Can you give me a test case?

All I did was change the display name in a card, while a message from that sender was open in the message list. I didn't debug it, however.

Comment 99

[Boying Lu]

> > Can you give me a test case?
> 
> All I did was change the display name in a card, while a message from that
> sender was open in the message list. I didn't debug it, however.

Does the name changed after you click the 'OK' button in the 'Edit Card' window?

Comment 100

[Boying Lu]

Attachment: Refresh all mail folders when address book is changed.

Comment 101

[Mark Banner (:standard8)]

Comment on attachment 495455 [details] [diff] [review]
Refresh all mail folders when address book is changed.

A couple of fly-by comments.

>+// addressbook manager
>+var gAbManager = null;
>+
>+var gAccountManager = null;
>+

Try and avoid globals whenever possible - extension tend to pick them up and rely on them making them harder to remove later on.

In this case:

>+  gAbManager = Components.classes["@mozilla.org/abmanager;1"]
>+                         .getService(Components.interfaces.nsIAbManager);
>+    
>+  gAbManager.addAddressBookListener(myAddressBookListener,
>+                                    Components.interfaces.nsIAbListener.all);
>+  pref.addObserver("mail.showCondensedAddresses", myAddressBookListener,false);

gAbManager is used only to add and remove, so the benefit of the global variable is lost.


>+  gAccountManager = Components.classes["@mozilla.org/messenger/account-manager;1"]
>+                              .getService(Components.interfaces.nsIMsgAccountManager);

You don't actually use gAccountManager, you use accountManager in myAddressBookListener instead, so either this patch doesn't work properly or it gets accountManager from elsewhere.

I suspect you could quite easily make gAccountManager into a member variable in myAddressBookListener, hence removing the global nature.

I'd also suggest a better name for myAddressBookListener, something like threadPaneAbListener?

Comment 102

[Boying Lu]

Comment on attachment 495455 [details] [diff] [review]
Refresh all mail folders when address book is changed.

Thanke Mark, I'll polish my patch.

Comment 103

[David :Bienvenu]

Comment on attachment 495455 [details] [diff] [review]
Refresh all mail folders when address book is changed.

This will touch every header in every database when a display name is changed and block the ui potentially for minutes.

Comment 104

[Boying Lu]

Yes, I notice this delay and working on this issue.

Comment 105

[Boying Lu]

Attachment: a new patch

I tested the new patch in my environment and the UI delay was decreased.

Comment 106

[David :Bienvenu]

I don't like any solution that involves iterating over every header in every database because a display name changes. It does not scale to large profiles *at all*. I think we need to back up a bit and think about other approaches. One possibility is to simply add a hash table to the nsMsgDBView code to do lookups, so we don't persist the info in the .msf files and don't have to touch every .msf file whenever a display name changes. Another possibility is to have some easy, fixed cost way of knowing when to throw away the cached info in the .msf files. For example, you could a global display name version # (perhaps stored as a pref). Whenever a display name is changed (not a common thing), we bump the version #. Whenever code caches a display name in a db, it would store the current global display name version number in the db. When a view is opened on a db, it would compare the current global display name version # against the # stored in the db, and if different, throw away the cached info. That might be tricky with cross-folder views, because you'd need to do that for all db's that make up the view. Or, you could store the global version # with the display name in the msghdr (e.g., if the display name is "Fred" and the global version # is 23, the value would be 23|Fred, and when you pull out the cached display name, you'd compare the global version numbers, and throw away the cached display name if the version numbers don't match, and cache a new version with the potentially new display name and newer global version number.

Comment 107

[David :Bienvenu]

Comment on attachment 496436 [details] [diff] [review]
a new patch

minusing based on prev comment.

Comment 108

[Boying Lu]

Thanks a lot for your comments, I'll think about it.

Comment 109

[Boying Lu]

Attachment: add version # to the header

Comment 110

[David :Bienvenu]

Comment on attachment 497742 [details] [diff] [review]
add version # to the header

thx for the new patch; sorry for the delay in reviewing it. I tried it out and it seems to work, so that's great.

With the new approach, I don't think you need the threadAbPaneListener since repaint will do the right thing and we might as well not have the overhead of additional pref observers.

+  int currentDisplayNameVersion = 0;

please use PRInt32.

+  currentDisplayNameVersion++;
+
+  prefs->SetIntPref("mail.addressbook.update",currentDisplayNameVersion);
+

just do prefs->SetIntPref("mail.addressbook.update", ++currentDisplayNameVersion);

again, PRInt32:

+  PRBool showCondensedEmailAddresses = PR_FALSE;
+  int currentDisplayNameVersion = 0;


+    cachedDisplayNameVersion = nsCRT::strtok(unparsedAuthor.BeginWriting(),"|",&cachedDisplayName);
+    if (atoi(cachedDisplayNameVersion ) == currentDisplayNameVersion )

I'd prefer ParseString here, or perhaps just unparsedAuthor.ToInteger(), which might just do the right thing because '|' is not a valid numeric character.

Not sure we should use nsPrintfCString with frozen linkage:

+  // update the sender_name with "current_display_name_vesion|aSenderString"
+  nsPrintfCString cachedSenderName("%d|",currentDisplayNameVersion);

You can use AppendInt instead...

Don't need braces here:

+          if (!curName.IsEmpty())
+          {
+            CopyUTF8toUTF16(curName, recipient);
+          }else
+            CopyUTF8toUTF16(curAddress, recipient);

else goes on its own line here:

+    PR_FREEIF(emailAddresses);
+  }else
+    aRecipientsString = unparsedRecipients;


+          rv = abManager->GetDirectories(getter_AddRefs(enumerator));
+          NS_ENSURE_SUCCESS(rv, PR_FALSE);
+
this method returns an nsresult, so that seems wrong. And this method should always fall back to filling in the best possible string, not returning an error.

Comment 111

[Jim Porter (:squib)]

I mentioned this above, and hopefully it didn't get lost, but it would be nice if we could use the Javascript function FormatDisplayName to get the display name, since that was created with extensions in mind. This might be more difficult than it's worth though, since I have no idea how to call Javascript from C++. (It may also make caching the names harder, but that could be fixed with some tweaks to FormatDisplayName.)

Comment 112

[Boying Lu]

Thanks a lot. I'll make a new patch.

Comment 113

[Boying Lu]

Attachment: the updated patch

Thanks David, I made a new patch based on your comments

Comment 114

[David :Bienvenu]

Comment on attachment 502429 [details] [diff] [review]
the updated patch

sorry for the delay, this is looking pretty good - one last thing - this code is duplicated, would it be possible to make a helper routine to get rid of code duplication?

+        {
+          // if the e-mail address is in the address book,
+          // use the display name as the result
+
+          rv = abManager->GetDirectories(getter_AddRefs(enumerator));
+          NS_ENSURE_SUCCESS(rv, PR_FALSE);
+
+          nsCOMPtr<nsISupports> supports;
+          nsCOMPtr<nsIAbDirectory> directory;
+          nsCOMPtr<nsIAbCard> cardForAddress;
+          PRBool hasMore = PR_FALSE;
+          PRBool preferDisplayName = PR_TRUE;
+  
+          while (NS_SUCCEEDED(enumerator->HasMoreElements(&hasMore)) &&
+                              hasMore && !cardForAddress)
+          {
+            rv = enumerator->GetNext(getter_AddRefs(supports));
+            NS_ENSURE_SUCCESS(rv, rv);
+            directory = do_QueryInterface(supports);
+            if (directory)
+            {
+              rv = directory->CardForEmailAddress(curAddress,
+                                 getter_AddRefs(cardForAddress));
+
+              if (NS_SUCCEEDED(rv) && cardForAddress)
+                // find the card of the email address,
+                // so jump out of the while loop
+                break;
+            }
+          }
+
+          if (cardForAddress)
+          {
+            cardForAddress->GetPropertyAsBool("PreferDisplayName",&preferDisplayName);
+
+            if (preferDisplayName)
+              cardForAddress->GetDisplayName(recipient);
+          }
+        }

Unfortunately, we can't do an xpcshell test for this because GetCellText isn't scriptable, but I'd like to figure out a way to expose that method, so we can add tests for things like this.

Comment 115

[David :Bienvenu]

Comment on attachment 502429 [details] [diff] [review]
the updated patch

minusing, but I think if you can share the duplicated code, then this would be ready to land.

Comment 116

[Boying Lu]

Attachment: remove duplicated codes

Two changes in the patch:
1. Remove duplicated codes into three helper routines
2. Save mail.showCondensedEmailAddresses also, so UI will also be updated if this preference is changed.

Comment 117

[David :Bienvenu]

(In reply to comment #116)

Thx for the new patch.

> 2. Save mail.showCondensedEmailAddresses also, so UI will also be updated if
> this preference is changed.

But, won't we display the right thing on repaint? If so, storing this persistently in the database seems unneeded and more complicated than we need.

Comment 118

[David :Bienvenu]

a few whitespace nits:

prefs->GetIntPref("mail.addressbook.update",&currentDisplayNameVersion);

We want a space after the ',' in lines like this.

Some of your lines go well over 80 chars; those should be wrapped, e.g.,

static nsresult GetCachedName....

There are also some blank lines with spaces, though I can clean those up before I land a final patch.

Comment 119

[David :Bienvenu]

Comment on attachment 507793 [details] [diff] [review]
remove duplicated codes

minusing based on prev questions/issues - the main one is why cache showCondensedEmailAddresses?

Comment 120

[Boying Lu]

(In reply to comment #118)
> a few whitespace nits:
> 
> prefs->GetIntPref("mail.addressbook.update",&currentDisplayNameVersion);
> 
> We want a space after the ',' in lines like this.
> 
> Some of your lines go well over 80 chars; those should be wrapped, e.g.,
> 
> static nsresult GetCachedName....
> 
> There are also some blank lines with spaces, though I can clean those up before
> I land a final patch.

David, thanks a lot.  I'll make a new patch tomorrow.

Comment 121

[Boying Lu]

Attachment: new patch

Following changes in this patch:
1.Wrap long lines
2.Remove spaces in blank lines.
3.Don't save mail.showCondensedEmailAddresses
4.Rename mail.addressbook.update to more meaningful name mail.displayname.version

Comment 122

[David :Bienvenu]

Attachment: a few tweaks to prev patch [Checked in: Comment 123]

I took the liberty of making a few tweaks to your prev patch - the only code change was to make GetCachedName take the current display version as an argument, and do the version check itself, instead of duplicating the version check. And I made it a void method - if an empty cached name is returned, the calling code knows it has to figure out the display name.  And I fixed some whitespace issues and a compiler warning.

I'll land this unless you see a problem with the changes I made. Thx for all your work on this!

Comment 123

[David :Bienvenu]

fixed on trunk. http://hg.mozilla.org/comm-central/rev/498f1e289147

Comment 125

[[e-mail]]

Comment on attachment 511764 [details] [diff] [review]
a few tweaks to prev patch
[Checked in: Comment 123]

>+static void GetCachedName(const nsCString& unparsedString,
>+                          PRInt32 displayVersion, nsACString& cachedName)
>+{
>+  PRInt32 err;
>+
>+  //get verion #
>+  PRInt32 cachedVersion = unparsedString.ToInteger(&err,10);
>+  if (cachedVersion != displayVersion)
>+    return;
>+  //get cached name
>+  nsACString::const_iterator begin,middle,end;
>+  unparsedString.BeginReading(begin);
>+  unparsedString.EndReading(end);
>+  middle = end;
>+
>+  if (FindInReadable(NS_LITERAL_CSTRING("|"),begin,middle))
>+    cachedName = Substring(middle,end);
>+}
This function unfortunately does not compile under the external API.

Comment 126

[[e-mail]]

Attachment: Untested external API fix

Comment 127

[David :Bienvenu]

Comment on attachment 514007 [details] [diff] [review]
Untested external API fix

thx Neil, sorry about that...

Comment 128

[[e-mail]]

Comment on attachment 514007 [details] [diff] [review]
Untested external API fix

Pushed changeset f5a543d60e6d to comm-central.

Comment 131

[Kalenz]

I've tried reading through all of the above but can't follow everything, so apologies if this is redundant. I'm working with Thunderbird 3.1.12 and the "show only display name" option enabled.

(1) Incoming messages from "THE_QUAKE_LORD <[e-mail]>" still display as "THE_QUAKE_LORD" and not as "Billy Geek" (from address book) which is mildly annoying, but I understand the views that have been argued for and against changing this. Can I propose an interface, "music tagging style", where a user can hard-configure this manually without going into source code or patches? E.g. "%a <%e>" for "addressbookname <emailaddress>" or "%a (%s)" for "addressbookname (sendername)", etc., with different configurations possible for known/unknown senders?

(2) When replying to a message from Billy Geek, the Compose window still opens with "THE_QUAKE_LORD <[e-mail]>" in the "To:" box, and unless this is changed manually, the message appears in the same way in sent mail. The obvious preference here would be "Billy Geek <[e-mail]>" i.e. pulling the display name from the address book automatically.

(3) Indirectly related to this, but to save me re-explaining the context in a separate bug: when clicking the "star" button to add a contact to the Address Book, I would strongly prefer the contact to be opened for editing, rather than stored directly with the sender's display name. There should at least be an option in Preferences to enable this.

Comment 132

[Jim Porter (:squib)]

(In reply to Haro de Grauw from comment #131)
> I've tried reading through all of the above but can't follow everything, so
> apologies if this is redundant. I'm working with Thunderbird 3.1.12 and the
> "show only display name" option enabled.

The feature in this bug only applies to 5.0+, so most of your problems would likely be fixed if you upgraded.