455714 - nsIAbCollection::getCardFromProperty and nsIAbCollection::cardForEmailAddress should be able to return multiple cards

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Mark Banner (:standard8)]

Per bug 450149 comment 8, we should extend these apis to return multiple cards. The results could then be used to provide matches where two people use the same email address, which could then be handled appropriately by the caller.

Comment 1

[Serge Gautherie (:sgautherie)]

(Mark, you copied the same function twice in the summary.)

*****

IIRC with Mark, about bug 309057 comment 77,
I had an idea that the |getCardForAddress()| there could return |[card, matching]|,
using |matching| to tell if the provided function matched or if returning the first found card with the right email address only.

Mark was concerned about possible inconsistencies on the card which would be returned/used (in different cases).
I'm not sure either way atm, but I postpone this issue to this (later) bug :-|

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #1)
> I'm not sure either way atm, but I postpone this issue to this (later) bug :-|

This bug is only for the API changes to the address book.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

It feels important for extension authors to have some way to get this data in a Tb3 timeframe.  It could be a modification of these APIs or some other API.

Comment 4

[Mark Banner (:standard8)]

Talking to sid on irc today I've realised that this functionality is already available within the address book, though its not easy access.

In summary: You can set up a search uri on an address book for one attribute, then use .childCards to get the cards.

This is more expensive (cpu/time) than a getCardFromProperty equivalent function because it gets all the cards from the database into nsAbCardProperty objects and then filters them - but the function is there.

Slightly extended description:

- Get an nsIAbDirectory URI
- Extend URI with search parameters e.g. "(or(PrimaryEmail,bw,@V))"
- Get a new nsIAbDirectory with the extended URI via nsIAbManager.getDirectory.
- Call .childCards for that nsIAbDirectory.

So I'm not sure we need to block on this because extensions do have a way to do it. Thoughts?

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

That's a reasonable argument, I have to admit, though as you say, it's a pretty ugly workaround.  I've added a few extension authors to get their thoughts on this: folks, is there stuff you couldn't/wouldn't do in your extensions if the workaround Mark describes is the only API available for this functionality?

Comment 6

[Leni]

I won't say I fully understand the suggested workaround but assuming it's performance/resource consumption is no worse than tb2's database.getCardFromAttribute() I can't see it being an issue for zindus.

What would be really nice is if there were some way of doing O(1) lookup of cards.  If uuid:
https://bugzilla.mozilla.org/show_bug.cgi?id=444093
offered that then zindus would probably switch and not need this kind of lookup at all.

In terms of sizing a test case, Google has a limit of 10,000 contacts per addressbook and some users go close to that.

Comment 7

[Mark Banner (:standard8)]

I'm starting to think we do actually want a fast function such as this - although it means another function in nsIAddrDatabase, its the sort of thing we want to be able to do.

For instance it would make searching the AB for the second email address easier and faster which is something I'd like to be able to do for bug 447927.

Comment 8

[Mark Banner (:standard8)]

Unassigning myself from this for now in the hopes that someone can pick it up. I'd certainly like to get this option in for TB 3.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Standard8 suggested that a reasonable API here would be to simply duplicate the two existing method, but pluralize them and have them return nsISimpleEnumerators.  This sounds like a good plan to me.

Comment 10

[u278084]

Sounds good. I'll take the bug.

Comment 11

[u278084]

Attachment: wip

It's been a few weeks, so I wanted to show some progress. I am removing the added member variable m_rowPos in the final patch. If you're brave and take a look, you might miss the part where I |#if 0| a huge part of code which makes the patch work, but I don't know the consequences of removing that code, yet.

Comment 12

[Joshua Cranmer [:jcranmer]]

(In reply to comment #11)
> It's been a few weeks, so I wanted to show some progress. I am removing the
> added member variable m_rowPos in the final patch. If you're brave and take a
> look, you might miss the part where I |#if 0| a huge part of code which makes
> the patch work, but I don't know the consequences of removing that code, yet.

The primary consequence is that you always take the slow path in search. I don't know enough about mork guarantees of how FindRow works under multiple results to know if this is possible to overcome.

I think, for performance reasons, it would be helpful to have the single-return do the O(1) lookup if possible, which would mean conditionally executing the if statement.

Comment 13

[u278084]

Attachment: first attempt

I added a few optional parameters to some functions that lets you keep track of the mork db position, so you can make multiple calls to the function. Other than that, it seems pretty straightforward. And there are tests and comments!

Comment 14

[Joshua Cranmer [:jcranmer]]

One little nit: could you add documentation to the IDL functions?

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 386369 [details] [diff] [review]
first attempt

This is a good first patch and I like the method of implementation. A few comments though:

>diff --git a/mailnews/addrbook/public/nsIAbCollection.idl b/mailnews/addrbook/public/nsIAbCollection.idl

You need to bump the uuid of nsIAbCollection and nsIAddrDatabase. "/msg firebot uuid" on irc to get new numbers.

>+  nsISimpleEnumerator getCardsFromProperty(in string aProperty,
>+                                           in AUTF8String aValue,
>+                                           in boolean aCaseSensitive);
...
>+  nsISimpleEnumerator getCardsFromAttribute(in nsIAbDirectory aDirectory,
>+                                            in string aName,
>+                                            in AUTF8String uUTF8Value,
>+                                            in boolean aCaseInsensitive);

As Joshua said, please document these interfaces in the same style as their original versions.

>+
>+
>   PRBool findMailListbyUnicodeName(in wstring listName);

No need for two blank lines here, one will do fine.

>+NS_IMETHODIMP nsAbMDBDirectory::GetCardsFromProperty(const char *aProperty,
>+                                                     const nsACString &aValue,
>+                                                     PRBool caseSensitive,
>+                                                     nsISimpleEnumerator **result)
>+{
...
>+  nsresult rv;
>+  if (!mDatabase)
>+  {
>+    rv = GetAbDatabase();
>+    if (rv == NS_ERROR_FILE_NOT_FOUND)
>+      return NS_OK;
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }

nit: please put nsresult rv inside the if statement and assign the GetAbDatabase result straight to it on initialisation.

>+NS_IMETHODIMP nsAddrDatabase::GetCardsFromAttribute(nsIAbDirectory *aDirectory,
>+                                                    const char *aName,
>+                                                    const nsACString & aUTF8Value,
>+                                                    PRBool aCaseInsensitive,
>+                                                    nsISimpleEnumerator **cards)
>+{
...

>+  nsresult rv;
>+  nsCOMPtr<nsIMutableArray> list = do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);

Well you should check for success on creation. However, please drop these two lines and use nsCOMArray<nsIAbCard> instead - it saves the xpcom overhead as we aren't returning the array anywhere.

>+  while (!done) 

May as well make this a do...while loop as the condition would always be false on entering. Please also remove the extra whitespace from the end of the line.

>+  return list->Enumerate(cards);

It is better to use NS_NewArrayEnumerator from nsArrayEnumerator.h. This can be used with nsIArray or nsCOMArray.

>+  rowPos = -1;

Might as well do this initialisation where it is declared.

>-  m_mdbPabTable->GetTableRowCursor(m_mdbEnv, -1, getter_AddRefs(rowCursor));
>+  m_mdbPabTable->GetTableRowCursor(m_mdbEnv, rowPos , getter_AddRefs(rowCursor));

nit: no space before the comma.

>diff --git a/mailnews/addrbook/test/unit/data/cardForEmail.mab b/mailnews/addrbook/test/unit/data/cardForEmail.mab

This change breaks test_nsAbAutoCompleteSearch1.js, as test_nsAbAutoCompleteSearch1.js effectively borrowed the file, I suggest you hg copy it and do the modification on the original file. The unmodified copied file can then be used for test_nsAbAutoCompleteSearch1.js.

Comment 16

[u278084]

Attachment: nits fixed, tests pass

Thanks for the review Mark. I made the changes you suggested. Holding off on asking review until it's not 3am and I look at my code again.

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 391805 [details] [diff] [review]
nits fixed, tests pass

Sorry for the delay in getting back to this.

>+  /**
>+   * Returns all address book cards with a specific property matching value
...
>+   * @result                A nsISimpleEnumerator that holds nsIAbCard
>+   *                        instances. May be null under certain curcumstances
>+   */
>+  nsISimpleEnumerator getCardsFromProperty(in string aProperty,
>+                                           in AUTF8String aValue,
>+                                           in boolean aCaseSensitive);

nsISimpleEnumerator can't be null except in error conditions. As error conditions throw, then we don't need to document the null fact here.

r=Standard8 with that fixed.

Comment 18

[David :Bienvenu]

Comment on attachment 391805 [details] [diff] [review]
nits fixed, tests pass

quick nit - curcumstances should be circumstances.

Comment 19

[Mark Banner (:standard8)]

The Thunderbird drivers wish to release Thunderbird 3 as soon as possible. As a
result, we feel that this bug shouldn't stand in the way of all the other good
work getting into the hands of users sooner rather than later. Therefore we are
retargeting it for 3.1. See http://ccgi.standard8.plus.com/blog/archives/242
for more details. The 3.1 release is expected to be a quick release soon after
Thunderbird 3.

(Note: we'd still like to get this in for TB 3 especially as it has a draft patch - we just won't block on it).

Comment 20

[Mark Banner (:standard8)]

Attachment: Fixed comment.

Updated for my comment. I think I have a use case that I'd really like to get in for TB 3 and having this capability would make it a lot easier to implement.

Comment 21

[Mark Banner (:standard8)]

Attachment: OS X implementation

So implementing this I realise my use case can't use this, as it is an exact match rather than substring. However, given the patches are basically ready to go, I see no harm in taking these for TB 3.0.

Comment 22

[David :Bienvenu]

Comment on attachment 404573 [details] [diff] [review]
Fixed comment.

should be an nsISimpleEnumerator
+   * @result                  Returns an SimpleEnumerator of nsIAbCard instances

Comment 23

[David :Bienvenu]

Comment on attachment 404580 [details] [diff] [review]
OS X implementation

this looks OK, but test_abAutoCompleteSearch1.js failed on the mac with these patches, in the call to inputs.forEach(CheckInputSet), 8 == 6 failed.

r/sr=me, once the test is sorted out.

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 404573 [details] [diff] [review]
Fixed comment.

Patch for new API, possibly unused in core, so low risk. a=Standard8.

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 404580 [details] [diff] [review]
OS X implementation

Low risk patch to go with the first implementing a new API. Based on existing code.

Comment 26

[Mark Banner (:standard8)]

Both patches checked in:

attachment 404573 [details] [diff] [review]: http://hg.mozilla.org/comm-central/rev/7c474098ace7
attachment 404580 [details] [diff] [review]: http://hg.mozilla.org/comm-central/rev/dfcd4aab7972

Thanks to Cesar for doing the bulk of this work.