Add UI to swap FN/LN to address book.

RESOLVED FIXED in mozilla1.3beta

Status

RESOLVED FIXED
16 years ago
14 years ago

People

(Reporter: nhottanscp, Assigned: nhottanscp)

Tracking

({intl})

Trunk
mozilla1.3beta

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

16 years ago
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.
(Assignee)

Updated

16 years ago
QA Contact: nbaca → marina
(Assignee)

Updated

16 years ago
Keywords: intl
Target Milestone: --- → mozilla1.3alpha
(Assignee)

Comment 1

16 years ago
Created attachment 107374 [details] [diff] [review]
Adding swapFnLn UI.

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

Comment 2

16 years ago
The change of nsIAddrDatabase.idl needs bug 179823.
Depends on: 179823
(Assignee)

Comment 3

16 years ago
Created attachment 107376 [details] [diff] [review]
Make the UI localizable and hide it by default
Attachment #107374 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #107376 - Flags: review?(cavin)

Comment 4

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

r=cavin.
Attachment #107376 - Flags: review?(cavin) → review+
(Assignee)

Updated

16 years ago
Attachment #107376 - Flags: superreview?(sspitzer)
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?)
Attachment #107376 - Flags: superreview?(sspitzer) → superreview-
since you are in there, can you fix my comment:

"we we do that" -> "when we do that"
(Assignee)

Comment 7

16 years ago
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.
(Assignee)

Comment 8

16 years ago
Created attachment 107917 [details] [diff] [review]
Updated to address reviewer's comment.
Attachment #107376 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #107917 - Flags: superreview?(sspitzer)
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?
(Assignee)

Comment 10

16 years ago
Created attachment 108088 [details] [diff] [review]
Changed to resort for kPhoneticNameColumn.
Attachment #107917 - Attachment is obsolete: true
Comment on attachment 108088 [details] [diff] [review]
Changed to resort for kPhoneticNameColumn.

sr=sspitzer
Attachment #108088 - Flags: superreview+
Comment on attachment 107917 [details] [diff] [review]
Updated to address reviewer's comment.

obsolete.
Attachment #107917 - Flags: superreview?(sspitzer) → superreview-
(Assignee)

Comment 13

16 years ago
checked in to the trunk

Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

16 years ago
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

16 years ago
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.
(Assignee)

Comment 16

16 years ago
Created attachment 108119 [details] [diff] [review]
Init displayNameLastnamefirst.
(Assignee)

Updated

16 years ago
Attachment #108119 - Flags: superreview?(sspitzer)
Attachment #108119 - Flags: review?(cavin)

Comment 17

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

r=cavin.
Attachment #108119 - Flags: review?(cavin) → review+
Comment on attachment 108119 [details] [diff] [review]
Init displayNameLastnamefirst.

sr=sspitzer
Attachment #108119 - Flags: superreview?(sspitzer) → superreview+
(Assignee)

Updated

16 years ago
Attachment #108119 - Flags: approval1.3a?

Comment 19

16 years ago
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?
(Assignee)

Comment 20

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

The last patch for the uninitialized value can be done after 1.3a.
Attachment #108119 - Flags: approval1.3a? → approval1.3a-

Comment 21

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

unsetting request (moving the flag from ? to empty rather than to -).
Attachment #108119 - Flags: approval1.3a-
(Assignee)

Comment 22

16 years ago
reoopen for the remaining problem
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla1.3alpha → mozilla1.3beta
(Assignee)

Comment 23

16 years ago
checked in to the trunk
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.