309057 - Port |Bug 240138 – Show just the name and not the email address in the message pane| to SeaMonkey; then bug 381555 and bug 243631

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Ian Neal]

If the email address is in one of your address books, we should just show the
user's name instead of the full name + email address in the message pane.

Outlook and mail.app both do this and it's a great way to reduce space in the
message pane for known addresses. And it lets folks know if the message is from
someone in one of their ABs.

This is the equivalent of TB bug 240138.

Cannot work on this until patch for bug 290881 has landed though.

Comment 1

[Manuel Reimer]

NO! I vote for WONTFIX or at least for disabling this by default. Please read
the comments on bug 240138 carefully.

Comment 2

[Karsten Düsterloh]

While it may be nice to have the option to manipulate, this for sure has no
priority. And the default should be "show both".

Comment 3

[Ian Neal]

Just because it will the equivalent of does not mean it will be identical to bug
240138. I will be taking on board the comments from that bug when I start work
on this one which means it might end up looking completely different in
implementation.

Comment 4

[Serge Gautherie (:sgautherie)]

Bug 240138 was fixed a few days ago.

Do we want to port what was done there, or do something different ?

Comment 5

[Ian Neal]

A starting point is to begin with porting but then adapting it to best suit SMs needs.

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Display part

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071402 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Bug 240138, ported with a few rewrites.

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Preference UI

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071402 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Bug 240138, ported to current code,
plus an accesskey.

Trouble is that this makes the panel too high to fit in the window :-<
I need a "ui-review" (= hint/help) on this.

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Add shared default prefence value

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1-TB) Remove forked default prefence value

Comment 10

[Karsten Düsterloh]

Comment on attachment 329578 [details] [diff] [review]
(Cv1) Add shared default prefence value

>+// Show the friendly display names for people I know.
>+pref("mail.showCondensedAddresses", true);

Nope. This is not going to be the default.

Comment 11

[jag (Peter Annema)]

(In reply to comment #7)
> Trouble is that this makes the panel too high to fit in the window :-<
> I need a "ui-review" (= hint/help) on this.

Heh, it fits in mine. I must've resized my pref window at some point. I don't see an easy solution, other than "make the pref window bigger by default." Maybe Neil can help?

And personally I much prefer seeing both the name and the e-mail address. It helps me figure out much more quickly what's spam and what's not.

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Cv2) Add shared default prefence value

Cv1, with comment 10 suggestion(s).

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: Image with Bv1 (too high) layout

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071402 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

I would need (for example) to remove the |<separator class="thin"/>| before and after to get the panel to fit again...

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> (In reply to comment #7)
> > Trouble is that this makes the panel too high to fit in the window :-<
> > I need a "ui-review" (= hint/help) on this.
> 
> Heh, it fits in mine. I must've resized my pref window at some point. I don't

Mine is at its default size (confirmed with a dumb profile).

> see an easy solution, other than "make the pref window bigger by default."
> Maybe Neil can help?

Comment 15

[[e-mail]]

Ah, I see bug 297534 used up some of our valuable space!
I guess we can't afford to put both of its radiobuttons on one line.
We could remove the thin separators from that groupbox, that looks as if it should give us enough room.

Comment 16

[Serge Gautherie (:sgautherie)]

(In reply to comment #15)
> I guess we can't afford to put both of its radiobuttons on one line.

The radiobuttons which seem they could fit on their previous line are the "Fixed + Variable" ones.

> We could remove the thin separators from that groupbox, that looks as if it
> should give us enough room.

As noted in comment 13, removal of 2 separators would be needed.

Comment 17

[Serge Gautherie (:sgautherie)]

Or we could create a new panel and see how to reorganize the preferences we have...

Comment 18

[[e-mail]]

(In reply to comment #16)
> (In reply to comment #15)
> > I guess we can't afford to put both of its radiobuttons on one line.
> The radiobuttons which seem they could fit on their previous line are the
> "Fixed + Variable" ones.
That's not the same thing. Those two buttons are already on the same line.

> > We could remove the thin separators from that groupbox, that looks as if it
> > should give us enough room.
> As noted in comment 13, removal of 2 separators would be needed.
Sure, but there happen to be two inside that groupbox.

Comment 19

[Ian Neal]

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > I guess we can't afford to put both of its radiobuttons on one line.
> > The radiobuttons which seem they could fit on their previous line are the
> > "Fixed + Variable" ones.
> That's not the same thing. Those two buttons are already on the same line.

I think he means moving them up to the same line as "Use the following font width:", which is possible though might have to shorten the label and/or change the radio buttons into a menulist (similar to HTML message composition).

Comment 20

[Serge Gautherie (:sgautherie)]

(In reply to comment #18)
> (In reply to comment #16)
> > As noted in comment 13, removal of 2 separators would be needed.
> Sure, but there happen to be two inside that groupbox.

Yes, that's what I tested (successfully)...
While there, should I removed the 2 separators from the other <groupbox> too ?

(In reply to comment #19)
> I think he means moving them up to the same line as "Use the following font
> width:", which is possible though might have to shorten the label and/or change
> the radio buttons into a menulist (similar to HTML message composition).

Yes, as in the unmigrated "M & N > Composition > General > Forward messages".

Just tell me which solution you prefer.

Comment 21

[Serge Gautherie (:sgautherie)]

Neil,

IanN: (IIRC)
I'm happier with either changing the label to "Font width:" followed by the two radio buttons on the same line or changing the label to "Font:" followed by a menulist with items "Fixed Width" and "Variable Width"

Me:
Reducing the text to simply "Font width:" and trivially moving the 2 radios on that line too would please both of us.
(Being identical as the other pane is fine too, thought not "needed" as we wouldn't have the font list in the dropdown.)

Final choice between the various solutions is yours...

Comment 22

[[e-mail]]

(In reply to comment #21)
I'm happy with IanN's second suggestion, as this then resembles the font menulist in the composition pane.

Comment 23

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a) Preference UI

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071502 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Bv1, with comment 22 suggestion(s).

NB: Although TB (currently) has it with the radios on the same line.

Comment 24

[[e-mail]]

Comment on attachment 329775 [details] [diff] [review]
(Bv1a) Preference UI

>+            <menuitem value="false" label="&fontVarWidth.label;"/>
>+            <menuitem value="true" label="&fontFixedWidth.label;"/>
IMHO fixed should come first.

>-<!ENTITY plainTextFont.label              "Use the following font width:">
>+<!ENTITY plainTextFont.label              "Font:">
>+<!ENTITY plainTextFont.accesskey          "F">
This string change might unfortunately need an entity change, maybe to fontPlainText.label/accesskey? (I'd ask KaiRo but he's not around right now.)

Comment 25

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1b) Preference UI

Bv1a, with comment 24 suggestion(s).

Comment 26

[Serge Gautherie (:sgautherie)]

(In reply to comment #24)
> >+            <menuitem value="false" label="&fontVarWidth.label;"/>
> >+            <menuitem value="true" label="&fontFixedWidth.label;"/>
> IMHO fixed should come first.

Should I swap the ones in </editor/ui/composer/content/editorOverlay.xul> too ?

Comment 27

[[e-mail]]

Comment on attachment 329847 [details] [diff] [review]
(Bv1b) Preference UI

No, because variable is the default there, but fixed is the default here.

Comment 28

[Karsten Düsterloh]

Comment on attachment 329656 [details] [diff] [review]
(Cv2) Add shared default prefence value

Basically okay, just two nits.

>Index: mailnews.js
>===================================================================
>+// Show the friendly display names for people I know.
>+pref("mail.showCondensedAddresses", false);
>+#else
>+// Show the friendly display names for people I know.
>+pref("mail.showCondensedAddresses", true);
> #endif

Drop the false advertisement (at least in the SeaMonkey part) - change the comment to something neutral like "Show both name and address for people in my addressbook."

You can wrap the TB pref override (patch D) into this one as well, since one is useless without the other and you have David looking at it anyway. ;-)

Comment 29

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) Display part

Av1, with a small optimization.

Comment 30

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) Add/Regroup default prefence value(s)

Cv2 and Dv1-TB, with comment 28 suggestion(s).

Comment 31

[jag (Peter Annema)]

Comment on attachment 329932 [details] [diff] [review]
(Av1a) Display part

>+function AddExtraAddressProcessing(emailAddress, addressNode)
>+{
>+  // If the "friendly name" feature is disabled,
>+  // don't care about the actual display name.
>+  const displayName = gShowCondensedEmailAddresses &&
>+                      addressNode.getAttribute("displayName");
>+
>+  if (displayName)
>+  {
>+    // always show the address for the from and reply-to fields
>+    const parentElementId = addressNode.parentNode.id;
>+    const condenseName = parentElementId != "expandedfromBox" &&
>+                         parentElementId != "expandedreply-toBox";
>+
>+    if (condenseName && useDisplayNameForAddress(emailAddress))
>+    {
>+      addressNode.setAttribute("label", displayName);
>+      addressNode.setAttribute("tooltiptext", emailAddress);
>+    }
>+  }
>+}

I think I would prefer:

function AddExtraAddressProcessing(emailAddress, addressNode)
{
  if (!gShowCondensedEmailAddresses)
    return;

  const displayName = addressNode.getAttribute("displayName");
  if (!displayName)
    return;

  if (!useDisplayNameForAddress(emailAddress))
    return;

  // don't condense the address for the from and reply-to fields
  const parentElementId = addressNode.parentNode.id;
  const condenseName = parentElementId != "expandedfromBox" &&
                       parentElementId != "expandedreply-toBox";

  if (condenseName)
  {
    addressNode.setAttribute("label", displayName);
    addressNode.setAttribute("tooltiptext", emailAddress);
  }
}

Looks good otherwise.

Comment 32

[Serge Gautherie (:sgautherie)]

(In reply to comment #31)
>   const condenseName = parentElementId != "expandedfromBox" &&
>                        parentElementId != "expandedreply-toBox";
>   if (condenseName)

And I could merge the lines and use an early return there too...

> Looks good otherwise.

Early returns were what I first thought of,
then I thought may be there was none because there might be another (different) extra processing added in the future...
Do you confirm that early returns are wanted ?

Comment 33

[Ian Neal]

Comment on attachment 329847 [details] [diff] [review]
(Bv1b) Preference UI

>Index: mailnews/base/prefs/resources/content/pref-viewing_messages.xul
>===================================================================

>+      <hbox align="center">
>+        <label value="&fontPlainText.label;" control="mailFixedWidthMessages"
>+               accesskey="&fontPlainText.accesskey;"/>
Nit: one attribute per line please with control being the last one.

>+        <menulist id="mailFixedWidthMessages"
>+                  preference="mail.fixed_width_messages">
>+          <menupopup>
>+            <menuitem value="true" label="&fontFixedWidth.label;"/>
>+            <menuitem value="false" label="&fontVarWidth.label;"/>
I would say one attribute per line, but as there is only two attributes in total I think it is okay in this case.

>Index: suite/locales/en-US/chrome/mailnews/pref/pref-viewing_messages.dtd
>===================================================================

>+<!ENTITY showCondensedAddresses.label     "Show only display name for people in my address book">
Should we mention it is only in the personal address book?

>+<!ENTITY showCondensedAddresses.accesskey "p">
Wouldn't "o" be a better accesskey.

Whilst you are touching this file, is it worth fixing the duplicate accesskey "e"?
I would suggest:
change wrapInMsg.accesskey to "W"
this frees up "t" for markAsReadDelay.accesskey

r=me with the relevant changes for this bug and either the additional changes done here or spun off into another bug.

Comment 34

[Ian Neal]

Comment on attachment 329932 [details] [diff] [review]
(Av1a) Display part

>Index: msgHdrViewOverlay.js
>===================================================================

>-  if ("AddExtraAddressProcessing" in this)
>-    AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
Are we sure that no extensions rely on this call? We could always leave this call in and just change the name of the new function that is going in.

I presume there is no way of keeping the tooltip over attachments in the attachment window?

Other than that query I am happy with jag's comments wrt early return.

>+function useDisplayNameForAddress(emailAddress)
>+{
>+  // For now, if the email address is in the personal address book, then consider this user a 'known' user
>+  // and use the display name. I could eventually see our rules enlarged to include other local ABs, replicated
>+  // LDAP directories, and maybe even domain matches (i.e. any email from someone in my company
>+  // should use the friendly display name)
Nit: lines should not be any more than 80 characters wide so please reformat the above comment lines - should still fit within the 4 lines though.

r=me with those points addressed

Comment 35

[Serge Gautherie (:sgautherie)]

(In reply to comment #34)
> (From update of attachment 329932 [details] [diff] [review])
> >-  if ("AddExtraAddressProcessing" in this)
> >-    AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
> Are we sure that no extensions rely on this call? We could always leave this
> call in and just change the name of the new function that is going in.

You may have missed
> +  AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
;->

Comment 36

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1c) Preference UI (Checkin: Comment 51)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.0.2pre) Gecko/2008071502 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Bv1b, with comment 33 suggestion(s),
plus the 2nd duplicate accesskey fix.

Comment 37

[Serge Gautherie (:sgautherie)]

(In reply to comment #33)
> Should we mention it is only in the personal address book?

(We could, but)
I think not, as there is bug 381555.

> r=me with the relevant changes for this bug and either the additional changes
> done here or spun off into another bug.

I had filed this as bug 445213 ;-)

Comment 39

[Serge Gautherie (:sgautherie)]

Attachment: (Av1b) Display part (Checkin: Comment 51)

Av1a, with comment 34 suggestion(s).

(In reply to comment #34)
> I presume there is no way of keeping the tooltip over attachments in the
> attachment window?

The tooltip is only set in another way, not removed: see bug 240138 comment 46.

Comment 40

[Serge Gautherie (:sgautherie)]

David,
(SM) code is ready for checkin;
now waiting for your sr on the default (TB) pref value patch.

Comment 41

[Ian Neal]

(In reply to comment #35)
> (In reply to comment #34)
> > (From update of attachment 329932 [details] [diff] [review] [details])
> > >-  if ("AddExtraAddressProcessing" in this)
> > >-    AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
> > Are we sure that no extensions rely on this call? We could always leave this
> > call in and just change the name of the new function that is going in.
> 
> You may have missed
> > +  AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
> ;->
> 
No I didn't. What I'm saying is that some extensions might provide their own "AddExtraAddressProcessing" and this patch now removes that hook by using that name for the new function. Now if you are sure that no extension makes use of this hook then fine, otherwise leave this hook and change the name of the function.

Comment 42

[Ian Neal]

(In reply to comment #39)
> Created an attachment (id=330239) [details]
> (Av1b) Display part
> 
> Av1a, with comment 34 suggestion(s).
> 
> (In reply to comment #34)
> > I presume there is no way of keeping the tooltip over attachments in the
> > attachment window?
> 
> The tooltip is only set in another way, not removed: see bug 240138 comment 46.
> 
Okay, I am happy with that part now.

Comment 43

[Serge Gautherie (:sgautherie)]

(In reply to comment #41)
> What I'm saying is that some extensions might provide their own
> "AddExtraAddressProcessing" and this patch now removes that hook by using that
> name for the new function. Now if you are sure that no extension makes use of
> this hook then fine, otherwise leave this hook and change the name of the
> function.

Let me (fully) dig history:

SeaMonkey:
r1.4 / 2000-01-23 18:00
added call (only) [bug 24399]
r1.29 / 2000-05-25 18:09
added if [(restricted) bug 40516: "fix aim presence for mail in the commercial tree." !]
<http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2000-05-25+18%3A07&maxdate=2000-05-25+18%3A10&cvsroot=%2Fcvsroot>
{{
2000-05-25 18:08	mscott%netscape.com 	mozilla/mailnews/base/resources/content/msgHdrViewAddresses.js 	1.5
Bug #40516 --> this file is now obsolete....instead of having these
stub methods in the mozilla tree, we'll test to see if the function is
defined (it is in the commercial tree) then call it if it is defined.
r=sspitzer

2000-05-25 18:09	mscott%netscape.com 	mozilla/mailnews/base/resources/content/msgHdrViewOverlay.js 	1.29
Bug #40516 --> fix aim presence for mail in the commercial tree.
Changes to make the toggle button an image instead of a button
r=sspitzer
}}

Thunderbird:
r1.1  / 2002-08-15 17:26
added initial (if and call) code
r1.17 / 2004-04-12 23:29
added/hijacked the function [bug 240138 !]
r1.77 / 2006-07-24 15:04
removed the (obsolete) if [bug 345650]

Indeed, only SM still has the hooks:
<http://mxr.mozilla.org/mozilla/search?string=fillEmailAddressMenu&case=on>
No results found
<http://mxr.mozilla.org/mozilla/search?string=FinishEmailProcessing&case=on>
/mailnews/base/resources/content/msgHdrViewOverlay.js (remnant hook)
<http://mxr.mozilla.org/mozilla/search?string=NotifyClearAddresses&case=on>
/mailnews/base/resources/content/msgHdrViewOverlay.js (remnant hook)
<http://mxr.mozilla.org/mozilla/search?string=AddExtraAddressProcessing&case=on>
/mail/base/content/msgHdrViewOverlay.js (TB "hijacked" function)
/mailnews/base/resources/content/mailWidgets.xml (See r1.42 : Bug 149515)
/mailnews/base/resources/content/msgHdrViewOverlay.js (See my current patch.)
/mailnews/base/resources/content/msgHdrViewAddresses.js (See Bug 445167 !)

Then:
Although I have no idea about possible current extensions using these hooks,
it seems the latters are only remnants of Netscape+Aim bundling,
which TB already got rid of.
Iiuc, we would be talking about SM-only and post-Aim extensions.

I think it should be worth/"safe" to try and remove (all !) the hooks,
then see if we get complains ... as we are still early enough in the SM v2.0 cycle.

Mnyromyr, what do you think ?

Comment 44

[Karsten Düsterloh]

I never met an extension actually using it, but that doesn't necessarily mean anything (Mnenhy just mimics mailnews' usage and calls it if present, it doesn't actually use it). Given the amount of complaints (none I know of, but again, that doesn't necessarily mean anything as I'm not following TB fora), I guess the hooks can be removed.
By my own experience these hooks are almost meaningless, because for doing useful changes to header processing, you need to overlay most of the functions doing these callbacks with your own completely... :-| 

Comment 45

[Serge Gautherie (:sgautherie)]

(In reply to comment #44)
> I guess the hooks can be removed.

I filed bug 446343, as a follow-up.

Comment 46

[Serge Gautherie (:sgautherie)]

Attachment: (Cv3-SM) Add SM default prefence value (Checkin: Comment 51)

Keeping
{{
(Ev1) Add/Regroup default prefence value(s)
mnyromyr: review+ 
}}

Comment 47

[Stefan [:stefanh]]

Cv3-SM still waits for sr, please add back keyword etc when it's ready

Comment 48

[Ian Neal]

Might be worth switching your sr request to someone else? Neil perhaps?

Comment 49

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #37)
> (In reply to comment #33)
> > Should we mention it is only in the personal address book?
> 
> (We could, but)
> I think not, as there is bug 381555.

Since that one is fixed now you might consider doing the necessary changes as part of this bug.

Comment 50

[Ian Neal]

Also worth looking at bug 243631 and https://bugzilla.mozilla.org/attachment.cgi?id=334253

Comment 51

[Ian Neal]

Comment on attachment 330239 [details] [diff] [review]
(Av1b) Display part (Checkin: Comment 51)

http://hg.mozilla.org/comm-central/rev/be8f98ac3842

Comment 52

[Ian Neal]

Leaving open for issues in comment 49 and comment 50 to be looked at / spun off.

Comment 53

[Jens Hatlak (:InvisibleSmiley)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080831003516 SeaMonkey/2.0a1pre

This doesn't work for me, does it work for anyone of you? I tried:
1. activated new pref (using UI, verified using about:config)
2. added Personal Address Book entry for my own email address
3. sent mail from myself to myself
4. triggered Get Messages for my IMAP account
5. selected the mail that I sent in step 3.

Expected results: "From:" in header pane should show only the name
Actual results: "From:" in header pane shows name and email address.

The same applies to news postings (test scenario: KaiRo's 1.1.12 candidates posting in m.d.a.seamonkey with KaiRo in the Personal Address Book) but I don't know if the mechanism is intended to work there.

FWIW: Collected Addresses is empty and there are no additional address books.

Comment 54

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #52)
> Leaving open for issues in comment 49 and comment 50 to be looked at / spun
> off.

Both affect the same file so they can be easily handled in one bug - or do you prefer to do it here?

Comment 55

[Serge Gautherie (:sgautherie)]

(In reply to comment #53)
> Expected results: "From:" in header pane should show only the name

"From:" is never condensed.

> The same applies to news postings (test scenario: KaiRo's 1.1.12 candidates

SM 1.1.x branch will never have this feature.

(In reply to comment #54)
> Both affect the same file so they can be easily handled in one bug - or do you
> prefer to do it here?

Let me handle it, unless you want to port these additional bugs yourself (which I would be happy with ;->).

Comment 56

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #55)
> (In reply to comment #53)
> > Expected results: "From:" in header pane should show only the name
> 
> "From:" is never condensed.

<Checking source code> You're right. But the only other exception is Reply-To. I cannot make it work for To either, though. Can you provide a screen shot and/or test case of how it should look like if it works?

> > The same applies to news postings (test scenario: KaiRo's 1.1.12 candidates
> 
> SM 1.1.x branch will never have this feature.

Funny guy. I know that! I was describing a test case where you put KaiRo into your Personal Address Book and view a newsgroup posting. It could have been any other one but that's the one I found first. But now that we found that From: is never condensed it's a bad test case anyway.

> (In reply to comment #54)
> > Both affect the same file so they can be easily handled in one bug - or do you
> > prefer to do it here?
> 
> Let me handle it, unless you want to port these additional bugs yourself (which
> I would be happy with ;->).

I could give it a try. That's why I asked.

Comment 57

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Port of bugs 243631 and 381555 plus parentElementId fix

This should be it. The fix for bug 381555 is a straight port; for bug 243631 I tried to keep the style of the present code.

I also found and fixed another bug: for SM we need to go up another two levels for parentElementId (found using DOMI and Venkman). Without that parentElementId is "" and matches neither From nor Reply-To.

BTW: I guess the comment that can be seen in bug 243631's attachment 334253 [details] [diff] [review] ("AddExtraAddressProcessing which is undefined in seamonkey") is now wrong.

BTW2: With my patch I now see only the display name for addresses that are both in my Personal Address Book and the To header row.

Comment 58

[[e-mail]]

Note that Serge's review won't be official and your official reviewer (usually whomever last reviewed changes to that code) may not agree with his comments.

Comment 59

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #58)
> Note that Serge's review won't be official and your official reviewer (usually
> whomever last reviewed changes to that code) may not agree with his comments.

That's OK; I just looked at the last patch (Cv3-SM) since I don't know who's the official reviewer or how to find one. The last changes to the code were through this bug. And if anything goes wrong there's still superreview and I don't have check-in rights anyway. ;-) If you think someone else should do the review I'm fine with that, too.

Comment 60

[Serge Gautherie (:sgautherie)]

(In reply to comment #59)
> That's OK; I just looked at the last patch (Cv3-SM)

You were misleaded on this one: see comment 46 ;->

> since I don't know who's the official reviewer or how to find one.

(You're welcome to join us and learn ;-))

Read pages like
http://developer.mozilla.org/En/Getting_your_patch_in_the_tree

> If you think someone else should do the review I'm fine with that, too.

I'll have a look at it, then request official review ;-)

Comment 61

[Ian Neal]

Comment on attachment 336266 [details] [diff] [review]
Port of bugs 243631 and 381555 plus parentElementId fix

> var gMessageListeners = new Array;
>@@ -1057,22 +1055,23 @@ function updateEmailAddressNode(emailAdd
>   AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
> }
> 
> function AddExtraAddressProcessing(emailAddress, addressNode)
> {
>   if (!gShowCondensedEmailAddresses)
>     return;
> 
>+  const card = useDisplayNameForAddress(emailAddress);
>+  if (!card)
>     return;
>+  const displayName = card.displayName;
You don't really need this assignment just use card.displayName in the setAttribute further down.
> 
>   // Don't condense the address for the from and reply-to fields.
>+  const parentElementId = addressNode.parentNode.parentNode.parentNode.id;
>   if (parentElementId == "expandedfromBox" ||
>       parentElementId == "expandedreply-toBox")
>     return;
> 
>   addressNode.setAttribute("label", displayName);
>   addressNode.setAttribute("tooltiptext", emailAddress);
> }
> 
>@@ -1087,30 +1086,32 @@ function fillEmailAddressPopup(emailAddr
>+  var books = Components.classes["@mozilla.org/abmanager;1"]
>+                        .getService(Components.interfaces.nsIAbManager)
>+                        .directories;
>+
>+  var card = null;
>+
>+  while (!card && books.hasMoreElements()) {
>+    var ab = books.getNext()
>+                  .QueryInterface(Components.interfaces.nsIAbDirectory);
>+    try {
>+      card = ab.cardForEmailAddress(emailAddress);
>+    }
>+    catch (ex) { }
Our bracing style is to have a new brace on a separate line unless it opens and closes on the same line, so in this case the bracket for while and for try needs to be on a new line.

Comment 62

[Jens Hatlak (:InvisibleSmiley)]

Attachment: Port v2 of bugs 243631 and 381555 plus parentElementId fix

Same as before, with Ian's nits addressed. I also took the liberty to remove the space inside the curly brackets after "catch" to match the style used in the rest of the file.

Comment 63

[Jens Hatlak (:InvisibleSmiley)]

I just read KaiRo's blog post about Alpha freeze next week. Of course the feature that is implemented through this bug isn't an Alpha blocker but if my patch won't go in until then then the feature will be only half working (since the parts that have already been checked in are unlikely to get removed). This statement is not meant to push anyone, just my 2c that I wanted to let you know before I'm AFK for the next two days.

Comment 64

[Karsten Düsterloh]

Comment on attachment 336367 [details] [diff] [review]
Port v2 of bugs 243631 and 381555 plus parentElementId fix

>+++ patch/msgHdrViewOverlay.js	2008-09-01 18:38:58.000000000 +0200
> function AddExtraAddressProcessing(emailAddress, addressNode)
> {
...
>+  const card = useDisplayNameForAddress(emailAddress);
>+  if (!card)
>     return;

Constants should have a k prefix, i.e. kCard.
OTOH, we usually don't use const in such cases, so "var card" (as you do further down) is preferred here.

>+  const parentElementId = addressNode.parentNode.parentNode.parentNode.id;

Eww, this is rather ugly. Not much we can do about it, though; let's hope no-one breaks this when touching the mail-multi-emailHeaderField XBL code. To aid him, add a comment like "get the id of the mail-multi-emailHeaderField binding parent".

Furthermore, use var instead of const here, too.

>@@ -1087,30 +1085,34 @@ function fillEmailAddressPopup(emailAddr
> function useDisplayNameForAddress(emailAddress)

<rant>Man, how I *hate* if new functions don't adhere to the current naming scheme!</rant>
(Just ignore this. ;-) )

>+  var card = null;
>+  while (!card && books.hasMoreElements())
>+  {
>+    var ab = books.getNext()
>+                  .QueryInterface(Components.interfaces.nsIAbDirectory);
>+    try
>+    {
>+      card = ab.cardForEmailAddress(emailAddress);
>+    }
>+    catch (ex) {}
>+  return card;

Using the instanceof operator gains some extra safety; furthermore, you can use an early return if you find a card, so no outer card variable is needed.

while (books.hasMoreElements())
{
  var ab = books.getNext();
  if (ab instanceof Components.interfaces.nsIAbDirectory)
  {
    try
    {
      var card = ab.cardForEmailAddress(emailAddress);
      if (card)
        return card;
    }
    catch (ex) {}
  }
}
return null;

Comment 65

[Serge Gautherie (:sgautherie)]

(In reply to comment #63)
> I wanted to let you know before I'm AFK for the next two days.

I'm working on it: I'll update your patch ;-)

Comment 66

[Serge Gautherie (:sgautherie)]

(In reply to comment #53)
> This doesn't work for me, does it work for anyone of you?

You're right !

My Av1b patch stopped working (before checkin) between
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080816002821 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
and
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080817002455 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
when
4f44120fd2a8 2008-08-16 10:54 -0400	Joshua Cranmer - Bug 449618 -- Move cardForEmailAddress to nsIAbDirectory r=Standard8, sr=dmose
landed... :-<

> The same applies to news postings [...] but I don't know if the mechanism is intended to work there.

Yes, this feature applies to newsgroups too.

(In reply to comment #56)
> Funny guy. I know that!

Sorry, I misread your comment and understood that you used a "1.1.12 candidates" build to test...

(In reply to comment #57)
> I also found and fixed another bug: for SM we need to go up another two levels
> for parentElementId (found using DOMI and Venkman).

Right ! I must have missed testing this :-<

> BTW: I guess the comment that can be seen in bug 243631's attachment 334253 [details] [diff] [review]
> ("AddExtraAddressProcessing which is undefined in seamonkey") is now wrong.

Removed by Bug 446343 ;-)

Comment 67

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1) Port bug 381555 (++)

Porting/Copying
1.104	jminta%gmail.com	2008-06-01 07:10	 	Bug 436677 Clean up rdf consumers of the addressbook, r=mkmelin
(only <msgHdrViewOverlay.js> part applies to SM)
makes this feature work again.
And this is still true with
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080901134927 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)

Yet, that (part of the) patch was "replaced" by bug 449618.
(which works fine in current build too.)

Then |useDisplayNameForAddress()| was rewritten (again) by
b6532caf76a6 2008-08-18 16:25 +0100	Mark Banner - Bug 381555 'Show only display name' works only for personal address book. r=bienvenu

***

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080901134927 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)

*Port (bug 436677 > bug 449618 >) bug 381555.
 *Additionally replace the 2 tabs in this file.
*|setFromBuddyIcon()|: optimize it a little and make it use |getCardForAddress()|.
*|updateEmailAddressNode()|: optimize the |*Attribute()| calls.
*|AddExtraAddressProcessing()|: fix |parentElementId|.
*|useDisplayNameForAddress()|:
 *Rename it |getCardForAddress()|.
 *Rewrite it some more.
 *I would think |QueryInterface()| is "enough", but as Karsten wanted |instanceof|...
*Update documentation.

*****

I prefer to port bug 243631 later/separately.

Later, I'll file a bug about sharing |getCardForAddress()| with <mailWindowOverlay.js> too... (see bug 449618)

Comment 68

[[e-mail]]

Comment on attachment 336740 [details] [diff] [review]
(Gv1) Port bug 381555 (++)

>+     if (card && card.setProperty("_AimScreenName")) {
I think this was supposed to be .getProperty (I did comment on that bug but I guess nothing happened...)

> function updateEmailAddressNode(emailAddressNode, address)
> {
>-  emailAddressNode.setAttribute("label", address.fullAddress || address.displayName);
>   emailAddressNode.setAttribute("emailAddress", address.emailAddress);
>   emailAddressNode.setAttribute("fullAddress", address.fullAddress);
>   emailAddressNode.setAttribute("displayName", address.displayName);
>-  emailAddressNode.removeAttribute("tooltiptext");
> 
>-  AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
>+  if (!AddExtraAddressProcessing(address.emailAddress, emailAddressNode))
>+  {
>+    emailAddressNode.setAttribute("label",
>+                                  address.fullAddress || address.displayName);
>+    emailAddressNode.removeAttribute("tooltiptext");
>+  }
> }
I'm not convinced by this change (and therefore the boolean returns below).

>+    if (!(ab instanceof Components.interfaces.nsIAbDirectory))
>+      continue;
> 
>+    try
>+    {
>+      var card = ab.cardForEmailAddress(emailAddress);
>+      if (card)
>+        return card;
>+    }
>+    catch (ex)
>+    {
>+      // Unsearchable address books throw |NS_ERROR_NOT_IMPLEMENTED|.
>+    }
I think I might prefer
if (ab instanceof Components.interfaces.nsIAbDirectory) {
  ...
}

Comment 69

[Serge Gautherie (:sgautherie)]

(In reply to comment #68)
> (From update of attachment 336740 [details] [diff] [review])
> >+     if (card && card.setProperty("_AimScreenName")) {
> I think this was supposed to be .getProperty (I did comment on that bug but I
> guess nothing happened...)

I very much thought so myself too, when I read the patch which changed it ! ;-<

Yet, I would rather not (look into and) change this here:
I would rather add a reminder to bug 453627, whether we end up keeping that code or not.

> >-  AddExtraAddressProcessing(address.emailAddress, emailAddressNode);
> >+  if (!AddExtraAddressProcessing(address.emailAddress, emailAddressNode))
> I'm not convinced by this change (and therefore the boolean returns below).

While testing/debugging my patch, I noticed that the full address was first displayed then replaced by the displayName.
I didn't look much further but thought this could only "optimize" this behavior.

But, as you don't like it, I'll remove it.

> I think I might prefer
> if (ab instanceof Components.interfaces.nsIAbDirectory) {

For sure, I like this even less (than the |QueryInterface()|, but I'll do it too.

***

Ian, I'll wait for your review before updating the patch.

Comment 70

[[e-mail]]

(In reply to comment #69)
> Yet, I would rather not (look into and) change this here:
> I would rather add a reminder to bug 453627, whether we end up keeping that
> code or not.
In that case could you add a reminder comment in the code too?

> While testing/debugging my patch, I noticed that the full address was first
> displayed then replaced by the displayName.
I can't see how that would happen, unless you stepped through it in Venkman; normally I would expect the display to update once at the end.

> For sure, I like this even less (than the |QueryInterface()|, but I'll do it
> too.
You mean var card = ab.QueryInterface(...)
                      .cardForEmailAddress(...); ?

Comment 71

[Serge Gautherie (:sgautherie)]

(In reply to comment #70)
> In that case could you add a reminder comment in the code too?

I filed bug 454073: I hope you agree with my plan there.

> I can't see how that would happen, unless you stepped through it in Venkman;
> normally I would expect the display to update once at the end.

I used |alert()|.
I thought that might be what triggered this behavior,
I wasn't sure but I thought the "optimization" could not hurt.

> You mean var card = ab.QueryInterface(...)
>                       .cardForEmailAddress(...); ?

"Yes", see comment 64 (like Jens "direct port" code).


NB: Ian, Neil, I agree with Jens's comment 63: I'd like to land this patch (one way or the other) before SM 2.0a1 freeze ... and deal with (all) the rest after that. (Neil, could you do the review too, if Ian is not around ?) Thanks.

Comment 72

[[e-mail]]

(In reply to comment #71)
> (In reply to comment #70)
> > In that case could you add a reminder comment in the code too?
> I filed bug 454073: I hope you agree with my plan there.
Looks good to me.

> > I can't see how that would happen, unless you stepped through it in Venkman;
> > normally I would expect the display to update once at the end.
> I used |alert()|.
Oh yes, that would also do it :-)

> I wasn't sure but I thought the "optimization" could not hurt.
It hurts code readability...

> > You mean var card = ab.QueryInterface(...)
> >                       .cardForEmailAddress(...); ?
> "Yes", see comment 64 (like Jens "direct port" code).
No, I was thinking the QueryInterface would be inside the try block.

> NB: Ian, Neil, I agree with Jens's comment 63: I'd like to land this patch (one
> way or the other) before SM 2.0a1 freeze ... and deal with (all) the rest after
> that. (Neil, could you do the review too, if Ian is not around ?) Thanks.
Given comment 64 maybe you should ask Mnyromyr if he has time to review?

Comment 73

[Ian Neal]

Comment on attachment 336740 [details] [diff] [review]
(Gv1) Port bug 381555 (++)

>@@ -994,33 +990,21 @@ function OutputEmailAddresses(headerEntr
> 
> function setFromBuddyIcon(email)
> {
>    var fromBuddyIcon = document.getElementById("fromBuddyIcon");
> 
>    try {
>      // better to cache this?
>      var myScreenName = pref.getCharPref("aim.session.screenname");
>+     if (myScreenName)
>+     {
> 
>+     var card = getCardForAddress(email);
>+     if (card && card.setProperty("_AimScreenName")) {
Please be consistent with your bracing, I know there's already a mix in this file but the preferred is to open/close braces on a new line.
r=me with that change.

Comment 74

[Serge Gautherie (:sgautherie)]

Attachment: (Gv1a) Port bug 381555 (++) [Checkin: Comment 75]

Gv1, with comment 68 and comment 73 suggestion(s).

(In reply to comment #72)
> No, I was thinking the QueryInterface would be inside the try block.

When I first wrote the patch, I even thought to get rid of the |var ab| by moving all the code inside the block,
but, because of the "silent" catch, I decided to keep it (outside).

I'll checkin this patch tomorrow, unless anyone wants some other update...

Comment 75

[Serge Gautherie (:sgautherie)]

Comment on attachment 337353 [details] [diff] [review]
(Gv1a) Port bug 381555 (++)
[Checkin: Comment 75]

http://hg.mozilla.org/comm-central/rev/b6fb6db81c5d

Comment 76

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1) Port bug 243631 attachment 334253 (++)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b1pre) Gecko/20080901134927 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)

*Port bug 243631 attachment 334253 [details] [diff] [review].
*|getCardForAddress()|: Add and use |checkFunction| parameter.
*Add |"collapsedfromValue"| exclusion,
 which everyone (including me) seems to have missed until now :->
*Check the UI before the address books, in hope this is a little faster.
 (At least, it seems more logical this way.)

Comment 77

[Mark Banner (:standard8)]

(In reply to comment #76)
> Created an attachment (id=337593) [details]
> (Hv1) Port bug 243631 attachment 334253 [details] [diff] [review] (++)

+  function hasDisplayName(card)
+  {
+    return card.displayName != "";
+  }
+
+  var card = getCardForAddress(emailAddress, hasDisplayName);

If you do this, you will come into problems later on. Assuming you're going to implement the add/edit menu options that TB now has, then if you have a matching card but with no display name you will fail to show that the card is in the address book - and you will be prompting the user to create a new card, rather than editing an existing one.

Comment 78

[[e-mail]]

Comment on attachment 337593 [details] [diff] [review]
(Hv1) Port bug 243631 attachment 334253 [details] [diff] [review] (++)

>       try
>       {
>-        var card = ab.cardForEmailAddress(emailAddress);
>-        if (card)
>-          return card;
>+        card = ab.cardForEmailAddress(emailAddress);
>       }
>       catch (ex)
>       {
>         // Unsearchable address books throw |NS_ERROR_NOT_IMPLEMENTED|.
>+        continue;
>       }
>+
>+      if (!checkFunction || checkFunction(card))
>+        return card;
You're not null-checking card any more. I'd also prefer it if you put the checkFunction call inside the try/catch.

Comment 79

[Serge Gautherie (:sgautherie)]

(In reply to comment #78)
> You're not null-checking card any more.

Ah, right :-<

> I'd also prefer it if you put the checkFunction call inside the try/catch.

Same story as with the QueryInterface() call:
I "moved" the checkFunction() call out of the try, because I didn't want the silent catch to mask a throw from it...

What I think I could do (probably for both), is to check the exception type inside the catch:
*NOT_IMPLEMENTED: remain silent.
*others: dump the exception but don't re-throw it.

Do you agree with this ?

Comment 80

[[e-mail]]

(In reply to comment #79)
> (In reply to comment #78)
> > I'd also prefer it if you put the checkFunction call inside the try/catch.
> Same story as with the QueryInterface() call:
> I "moved" the checkFunction() call out of the try, because I didn't want the
> silent catch to mask a throw from it...
OK, I guess that makes sense.

Comment 81

[Serge Gautherie (:sgautherie)]

Attachment: (Hv2) Port bug 243631 attachment 334253 (++) [Checkin: Comment 82]

Hv1, with (new) "bug 455714 part" removed.

Comment 82

[Serge Gautherie (:sgautherie)]

Comment on attachment 339075 [details] [diff] [review]
(Hv2) Port bug 243631 attachment 334253 [details] [diff] [review] (++)
[Checkin: Comment 82]

http://hg.mozilla.org/comm-central/rev/dba6f5a634ec

Comment 83

[Serge Gautherie (:sgautherie)]

(In reply to comment #67)
> Later, I'll file a bug about sharing |getCardForAddress()| with
> <mailWindowOverlay.js> too... (see bug 449618)

I filed bug 456655.

Comment 84

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1a) Regroup default prefence value(s)

Keeping
{{
(Ev1) Add/Regroup default prefence value(s)
mnyromyr: review+ 
}}

Comment 85

[Serge Gautherie (:sgautherie)]

Comment on attachment 340037 [details] [diff] [review]
(Ev1a) Regroup default prefence value(s)

I filed bug 456663 instead.