Closed Bug 664726 Opened 13 years ago Closed 13 years ago

Add hooks to make address book more extend-able

Categories

(Thunderbird :: Address Book, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 7.0

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(1 file, 6 obsolete files)

My work with EDS contacts integration requires a few additional hooks in the TB address book.
Attached patch WIP Patch V1 (obsolete) — Splinter Review
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.
Assignee: nobody → mconley
Attachment #539799 - Flags: feedback?(Pidgeot18)
Depends on: 652855
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 ;-).
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 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 ;-)
Attachment #539799 - Flags: feedback?(Pidgeot18) → feedback-
Attached patch WIP Patch v2 (obsolete) — Splinter Review
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!
Attachment #539799 - Attachment is obsolete: true
Attachment #541493 - Flags: feedback?(Pidgeot18)
Comment on attachment 541493 [details] [diff] [review]
WIP Patch v2

Review of attachment 541493 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #541493 - Flags: feedback?(Pidgeot18) → feedback+
Attached patch Patch v1 (obsolete) — Splinter Review
Mostly styling fixups.
Attachment #541493 - Attachment is obsolete: true
Attachment #542814 - Flags: review?(Pidgeot18)
Comment on attachment 542814 [details] [diff] [review]
Patch v1

Ack - I have a few changes to make to this still.  Cancelling review request.
Attachment #542814 - Flags: review?(Pidgeot18)
Attached patch Patch v2 (obsolete) — Splinter Review
Ok, I think I've got everything I need with this patch.  Have at it!

Thanks,

-Mike
Attachment #542814 - Attachment is obsolete: true
Attachment #543696 - Flags: review?(mbanner)
Attachment #543696 - Flags: review?(Pidgeot18)
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 on attachment 543696 [details] [diff] [review]
Patch v2

Argh - one final change required.  Review cancelled - will be back once my try build passes.
Attachment #543696 - Flags: review?(mbanner)
Attachment #543696 - Flags: review?(Pidgeot18)
Attached patch Patch v3 (obsolete) — Splinter Review
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!
Attachment #543696 - Attachment is obsolete: true
Attachment #543830 - Flags: review?(mbanner)
Attachment #543830 - Flags: review?(Pidgeot18)
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.
Attachment #543696 - Attachment is obsolete: false
Attached patch Patch v4 (obsolete) — Splinter Review
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.
Attachment #543696 - Attachment is obsolete: true
Attachment #543830 - Attachment is obsolete: true
Attachment #543830 - Flags: review?(mbanner)
Attachment #543830 - Flags: review?(Pidgeot18)
Attachment #543836 - Flags: review?(mbanner)
Attachment #543836 - Flags: review?(Pidgeot18)
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.
Attachment #543836 - Flags: review?(Pidgeot18) → review+
Comment on attachment 543836 [details] [diff] [review]
Patch v4

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

r=Standard8 assuming Joshua's comments are addressed.
Attachment #543836 - Flags: review?(mbanner) → review+
Attached patch Patch v6Splinter Review
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!
Attachment #543836 - Attachment is obsolete: true
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/2a1da01c4f76
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 7.0
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;
+}
Depends on: 690655
Blocks: 761852
You need to log in before you can comment on or make changes to this bug.