97294 - New address book imported from CSV does not generate displaynames

Status: ASSIGNED

(MailNews Core :: Address Book, enhancement)

Description

[Quinn Yost (mythdraug)]

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"

Comment 1

[Quinn Yost (mythdraug)]

Attachment: CSV testcase

Comment 2

[Quinn Yost (mythdraug)]

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

[Keyser Sose]

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.

Comment 4

[Christopher Hoess (gone)]

reopening.

Comment 5

[R.K.Aa.]

bug 101145 had a similar problem with blank Display Name, later fixed in bug
99391.

mythdraug: are you still seeing this bug?

Comment 6

[Quinn Yost (mythdraug)]

Yes.  Just verified that with 2002020703, the testcase above does
not generate a displayname when imported.

Comment 7

[(not reading, please use [e-mail] instead)]

taking all of chuang's bugs.  she doesn't work on mozilla anymore.

Comment 8

[(not reading, please use [e-mail] instead)]

over to cavin

Comment 9

[Scott Dial]

Attachment: Patch to 97294

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.

Comment 10

[Scott Dial]

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.

Comment 11

[Mark Banner (:standard8)]

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.

Comment 12

[Mark Banner (:standard8)]

(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

[Gilles Dartiguelongue]

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 ?

Comment 14

[Scott Dial]

Attachment: Adds DisplayName generation to text-based import.

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.

Comment 15

[Magnus Melin [:mkmelin]]

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)

Comment 16

[Scott Dial]

Comment on attachment 363018 [details] [diff] [review]
Adds DisplayName generation to text-based import.

Updated review flag to Mark Banner.

Comment 17

[Mark Banner (:standard8)]

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.

Comment 18

[Scott Dial]

(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.

Comment 19

[Scott Dial]

Comment on attachment 363018 [details] [diff] [review]
Adds DisplayName generation to text-based import.

Removed review flag pending suggested UI changes.

Comment 20

[Wayne Mery (:wsmwk)]

Scott, will you be able to update the patch?


reported in http://getsatisfaction.com/mozilla_messaging/topics/address_book_not_working