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

RESOLVED FIXED in Thunderbird 3.0rc1

Status

MailNews Core
Address Book
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: standard8, Assigned: cesar)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 3.0rc1
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 -
blocking-thunderbird3.1 +
wanted-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
(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 :-|
(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.
Summary: nsIAbDirectory::getCardFromProperty and nsIAbDirectory::getCardFromProperty should be able to return multiple cards → nsIAbDirectory::getCardFromProperty and nsIAbDirectory::cardForEmailAddress should be able to return multiple cards
Blocks: 456011

Comment 3

10 years ago
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.
Assignee: nobody → bugzilla
Flags: blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0rc1
Whiteboard: [needs owner][no l10n impact]
Summary: nsIAbDirectory::getCardFromProperty and nsIAbDirectory::cardForEmailAddress should be able to return multiple cards → nsIAbCollection::getCardFromProperty and nsIAbCollection::cardForEmailAddress should be able to return multiple cards
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?
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

9 years ago
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.
Blocks: 447927
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.
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.
Assignee: bugzilla → nobody
Keywords: helpwanted
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.
(Assignee)

Comment 10

9 years ago
Sounds good. I'll take the bug.
Assignee: nobody → cdolivei.bugzilla
Status: NEW → ASSIGNED
(Assignee)

Comment 11

9 years ago
Created attachment 383412 [details] [diff] [review]
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.
(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.
(Assignee)

Comment 13

9 years ago
Created attachment 386369 [details] [diff] [review]
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!
Attachment #383412 - Attachment is obsolete: true
Attachment #386369 - Flags: review?(bugzilla)
One little nit: could you add documentation to the IDL functions?
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.
Attachment #386369 - Flags: review?(bugzilla) → review-
Whiteboard: [needs owner][no l10n impact] → [needs updated patch][no l10n impact]
(Assignee)

Comment 16

9 years ago
Created attachment 391805 [details] [diff] [review]
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.
Attachment #386369 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #391805 - Flags: review?(bugzilla)
Attachment #391805 - Flags: review?(bugzilla) → review+
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

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

quick nit - curcumstances should be circumstances.
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).
Flags: blocking-thunderbird3.1+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Flags: wanted-thunderbird3?
Created attachment 404573 [details] [diff] [review]
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.
Attachment #391805 - Attachment is obsolete: true
Attachment #404573 - Flags: superreview?(bienvenu)
Attachment #404573 - Flags: review+
Created attachment 404580 [details] [diff] [review]
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.
Attachment #404580 - Flags: superreview?(bienvenu)
Attachment #404580 - Flags: review?(bienvenu)
No longer blocks: 447927

Comment 22

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

should be an nsISimpleEnumerator
+   * @result                  Returns an SimpleEnumerator of nsIAbCard instances
Attachment #404573 - Flags: superreview?(bienvenu) → superreview+

Comment 23

9 years ago
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.
Attachment #404580 - Flags: superreview?(bienvenu)
Attachment #404580 - Flags: superreview+
Attachment #404580 - Flags: review?(bienvenu)
Attachment #404580 - Flags: review+
Comment on attachment 404573 [details] [diff] [review]
Fixed comment.

Patch for new API, possibly unused in core, so low risk. a=Standard8.
Attachment #404573 - Flags: approval-thunderbird3+
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.
Attachment #404580 - Flags: approval-thunderbird3+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: wanted-thunderbird3? → wanted-thunderbird3+
Resolution: --- → FIXED
Whiteboard: [needs updated patch][no l10n impact]
Keywords: helpwanted
You need to log in before you can comment on or make changes to this bug.