664726 - Add hooks to make address book more extend-able

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

My work with EDS contacts integration requires a few additional hooks in the TB address book.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch V1

In order to get auto-complete working with my EDS contacts add-on, I needed to create a scriptable wrapper for the nsAbQueryStringToExpression::Convert method.  Blake suggested I stuff it into something like nsIAbUtilsService for now, just in case something similar comes up.

I've also added a PhotoType for contacts, "data_uri", which allows me to render contact photos dumped out via EDS.

All feedback welcome.

Comment 2

[Joshua Cranmer [:jcranmer]]

Why are you making a new interface instead of placing the method on nsIAbManager? I don't have a real problem with this if you're going to be adding, e.g., the rest of that class to IDL, but a new interface for a single method seems like overkill.

And if you're going to go ahead and make a new C++ class, you might as well help introduce namespaces ;-).

Comment 3

[Mark Banner (:standard8)]

I actually wonder if it still makes sense to keep the query string stuff, but I can't think of a good replacement at the moment.

Comment 4

[Joshua Cranmer [:jcranmer]]

Comment on attachment 539799 [details] [diff] [review]
WIP Patch V1

Review of attachment 539799 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/addrbook/content/abCardViewOverlay.js
@@ +215,5 @@
> +    var photoDataUri = "data:" + photoMimeType + ";base64," + btoa(photoData);
> +    cvData.cvPhoto.setAttribute("src", photoDataUri);
> +  } else {
> +    cvData.cvPhoto.setAttribute("src", getPhotoURI(card.getProperty("PhotoName")));
> +  }

Do we seriously not support data: URIs as a photoURI?

If you do add a new property, PLEASE add it to the giant comment header atop nsIAbCard.

Oh yeah, this may also need handling in vCard code, since I'm sure people would want to be able to import photos in vcards ;-)

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch v2

Hey - thanks for the feedback!

So I've moved convertQueryToExpression into nsIAbManager, as suggested.  I've also done some abstraction in the way that contact photos are handled, for easier overriding / extensibility.

All feedback welcome.  Thanks!

Comment 6

[Joshua Cranmer [:jcranmer]]

Comment on attachment 541493 [details] [diff] [review]
WIP Patch v2

Review of attachment 541493 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Mostly styling fixups.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 542814 [details] [diff] [review]
Patch v1

Ack - I have a few changes to make to this still.  Cancelling review request.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Ok, I think I've got everything I need with this patch.  Have at it!

Thanks,

-Mike

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Also, just a reminder - this patch, like the one for bug 652855, *must* land on Tuesday if I want my EDS integration add-on to be ready for Ubuntu Oneiric.  These two patches not landing would be a pretty major setback for that project.  Any and all help there would be appreciated!

Thanks,

-Mike

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 543696 [details] [diff] [review]
Patch v2

Argh - one final change required.  Review cancelled - will be back once my try build passes.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3

Alright, just one last change - nsIAbDirectory.addMailList now returns the added mailing list, much like nsIAbDirectory.addCard.

I'm done messing with this.  Sorry for the flip flopping!

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Oh, and results of my try build will be available here:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=42dce0577307

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 543696 [details] [diff] [review]
Patch v2

Review of attachment 543696 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/addrbook/content/abCardOverlay.js
@@ +334,5 @@
> +    gOnLoadListeners.splice(fIndex, 1);
> +}
> +
> +// this is used by people who extend the ab card dialog
> +// like Netscape does for screenname

I think we can drop the netscape references now.

@@ +340,5 @@
> +{
> +  if (!gOnLoadListeners.length)
> +    return;
> +
> +  for ( var i = 0; i < gOnLoadListeners.length; i++ )

nit: no space after ( or before ).

@@ +1049,5 @@
> +
> +  onSet: function(aCard, aDocument, aTargetID) {
> +    aDocument
> +    .getElementById(aTargetID)
> +    .setAttribute("src", defaultPhotoURI);

nit: We generally prefer:

aDocument.getElementById(aTargetID)
         .setAttribute("src", defaultPhotoURI);

(several places in this file).

::: mailnews/addrbook/public/nsIAbDirectory.idl
@@ +297,5 @@
>  
>    /**
> +   * The following attributes are read from an nsIAbDirectory via the above methods:
> +   *
> +   * HidesRecipients (Boolean)

Ideally I think this would actually use the group syntax rather than BCC. However, I also know that group syntax is rather broken in mailnews at the moment. So this is fine for now, just noting that in future we may want to consider group syntax here.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4

Thanks for the review!

> I think we can drop the netscape references now.

Done.

> nit: no space after ( or before ).

Fixed.

> nit: We generally prefer:
>
> aDocument.getElementById(aTargetID)
>         .setAttribute("src", defaultPhotoURI);
>
> (several places in this file).

Fixed.

> Ideally I think this would actually use the group syntax rather than BCC. 
> However, I also know that group syntax is rather broken in mailnews at the 
> moment. So this is fine for now, just noting that in future we may want to 
> consider group syntax here.

Alright - I've added a FIXME comment in abCommon.js, at the point where we use BCC.

Comment 16

[Joshua Cranmer [:jcranmer]]

Comment on attachment 543836 [details] [diff] [review]
Patch v4

Review of attachment 543836 [details] [diff] [review]:
-----------------------------------------------------------------

Overall, this looks pretty good. I'm not thrilled with the fact that adding new photo handlers requires registering stuff in two different places, but I don't see any clear way around it.

Modulo the review comments I have r+. And don't forget to put some documentation on mdc about adding new photo handlers.

::: mail/components/addrbook/content/abCardOverlay.js
@@ +1026,5 @@
> + *   input fields with information about aCard's photo.  Note that
> + *   this does NOT make aCard's photo appear in the contact editor -
> + *   this is left to the onSet function.
> + *
> + * onSet: function(aCard, aDocument, aTargetID):

The "onSet" method is misnamed, especially since you're calling it (essentially) from a method getCardValues. A better name would probably be "onShow".

@@ +1039,5 @@
> + *   onSave method is responsible for analyzing the photo of this
> + *   type requested by the user, and storing it, as well as the
> + *   other fields required by onLoad/onSet to retrieve and display
> + *   the photo again.
> +*/

Also, for this block of comments, please document that there is a meaningful return value. And nit: the close */ is slightly misaligned.

@@ +1201,5 @@
> +      break;
> +  }
> +}
> +
> +registerDefaultPhotoHandler("generic");

Why not just say registerPhotoHandler("generic", genericPhotoHandler); and remove the registerDefaultPhotoHandler? This is somewhat different from the semantics in your changes in abCardViewOverlay.js, and I suspect that anyone who is overriding the default photo handler would know to put it back when the deregister it.

::: mail/components/addrbook/content/abCardViewOverlay.js
@@ +617,5 @@
> + *
> + * onDisplay: function(aCard, aImg):
> + *    onDisplay is responsible for determining how to retrieve
> + *    the photo from aCard, and for displaying it in aImg.  Returns
> + *    true if successful.

If you only need one function in the handler, it's probably better to make the handler map just map strings -> functions as opposed to string -> objects.

::: mail/components/addrbook/content/abCommon.js
@@ +363,5 @@
> +            directory.getBoolValue("HidesRecipients", false))
> +          // FIXME:  We're using BCC right now to hide recipients from one
> +          // another.  We should probably use group syntax, but that's broken
> +          // right now, so this will have to do.
> +          composeFields.bcc = GetSelectedAddressesFromDirTree();

If we have a bug on file about the mailing list syntax, please reference it in the comment; if not, file a bug and reference it.

::: mailnews/addrbook/src/nsAbManager.cpp
@@ +1282,5 @@
>  }
> +
> +NS_IMETHODIMP
> +nsAbManager::ConvertQueryStringToExpression(const char *aQueryString,
> +                                                 nsIAbBooleanExpression **_retval NS_OUTPARAM)

Nit: Align the parameter and remove NS_OUTPARAM.

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 543836 [details] [diff] [review]
Patch v4

Review of attachment 543836 [details] [diff] [review]:
-----------------------------------------------------------------

r=Standard8 assuming Joshua's comments are addressed.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v6

Thanks for the review, jcranmer!

> A better name would probably be "onShow".

Done.

> Also, for this block of comments, please document that there is a meaningful > return value. And nit: the close */ is slightly misaligned.

Done.

> Why not just say registerPhotoHandler("generic", genericPhotoHandler); and 
> remove the registerDefaultPhotoHandler? 

Good idea - done.

> If you only need one function in the handler, it's probably better to make 
> the handler map just map strings -> functions as opposed to string -> objects.

Another good idea.  Done.

> If we have a bug on file about the mailing list syntax, please reference it 
> in the comment; if not, file a bug and reference it.

Done.

> Nit: Align the parameter and remove NS_OUTPARAM.

Done.

Thanks again!

Comment 19

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/2a1da01c4f76

Comment 20

[Samuel Müller]

There are some small inconsistency in the photo handlers I stumbled upon:

+/* In order for other photo handlers to be recognized for
+ * a particular type, they must be registered through this
+ * function.
+ * @param aType the type of photo to handle
+ * @param aPhotoHandler the photo handler to register
+ */
+function registerPhotoHandler(aType, aPhotoHandler)
+{
+  gPhotoHandlers[aType] = aPhotoHandler;
+}

This function allows to overwrite an existing handler, while the following function does not. I have to use gPhotoDisplayHandlers directly, which is not the intention uf using the registering functions.

+/* In order for a photo display handler to be registered for
+ * a particular photo type, it must be registered here.
+ */
+function registerPhotoDisplayHandler(aType, aPhotoDisplayHandler)
+{
+  if (!gPhotoDisplayHandlers[aType])
+    gPhotoDisplayHandlers[aType] = aPhotoDisplayHandler;
+}

---

The generic photo is inside a dropdown list, but the generic handler does not support this by default. Even if the generic list has been extended, it always assigns the defaultPhotoURI. Also the onLoad and onSave function should assume that there could be other list entries than the default photo, or there should be no list at all for the generic photo.

+var genericPhotoHandler = {
+
+  onLoad: function(aCard, aDocument) {
+    return true;
+  },
+
+  onShow: function(aCard, aDocument, aTargetID) {
+    aDocument.getElementById(aTargetID)
+             .setAttribute("src", defaultPhotoURI);
+    return true;
+  },

---

Is it a typo that one registering function's name starts uppercase, the other lowercase? (abCardOverlay.js)

+function RegisterLoadListener(aFunc)
+{
+  gOnLoadListeners[gOnLoadListeners.length] = aFunc;
+}

vs.

+function registerPhotoHandler(aType, aPhotoHandler)
+{
+  gPhotoHandlers[aType] = aPhotoHandler;
+}