181883 - Add UI to swap FN/LN to address book.

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Address Book & Contacts, defect)

Description

[nhottanscp]

For collected address book, there are cases which FN and LN are assigned
incorrectly. This is because e-mail pretty name has no information about FN and LN.
The same happens for imported address book.
This is about adding UI to swap FN and LN easily. This is similar to what
Apple's address book has.

Comment 1

[nhottanscp]

Attachment: Adding swapFnLn UI.

There is a remaining change to hide the UI as default.

Comment 2

[nhottanscp]

The change of nsIAddrDatabase.idl needs bug 179823.

Comment 3

[nhottanscp]

Attachment: Make the UI localizable and hide it by default

Comment 4

[Cavin Song]

Comment on attachment 107376 [details] [diff] [review]
Make the UI localizable and hide it by default

r=cavin.

Comment 5

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

Comment on attachment 107376 [details] [diff] [review]
Make the UI localizable and hide it by default

looks good, but I have a few nits / comments / questions.

1)

you can move the bundleService declaration inside the if
(displayNameAutoGeneration) {} block.

+  nsCOMPtr<nsIStringBundleService> bundleService;
+  nsCOMPtr<nsIStringBundle> bundle;
+  if (displayNameAutoGeneration)
+  {
+    rv =
prefBranch->GetBoolPref(PREF_MAIL_ADDR_BOOK_DISPLAYNAME_LASTNAMEFIRST,
&displayNameLastnamefirst);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    bundleService = do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    rv =
bundleService->CreateBundle("chrome://messenger/locale/addressbook/addressBook.
properties", 
+				      getter_AddRefs(bundle));
+    NS_ENSURE_SUCCESS(rv, rv);
+  }

2)

Why do fn and ln need to be non-empty?

I think we only want to skip this code if both are empty

+
+	   // generate display name using the new order
+	   if (displayNameAutoGeneration &&
+	       !fn.IsEmpty() && !ln.IsEmpty())
+	   {

instead do:

+	   if (displayNameAutoGeneration &&
+	       (!fn.IsEmpty() || !ln.IsEmpty()))

3)

can you check the return value of this call to GetDisplayName()

+	     // get the current display name
+	     nsXPIDLString dn;
+	     abCard->GetDisplayName(getter_Copies(dn));

4)

instead of nsCRT::strcmp(), why not use .Equals()?

also, can you check the rv of SetDisplayName()

+
+	     // swap the display name if not edited
+	     if (displayNameLastnamefirst)
+	     {
+	       if (!nsCRT::strcmp(dn.get(), dnLnFn.get()))
+		 abCard->SetDisplayName(dnFnLn);
+	     }
+	     else
+	     {
+	       if (!nsCRT::strcmp(dn.get(), dnFnLn.get()))
+		 abCard->SetDisplayName(dnLnFn);
+	     }

5)

can you check the rv of these calls?

+	   // swap phonetic names
+	   abCard->GetPhoneticFirstName(getter_Copies(fn));
+	   abCard->GetPhoneticLastName(getter_Copies(ln));

6)

instead of nsCRT::strcmp(), why not use .Equals()?

+  if (!nsCRT::strcmp(mSortColumn.get(),
NS_LITERAL_STRING(GENERATED_NAME_COLUMN_ID).get()) ||
+      !nsCRT::strcmp(mSortColumn.get(),
NS_LITERAL_STRING(kPhoneticNameColumn).get()))
+    rv = SortBy(mSortColumn.get(), mSortDirection.get());
+  else
+    rv = InvalidateTree(ALL_ROWS);

also, you care if mSortColumn is kPriEmailColumn.  If it is, the secondary sort
column is GENERATED_NAME_COLUMN_ID
which means you need to resort.

See the comment in nsAbView::Observe()

      // the PREF_MAIL_ADDR_BOOK_LASTNAMEFIRST pref affects how the
GeneratedName column looks.
      // so if the GeneratedName is our primary or secondary sort,
      // we need to resort.
      //
      // XXX optimize me
      // PrimaryEmail is always the secondary sort, unless it is currently the
      // primary sort.	So, if PrimaryEmail is the primary sort, 
      // GeneratedName might be the secondary sort.
      //
      // one day, we can get fancy and remember what the secondary sort is.
      // we we do that, we can fix this code.  at best, it will turn a sort
into a invalidate.
      // 
      // if neither the primary nor the secondary sorts are GeneratedName, all
we have to do is
      // invalidate (to show the new GeneratedNames), but the sort will not
change.
      if (!nsCRT::strcmp(mSortColumn.get(),
NS_LITERAL_STRING(GENERATED_NAME_COLUMN_ID).get()) ||
	  !nsCRT::strcmp(mSortColumn.get(),
NS_LITERAL_STRING(kPriEmailColumn).get())) {
	rv = SortBy(mSortColumn.get(), mSortDirection.get());
      }
      else {
	rv = InvalidateTree(ALL_ROWS);
      }

can you move this block out to a method, add in the check for
kPhoneticNameColumn (we want ::Observe() to check that too)
and call from both places (and fix the nsCRT::strcmp() code?)

Comment 6

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

since you are in there, can you fix my comment:

"we we do that" -> "when we do that"

Comment 7

[nhottanscp]

2)
It does the swap only if both fields are not empty because if either is empty
then the swapped result would not changed and no swap is necessary.

Comment 8

[nhottanscp]

Attachment: Updated to address reviewer's comment.

Comment 9

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

Comment on attachment 107917 [details] [diff] [review]
Updated to address reviewer's comment.

1)

+  if (mSortColumn.Equals(NS_LITERAL_STRING(GENERATED_NAME_COLUMN_ID)) ||
+      mSortColumn.Equals(NS_LITERAL_STRING(kPriEmailColumn))) {
+    rv = SortBy(mSortColumn.get(), mSortDirection.get());

don't you want to also check if we are sorted by the kPhoneticNameColumn
column?

if we are sorted by that way, and someone swaps, we need to re-sort, right?

Comment 10

[nhottanscp]

Attachment: Changed to resort for kPhoneticNameColumn.

Comment 11

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

Comment on attachment 108088 [details] [diff] [review]
Changed to resort for kPhoneticNameColumn.

sr=sspitzer

Comment 12

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

Comment on attachment 107917 [details] [diff] [review]
Updated to address reviewer's comment.

obsolete.

Comment 13

[nhottanscp]

checked in to the trunk

Comment 14

[nhottanscp]

The UI is disabled as default.
In order to test, localize, set "hideSwapFnLnUI" to "false" in abMainWindow.dtd.

Index: resources/locale/en-US/abMainWindow.dtd
===================================================================
RCS file:
/cvsroot/mozilla/mailnews/addrbook/resources/locale/en-US/abMainWindow.dtd,v
retrieving revision 1.48
diff -u -r1.48 abMainWindow.dtd
--- resources/locale/en-US/abMainWindow.dtd	3 Oct 2002 20:35:01 -0000	1.48
+++ resources/locale/en-US/abMainWindow.dtd	25 Nov 2002 20:01:33 -0000
@@ -56,6 +56,8 @@
 <!ENTITY deleteAbCmd.label                              "Delete Address Book">  
 <!ENTITY deleteCardCmd.label                            "Delete Card">  
 <!ENTITY deleteCardsCmd.label                           "Delete Selected Cards">  
+<!ENTITY swapFirstNameLastNameCmd.label                 "Swap First/Last Name">
+<!ENTITY swapFirstNameLastNameCmd.accesskey             "w">  
 <!ENTITY propertiesCmd.label                            "Properties...">
 <!-- LOCALIZATION NOTE (propertiesCmd.accesskey) : DONT_TRANSLATE -->  
 <!ENTITY propertiesCmd.accesskey                        "i">  
@@ -105,3 +107,7 @@
 
 <!-- Status Bar -->  
 <!ENTITY  statusText.label "">
+
+<!-- LOCALIZATION NOTE (hideSwapFnLnUI) : DONT_TRANSLATE -->
+<!-- Swap FN/LN UI  Set to "false" to show swap fn/ln UI -->
+<!ENTITY  hideSwapFnLnUI "true">

Comment 15

[Aleksey Nogin]

This commit have added a new "might be used uninitialized" warning on brad TBox:

+mailnews/addrbook/src/nsAbView.cpp:1194
+ `PRBool displayNameLastnamefirst' might be used uninitialized in this function

P.S. According to bug 179819 such warnings ought to be considered compilation
errors.

Comment 16

[nhottanscp]

Attachment: Init displayNameLastnamefirst.

Comment 17

[Cavin Song]

Comment on attachment 108119 [details] [diff] [review]
Init displayNameLastnamefirst.

r=cavin.

Comment 18

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

Comment on attachment 108119 [details] [diff] [review]
Init displayNameLastnamefirst.

sr=sspitzer

Comment 19

[Asa Dotzler [:asa]]

This is fixed or it isn't? If it's not fixed then can you reopen the bug. Is
this something that really needs to be in for 1.3a?

Comment 20

[nhottanscp]

Comment on attachment 108119 [details] [diff] [review]
Init displayNameLastnamefirst.

The last patch for the uninitialized value can be done after 1.3a.

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 108119 [details] [diff] [review]
Init displayNameLastnamefirst.

unsetting request (moving the flag from ? to empty rather than to -).

Comment 22

[nhottanscp]

reoopen for the remaining problem

Comment 23

[nhottanscp]

checked in to the trunk