Closed Bug 126491 Opened 22 years ago Closed 15 years ago

the "Card for" issue: what do use for title for cards without generated names or emails

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: sspitzer, Assigned: standard8)

References

Details

Attachments

(1 file, 1 obsolete file)

the "Card for" issue:  what do use for title for cards without generated names 
or emails

both printing, and the card preview pane will show:

"Card for"

if we don't have a generated name or email.  (if we do, we use those first)

how should we address this?  one suggestion, after those fail, provide a way to 
march through all possible columns (in an intelligent order, like start with 
generic ones, like _AimScreenName first, for example) to find *something* to 
use as the title.

on a related note, we can have completely empty addressbook cards, we should be 
preventing that.
Product: Browser → Seamonkey
Assignee: sspitzer → mail
Changing dependency, as bug 126498 is now a dup, though I think this will be
resolved WFM/invalid when bug 64305 is fixed.
Depends on: 64305
No longer depends on: 126498
(In reply to comment #1)
> I think this will be
> resolved WFM/invalid when bug 64305 is fixed.

Mark, bug 64305 is now fixed, and new cards will be garanted to have an email;
May be WontFix this one if old "empty" cards could still exist !?
(In reply to comment #2)
> Mark, bug 64305 is now fixed, and new cards will be garanted to have an email;
> May be WontFix this one if old "empty" cards could still exist !?

Although 64305 is now fixed, we've decided to expand the number of possible required fields for the address book - see bug 314995 - as just email address is a bit limiting.

Once I've fixed 314995, I want to follow it up relatively quickly with this one to sort out the display of the card pane. Therfore moving this to core & taking.
Assignee: mail → bugzilla
Component: Address Book → MailNews: Address Book
Depends on: 314995
OS: Windows 2000 → All
Product: Mozilla Application Suite → Core
QA Contact: nbaca → addressbook
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
Assignee: bugzilla → nobody
Keywords: helpwanted
> Therfore moving this to core & taking.

Ok, I'm not looking at this at the current time.
Product: Core → MailNews Core
Target Milestone: mozilla1.9alpha1 → ---
Assignee: nobody → bugzilla
Blocks: 455246
Flags: wanted-thunderbird3+
Keywords: helpwanted
Target Milestone: --- → Thunderbird 3.0b2
Attached patch Proposed Fix (obsolete) — Splinter Review
I've never really liked the "Card for %S" string on the card view properties pane in the address book window. Now I'm working on changing "Card" to "Contact" (bug 455246), "Contact for %S" seems slightly worse; therefore I'm proposing that we just drop it.

Address Book Contacts are currently required to have at least one of: First Name, Last Name, Display Name, Company (Organization) or email address.

Therefore I'm proposing:

- if a card has one of the names, display the name according to the View -> Show Name As option.
- else, if it has a company name use that
- else use the email address as a fallback option.
Attachment #354939 - Flags: ui-review?(clarkbw)
Attachment #354939 - Flags: superreview?(neil)
Attachment #354939 - Flags: review?(mnyromyr)
Attachment #354939 - Flags: review?(Pidgeot18)
Comment on attachment 354939 [details] [diff] [review]
Proposed Fix

>+    // Note: we don't blindly just use the generatedName as that can return
>+    // the local part of the email address, whereas we want to have the option
>+    // to use the company name instead.
Would it be sensible to change the generatedName algorithm instead?

>+    if (card.getProperty("FirstName") || card.getProperty("LastName") ||
>+        card.displayName)
Nit: prefer to check the displayName first.

>-viewCardTitle=Card for %S
Two alternative suggestions:
Contact: %S
%S
(In reply to comment #6)
> (From update of attachment 354939 [details] [diff] [review])
> >+    // Note: we don't blindly just use the generatedName as that can return
> >+    // the local part of the email address, whereas we want to have the option
> >+    // to use the company name instead.
> Would it be sensible to change the generatedName algorithm instead?

I was concerned about affecting other areas, e.g. autocomplete, but I've just realised that doesn't use this function.
Comment on attachment 354939 [details] [diff] [review]
Proposed Fix

has always seemed superfluous. drop it!
Attachment #354939 - Flags: ui-review?(clarkbw) → ui-review+
Comment on attachment 354939 [details] [diff] [review]
Proposed Fix

>diff --git a/mail/components/addrbook/content/abCardViewOverlay.js b/mail/components/addrbook/content/abCardViewOverlay.js
>--- a/mail/components/addrbook/content/abCardViewOverlay.js
>+++ b/mail/components/addrbook/content/abCardViewOverlay.js
>+    if (card.getProperty("FirstName") || card.getProperty("LastName") ||
>+        card.displayName)

I'd rather see card.firstName and card.lastName here...

Asides from that, I'm good with the patch.
Attachment #354939 - Flags: review?(Pidgeot18) → review+
Attachment #354939 - Flags: review?(mnyromyr) → review+
Comment on attachment 354939 [details] [diff] [review]
Proposed Fix

Just minor nits:

>diff --git a/mailnews/addrbook/resources/content/abCardViewOverlay.js b/mailnews/addrbook/resources/content/abCardViewOverlay.js
>   if (card.isMailList)
>     cvSetNode(data.CardTitle, gAddressBookBundle.getFormattedString("viewListTitle", [generatedName]));
>-  else
>-    cvSetNode(data.CardTitle, gAddressBookBundle.getFormattedString("viewCardTitle", [titleString]));
>+  else {

If one the if branches needs braces, the other one should have as well.

>+    var titleString;
>+    // If the card has a first, last or display name use the generatedName.
>+    // Note: we don't blindly just use the generatedName as that can return
>+    // the local part of the email address, whereas we want to have the option
>+    // to use the company name instead.
...
>+    // last resourt, use the email address.

Sentences ending with punctuation should start with uppercase letters.


r=me with that.
Comment on attachment 354939 [details] [diff] [review]
Proposed Fix

>+    if (card.getProperty("FirstName") || card.getProperty("LastName") ||
>+        card.displayName)

Oh, and either don't wrap or wrap after each ||.
Sorry for not getting back to this earlier.

(In reply to comment #6)
> (From update of attachment 354939 [details] [diff] [review])
> >+    // Note: we don't blindly just use the generatedName as that can return
> >+    // the local part of the email address, whereas we want to have the option
> >+    // to use the company name instead.
> Would it be sensible to change the generatedName algorithm instead?

That's a good point. I have updated the patch so that generatedName falls back to the Company (aka Organization) name if none of the first/last/display names are filled in (regardless of the setting to display "first last", "last, first" or "display name").

This affects:

- The list of cards in the ab results pane.
- Card preview pane (in the same way as the original patch)
- Printing cards.

> >-viewCardTitle=Card for %S
> Two alternative suggestions:
> Contact: %S
> %S

I think TB definitely doesn't want "Contact: %S". I see no value in providing a "%S" option unless SM wants "Contact: %S" (and Karsten didn't say that SM did ;-) ).

Most of the other comments are obsolete due to the way I've changed the patch.
Attached patch The fix v2Splinter Review
See comment 12 for details - rerequesting some reviews because of the slight changes in the patch.
Attachment #354939 - Attachment is obsolete: true
Attachment #356934 - Flags: ui-review?(clarkbw)
Attachment #356934 - Flags: superreview?(neil)
Attachment #356934 - Flags: review?(Pidgeot18)
Attachment #354939 - Flags: superreview?(neil)
Attachment #356934 - Flags: superreview?(neil) → superreview+
Attachment #356934 - Flags: review?(Pidgeot18) → review+
Attachment #356934 - Flags: ui-review?(clarkbw) → ui-review+
Checked in: http://hg.mozilla.org/comm-central/rev/c5c795d2335c
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.