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)
MailNews Core
Address Book
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b2
People
(Reporter: sspitzer, Assigned: standard8)
References
Details
Attachments
(1 file, 1 obsolete file)
6.12 KB,
patch
|
jcranmer
:
review+
neil
:
superreview+
clarkbw
:
ui-review+
|
Details | Diff | Splinter Review |
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.
Updated•20 years ago
|
Product: Browser → Seamonkey
Updated•19 years ago
|
Assignee: sspitzer → mail
Assignee | ||
Comment 1•19 years ago
|
||
Changing dependency, as bug 126498 is now a dup, though I think this will be resolved WFM/invalid when bug 64305 is fixed.
Comment 2•19 years ago
|
||
(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 !?
Assignee | ||
Comment 3•19 years ago
|
||
(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 | ||
Updated•18 years ago
|
Assignee: bugzilla → nobody
Keywords: helpwanted
Assignee | ||
Comment 4•18 years ago
|
||
> Therfore moving this to core & taking.
Ok, I'm not looking at this at the current time.
Updated•16 years ago
|
Product: Core → MailNews Core
Updated•16 years ago
|
Target Milestone: mozilla1.9alpha1 → ---
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → bugzilla
Blocks: 455246
Flags: wanted-thunderbird3+
Keywords: helpwanted
Target Milestone: --- → Thunderbird 3.0b2
Assignee | ||
Comment 5•15 years ago
|
||
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 6•15 years ago
|
||
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
Assignee | ||
Comment 7•15 years ago
|
||
(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 8•15 years ago
|
||
Comment on attachment 354939 [details] [diff] [review] Proposed Fix has always seemed superfluous. drop it!
Attachment #354939 -
Flags: ui-review?(clarkbw) → ui-review+
Comment 9•15 years ago
|
||
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+
Updated•15 years ago
|
Attachment #354939 -
Flags: review?(mnyromyr) → review+
Comment 10•15 years ago
|
||
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 11•15 years ago
|
||
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 ||.
Assignee | ||
Comment 12•15 years ago
|
||
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.
Assignee | ||
Comment 13•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #356934 -
Flags: superreview?(neil) → superreview+
Updated•15 years ago
|
Attachment #356934 -
Flags: review?(Pidgeot18) → review+
Updated•15 years ago
|
Attachment #356934 -
Flags: ui-review?(clarkbw) → ui-review+
Assignee | ||
Comment 14•15 years ago
|
||
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.
Description
•