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

RESOLVED FIXED in Thunderbird 3.3a3

Status

enhancement
RESOLVED FIXED
15 years ago
8 years ago

People

(Reporter: whimboo, Assigned: eagle.lu)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
Thunderbird 3.3a3
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [msg db view has perf issues][msg hdr display pushed][penelope_wants])

Attachments

(3 attachments, 22 obsolete attachments)

1.45 KB, patch
Details | Diff | Splinter Review
12.90 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
1.22 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
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?
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?
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.
(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...
I do not use the collected address book or have it set to save addresses.
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
Some.Name@domain.com CC'd, and I have an entry for some.name@domain.com 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" <some.name@domain.com>
And then the name used is "Some 1337 h4x0r Name" instead of the one from the
address book.
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.
The symptom at comment 2 is bug 279290 -- which was WontFix'd for unexplained 
reasons.
QA Contact: front-end
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 <hillary@clinton.name>" 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.]
Depends on: 240138
No longer depends on: 381555
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 <billgates@microsoft.com>
  Scott McNealy <scottmcnealy@sun.com>

If Mr. Gates sends me an email and wants me to think that it's from Mr. McNealy, he might use "Scott McNealy <billgates@microsoft.com>".

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 <billgates@microsoft.com>", 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).
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.
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.
We should also keep in mind bug 381555 which let us search in each address book. Perhaps that should be fixed before this issue.
Assignee: mscott → brian.lu
Depends on: 381555
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 

(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 BoYiNgLu@test.com, that should find BOYINGLU@TEST.COM or boyinglu@test.com in the address book.
Attachment #282532 - Attachment is obsolete: true
Attachment #282532 - Flags: review?(mscott)
Attachment #282956 - Flags: review?(mscott)
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. foo@bar.de). 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.

Status: NEW → ASSIGNED
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. foo@bar.de). 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.

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
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
(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 <brian.lu@sun.com> should 
be shown as "Brian Lu" instead of "Brian Lu <brian.lu@sun.com>"

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 eagle.lu@gmail.com (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 eagle.lu@gmail.com are shown as "Hawk Lu" instead of "Eagle Lu" 
or "Eagle Lu <eagle.lu@gmail.com>"

This case shows that the patch also fix the bug 381555

Hope this can be helpful to you.  Thanks
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 ? 
Attachment #282956 - Flags: review?(mscott) → review?(neil)
Attachment #282956 - Flags: review?(neil) → review?(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?
David, could you please have a look at comment 17 where pointed out two other things in relation to the latest patch?
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.
Attachment #282956 - Attachment is obsolete: true
Attachment #282956 - Flags: review?(bienvenu)
Posted patch patch v1 (obsolete) — Splinter Review
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
Attachment #289806 - Flags: review?(bienvenu)
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 <foobar@foo.bar>' and others like '"Foo Bar" <foobar@foo.bar>'. 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 <test123@example.com>" 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.
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.
(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
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 <foobar@foo.bar>' and others like '"Foo Bar"
> <foobar@foo.bar>'. 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 <test123@example.com>" 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.
(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.
(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 <foobar@foo.bar>' and others like '"Foo Bar"
> <foobar@foo.bar>'. 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 <test123@example.com>" 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
(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 <foobar@foo.bar>' and others like '"Foo Bar"
> > <foobar@foo.bar>'. 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" <foo@bar.de>'. 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 <test123@example.com>" 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 on attachment 289806 [details] [diff] [review]
patch v1

I made a new patch. After some testing, I'll post it here
Attachment #289806 - Attachment is obsolete: true
Attachment #289806 - Flags: review?(bienvenu)
Comment on attachment 289806 [details] [diff] [review]
patch v1

I made a new patch. After some testing, I'll post it here
Posted patch new patch (obsolete) — Splinter Review
I've addresses all issues in Comment #27
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.
(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
Attachment #292379 - Attachment is obsolete: true
Posted patch patch (obsolete) — Splinter Review
First thanks very much for Mark's comment, I re-made the patch based on his comment
(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.
(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 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.
(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.
Depends on: 407564
Version: unspecified → Trunk
(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 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.
Attachment #292580 - Attachment is obsolete: true
Posted patch patch again (obsolete) — Splinter Review
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
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.
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.
(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
Attachment #293640 - Attachment is obsolete: true
Posted patch patch v2 (obsolete) — Splinter Review
The patch fixes issues in comment #47 (except the last one).
> * 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.
(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?
(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?
(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.
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.
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.
(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?

(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.
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 <person@not.in.address.book>
** To: Person Not In AddressBook <person@not.in.address.book>
** CC: Person In AddressBook, Person Not In AddressBook <person@not.in.address.book>

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.
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.
Flags: blocking-thunderbird3?
Comment on attachment 295066 [details] [diff] [review]
patch v2

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

At least this part of comment 37 wasn't addressed...
Blocks: 381555
No longer depends on: 381555
(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.
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.
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.
Boying are you willed to update your patch? Or are you time limited at the moment?
(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
Attachment #295066 - Attachment is obsolete: true
Attachment #333196 - Flags: review?(bugzilla)
Duplicate of this bug: 450076
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.
Attachment #333196 - Flags: review?(bugzilla) → review-
Mark, thanks a lot for review. I'm making a new patch and will post it here when ready.
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.
Attachment #333196 - Attachment is obsolete: true
I'll give out a better solution
(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?
Attachment #333893 - Attachment is obsolete: true
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 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.
Attachment #333915 - Attachment is obsolete: true
Posted patch new patch (obsolete) — Splinter Review
Attachment #334239 - Flags: review?(bugzilla)
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).
Attachment #334239 - Flags: review?(bugzilla) → review+
(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.
Attachment #334239 - Attachment is obsolete: true
Mark, if possible, can you help me to check in the patch? I don't have permission to do this.
Comment on attachment 334253 [details] [diff] [review]
[checked in] patch for message header display

Thanks Boying, patch checked in. Changeset id: 127:a7881593c05a.
Attachment #334253 - Attachment description: patch → [checked in] patch for message header display
Whiteboard: [msg db view has perf issues][msg hdr display pushed]
Attachment #333895 - Attachment is obsolete: true
Attachment #336299 - Flags: review?(bugzilla)
Attachment #336299 - Attachment is obsolete: true
Attachment #336299 - Flags: review?(bugzilla)
Attachment #336302 - Flags: review?(bugzilla)
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.
Attachment #336302 - Flags: review?(bugzilla) → review-
Mark, it sounds like this bug should be closed at this point?
Flags: blocking-thunderbird3? → blocking-thunderbird3-
(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.
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 ?
Blocks: 165249
Duplicate of this bug: 487129
Whiteboard: [msg db view has perf issues][msg hdr display pushed] → [msg db view has perf issues][msg hdr display pushed][penelope_wants]
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++.)
Posted patch new patch for thread pane (obsolete) — Splinter Review
Attachment #484634 - Flags: review?
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.
Attachment #484634 - Flags: review? → review?(bugzilla)
(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 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.
Attachment #484634 - Flags: review?(bugzilla) → review?(bienvenu)
> 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.
(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 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.
Attachment #484634 - Flags: review?(bienvenu) → review-
(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.
(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.
> > 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?
Attachment #294542 - Attachment is obsolete: true
Attachment #336302 - Attachment is obsolete: true
Attachment #484634 - Attachment is obsolete: true
Attachment #495455 - Flags: review?(bienvenu)
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 on attachment 495455 [details] [diff] [review]
Refresh all mail folders when address book is changed.

Thanke Mark, I'll polish my patch.
Attachment #495455 - Flags: review?(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.
Attachment #495455 - Flags: feedback-
Yes, I notice this delay and working on this issue.
Posted patch a new patch (obsolete) — Splinter Review
I tested the new patch in my environment and the UI delay was decreased.
Attachment #495455 - Attachment is obsolete: true
Attachment #496436 - Flags: review?(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 on attachment 496436 [details] [diff] [review]
a new patch

minusing based on prev comment.
Attachment #496436 - Flags: review?(bienvenu) → review-
Thanks a lot for your comments, I'll think about it.
Posted patch add version # to the header (obsolete) — Splinter Review
Attachment #496436 - Attachment is obsolete: true
Attachment #497742 - Flags: review?(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.
Attachment #497742 - Flags: review?(bienvenu) → review-
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.)
Thanks a lot. I'll make a new patch.
Posted patch the updated patch (obsolete) — Splinter Review
Thanks David, I made a new patch based on your comments
Attachment #497742 - Attachment is obsolete: true
Attachment #502429 - Flags: review?(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 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.
Attachment #502429 - Flags: review?(bienvenu) → review-
Posted patch remove duplicated codes (obsolete) — Splinter Review
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.
Attachment #507793 - Flags: review?(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.
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 on attachment 507793 [details] [diff] [review]
remove duplicated codes

minusing based on prev questions/issues - the main one is why cache showCondensedEmailAddresses?
Attachment #507793 - Flags: review?(bienvenu) → review-
(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.
Posted patch new patch (obsolete) — Splinter Review
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
Attachment #502429 - Attachment is obsolete: true
Attachment #507793 - Attachment is obsolete: true
Attachment #511646 - Flags: review?(bienvenu)
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!
Attachment #511646 - Attachment is obsolete: true
Attachment #511764 - Flags: review+
Attachment #511646 - Flags: review?(bienvenu)
fixed on trunk. http://hg.mozilla.org/comm-central/rev/498f1e289147
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a3
Attachment #511764 - Attachment description: a few tweaks to prev patch → a few tweaks to prev patch [Checked in: Comment 123]
Duplicate of this bug: 312821
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.
Attachment #514007 - Flags: review?(bienvenu)
Comment on attachment 514007 [details] [diff] [review]
Untested external API fix

thx Neil, sorry about that...
Attachment #514007 - Flags: review?(bienvenu) → review+
Comment on attachment 514007 [details] [diff] [review]
Untested external API fix

Pushed changeset f5a543d60e6d to comm-central.
Blocks: 460737
No longer blocks: 460737
Duplicate of this bug: 650693
Depends on: 652318
Duplicate of this bug: 572304
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 <billy.geek@domain.com>" 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 <billy.geek@domain.com>" 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 <billy.geek@domain.com>" 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.
(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.
You need to log in before you can comment on or make changes to this bug.