408180 - Generating a name from a card should be a function of nsIAbCard and not of nsIAddrBookSession

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

On nsIAddrBookSession we currently have:

wstring generateNameFromCard(in nsIAbCard card, in long generateFormat);
wstring generatePhoneticNameFromCard(in nsIAbCard aCard, in boolean aLastNameFirst);

These don't use anything from the addr book session to generate the result. Additionally, all they are doing is calling functions in nsIAbCard to complete the result.

The wstring also means we have to do more work than necessary in some areas of the code.

Therefore we should move these to the nsIAbCard and generate the name from there which is much more logical anyway (and also saves us getting the addr book session service each time as nsAbView::GetCardValue currently does).

I'm also wondering if it would be worth providing a version of the function that can take the string formatting from the string bundle as an argument (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/addrbook/src/nsAddrBookSession.cpp&rev=1.34&mark=222-226#202), so that things like nsAbView which may have to call the function for each card in the list can just pass in the string bundle format and hence we only have to get the bundle once for each time we display all cards in an ab (or less if we cache it).

Comment 1

[Mark Banner (:standard8)]

Attachment: WIP v1

Although this will work, I'm not sure if this is what I'll go for yet.

Comment 2

[Mark Banner (:standard8)]

Attachment: The fix

I think I'm happy with this patch. This simplifies things over what we currently do. The caller can cache the string bundle if they wish, but they don't have to, and it also means we don't have to try and do management of a static member in nsAbCardProperty. Unit tests also included.

Comment 3

[[e-mail]]

Comment on attachment 293391 [details] [diff] [review]
The fix

Excellent though it is, there is a possibility of improvment - apparently idl supports optional arguments these days, so consider making the bundle optional.

Comment 4

[Mark Banner (:standard8)]

Attachment: The fix v2

Slightly better version:

- Merged GenerateName and GenerateNameWithBundle into one function with an optional parameter.
- Reused constant for the string bundle uri in nsAbCardProperty
- included test case that I'd somehow forgotten to put in the previous two patches.

Also requesting sr this time at the same time.

Comment 5

[[e-mail]]

Comment on attachment 293538 [details] [diff] [review]
The fix v2

>+  // No need to check for aBundle present straight away, only do that if we're
>+  // actually going to use it.
>+  if (aGenerateFormat == kDisplayName) {
>+    aResult = m_DisplayName;
>+  }
>+  else if (!m_LastName.IsEmpty() && !m_FirstName.IsEmpty()) {
>+    nsCOMPtr<nsIStringBundle> bundle(aBundle);
>+    if (!bundle) {
>+      // As we know that generateNameWithBundle doesn't use the string
>+      // bundle in a display name format, we can just skip getting the service.
>+      if (aGenerateFormat != kDisplayName) {
>+        nsresult rv;
>+        nsCOMPtr<nsIStringBundleService> stringBundleService =
>+          do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv); 
>+        NS_ENSURE_SUCCESS(rv, rv);
>+        
>+        rv = stringBundleService->CreateBundle(sAddrbookProperties,
>+                                               getter_AddRefs(bundle));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+      }
>+    }
>+    nsresult rv;
Nit: Too many rvs, just one will do.
You're checking aGenerateFormat against kDisplayName twice.

>+  else {
>+    if (m_LastName.Length())
>+      aResult = m_LastName;
>+    else {
>+      // name may be empty here, but that's ok.
>+      aResult = m_FirstName;
>+    }
>+  }
I meant to mention these last time, but forgot :-(
Nit: Please use !m_lastName.IsEmpty()
Nit: Please use else if without the extra {}s
You might like this way of writing the ifs which seems slightly simpler:
if (aGenerateFormat == kDisplayName) {
  aResult = m_DisplayName;
}
else if (m_LastName.IsEmpty()) {
  aResult = m_FirstName;
}
else if (m_FirstName.IsEmpty()) {
  aResult = m_LastName;
}
else {
  /* bundle the rest of the code in here */
}

Comment 6

[Mark Banner (:standard8)]

Attachment: The fix v3

Addressed Neil's comments. Carrying forward his r, requesting sr.

Comment 7

[David :Bienvenu]

Comment on attachment 293727 [details] [diff] [review]
The fix v3

thx, Mark.

Comment 8

[Mark Banner (:standard8)]

Patch checked in -> fixed.