Open
Bug 97294
Opened 23 years ago
Updated 3 months ago
New address book imported from CSV does not generate displaynames
Categories
(MailNews Core :: Address Book, enhancement)
MailNews Core
Address Book
Tracking
(Not tracked)
ASSIGNED
People
(Reporter: mythdraug, Assigned: scott+mozilla)
References
()
Details
(Keywords: testcase, Whiteboard: [has draft patch][gs])
Attachments
(2 files, 1 obsolete file)
139 bytes,
text/plain
|
Details | |
6.71 KB,
patch
|
scott+mozilla
:
review-
|
Details | Diff | Splinter Review |
Build: 2001082703 Platform: Win32 Observed behavior: Import from CSV. Assign field for LastName and FirstName Choose not to import DisplayName (does not exist in file) Import. DisplayName is blank. Expected behavior: DisplayName would be populated with the combined "FirstName LastName" or "LastName, FirstName"
Reporter | ||
Comment 1•23 years ago
|
||
Reporter | ||
Comment 2•23 years ago
|
||
Mapping used in testcase: Work Phone -> Work Last Name -> Last Name First Name -> First Name Custom 1 -> VM Box Home Phone -> Home Pager Number -> Pager Fax Number -> Fax Cellular Number-> Cell Phone Primary Email -> E-mail Notes -> Other All other AB fields were deselected.
Comment 3•23 years ago
|
||
Marking these all WORKSFORME sorry about lack of response but were very overloaded here. Only reopen the bug if you can reproduce with the following steps: 1) Download the latest nightly (or 0.9.6 which should be out RSN) 2) Create a new profile 3) test the bug again If it still occurs go ahead and reopen the bug. Again sorry about no response were quite overloaded here and understaffed.
Status: UNCONFIRMED → RESOLVED
Closed: 23 years ago
Resolution: --- → WORKSFORME
Comment 4•23 years ago
|
||
reopening.
bug 101145 had a similar problem with blank Display Name, later fixed in bug 99391. mythdraug: are you still seeing this bug?
Reporter | ||
Comment 6•23 years ago
|
||
Yes. Just verified that with 2002020703, the testcase above does not generate a displayname when imported.
Reporter | ||
Updated•23 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 7•22 years ago
|
||
taking all of chuang's bugs. she doesn't work on mozilla anymore.
Assignee: chuang → sspitzer
Assignee | ||
Comment 9•20 years ago
|
||
This patch will patch nsTextAddress.cpp/ProcessLine to add the generation of the "Display Name" field should none be provided by a text-format import. This should resolve this case. I would note that other importing modules are not effected by this patch and I am unaware of whether they suffer from the same lack of functionality. It would be much more elegant to solve this within AddCardRowToDB but I'm not sure that is entirely possible without signifigant revision or even needed.
Assignee | ||
Updated•20 years ago
|
Attachment #164315 -
Flags: review?(sspitzer)
Assignee | ||
Updated•20 years ago
|
Attachment #164315 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 10•20 years ago
|
||
Marked this for review. I'm not sure who is doing the reviewing for this, so I ended up marking it for two different people; my apologies for the annoyance.
Updated•20 years ago
|
Product: Browser → Seamonkey
Updated•19 years ago
|
Assignee: cavin → nobody
Component: Address Book → MailNews: Address Book
Product: Mozilla Application Suite → Core
QA Contact: nbaca → addressbook
Updated•19 years ago
|
Attachment #164315 -
Flags: review?(sspitzer) → review?(bugzilla)
Updated•19 years ago
|
Assignee: nobody → scott
Comment 11•19 years ago
|
||
Comment on attachment 164315 [details] [diff] [review] Patch to 97294 Thanks for the nice patch, sorry its taken so long for someone to look at it. Works well, but could do with a couple of tweaks: + switch( fieldNum) { ... + } Please add in a default: break; option to the switch. Although not strictly required, I believe its better practice to have one. + displayName.Append(PRUnichar(' ')); It should be: displayName.Append(NS_LITERAL_STRING(" "); this aviods run-time conversions. + if (!firstName.IsEmpty()) { + displayName = firstName; + } + if (!firstName.IsEmpty() && !lastName.IsEmpty()) { + displayName.Append(PRUnichar(' ')); + } Move the second if inside the first if statement, and that way you can drop the second check for !firstName.IsEmpty(). If you could update the patch with those changes, and re-request review from myself, that would be good (hence I assigned the bug to you). If you can't do that, reassign the bug to the default owner, and let us know.
Attachment #164315 -
Flags: review?(neil.parkwaycc.co.uk)
Attachment #164315 -
Flags: review?(bugzilla)
Attachment #164315 -
Flags: review-
Updated•19 years ago
|
Severity: major → enhancement
OS: Windows 2000 → All
Hardware: PC → All
Comment 12•19 years ago
|
||
(In reply to comment #11) > (From update of attachment 164315 [details] [diff] [review] [edit]) > Works well, but could do with a couple of tweaks: Ok, I've just been reminded that there are localisation issues here. Some locales require the lastname before the first name. This is currently set via the preference: mail.addr_book.displayName.lastnamefirst See http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/resources/content/abCardOverlay.js#502 and http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/resources/content/abCardOverlay.js#307 for how the new card dialog actually does it. Also, should we have an option to turn this on and off? See bug 180057 for one example where I added an option to skip the first line of imported files.
Comment 13•18 years ago
|
||
currently, in thunderbird 1.5.0.10, our users are really upset about the lack of such feature. May I ask what's the status of this ? Are we going to see this in tb-2 ?
Updated•16 years ago
|
Product: Core → MailNews Core
Assignee | ||
Comment 14•16 years ago
|
||
I've updated the code to generate names in the same manner that Card's do (respecting the first/last order and format strings). I am using the displayName.autoGeneration preference to decide whether to do it or not, but a checkbox may still be desirable.
Attachment #164315 -
Attachment is obsolete: true
Attachment #363018 -
Flags: review?
Comment 15•16 years ago
|
||
Scott: please set the review flag of the patch to the email address of a suitable reviewer - like standard8. (See http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree)
Status: NEW → ASSIGNED
Assignee | ||
Updated•16 years ago
|
Attachment #363018 -
Flags: review? → review?(bugzilla)
Assignee | ||
Comment 16•16 years ago
|
||
Comment on attachment 363018 [details] [diff] [review] Adds DisplayName generation to text-based import. Updated review flag to Mark Banner.
Comment 17•16 years ago
|
||
Comment on attachment 363018 [details] [diff] [review] Adds DisplayName generation to text-based import. >+#define PREF_MAIL_ADDR_BOOK_DISPLAYNAME_AUTOGENERATION "mail.addr_book.displayName.autoGeneration" I think we should expose this in the import UI. Some people may not have display names, and prefer not to generate them, I've just cc'ed our user-experience guy in case he disagrees. >+ case 2: >+ hasDisplayName = PR_TRUE; >+ break; What about the case where the user has chosen to import an existing address book, but hasn't got a display name in some/all rows? >+ } else if (!primaryEmail.IsEmpty()) { >+ // see bug #211078 >+ // if there is no generated name at this point >+ // use the userid from the email address >+ // it is better than nothing. >+ displayName = primaryEmail; >+ PRInt32 index = displayName.FindChar('@'); >+ if (index != -1) >+ displayName.SetLength(index); I'm not sure we want to copy this clause. If we haven't got a first/last name then I think generating one from the email in this case isn't such a good idea, and we should just give up.
Assignee | ||
Comment 18•16 years ago
|
||
(In reply to comment #17) > I think we should expose this in the import UI. Some people may not have > display names, and prefer not to generate them, I've just cc'ed our > user-experience guy in case he disagrees. > > What about the case where the user has chosen to import an existing address > book, but hasn't got a display name in some/all rows? With that in mind, I would agree that having a check box (defaulting to the state of the preference in mail.addr_book.displayName.autoGeneration) would be best. In that case, the user can make the decision if only some rows have display names. In the case that the user would like to replace all of the display names, they could simply uncheck importing that field while auto-generate was enabled. > >+ } else if (!primaryEmail.IsEmpty()) { > >+ // see bug #211078 > >+ // if there is no generated name at this point > >+ // use the userid from the email address > >+ // it is better than nothing. > >+ displayName = primaryEmail; > >+ PRInt32 index = displayName.FindChar('@'); > >+ if (index != -1) > >+ displayName.SetLength(index); > > I'm not sure we want to copy this clause. If we haven't got a first/last name > then I think generating one from the email in this case isn't such a good idea, > and we should just give up. Having now read bug #211078, I would agree it was an ill-advised copy. I will wait to hear the opinion of the user-experience guy before revising further.
Assignee | ||
Comment 19•16 years ago
|
||
Comment on attachment 363018 [details] [diff] [review] Adds DisplayName generation to text-based import. Removed review flag pending suggested UI changes.
Attachment #363018 -
Flags: review?(bugzilla) → review-
See Also: → https://launchpad.net/bugs/600755
Comment 20•14 years ago
|
||
Scott, will you be able to update the patch? reported in http://getsatisfaction.com/mozilla_messaging/topics/address_book_not_working
Whiteboard: [has draft patch][gs]
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•