469209 - Add labeled properties (multivalued attributes) to nsIAbCard

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect, P1)

Description

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 1.0

This is step 1 of bug 118665 comment 48.

The attached patch adds support for labeled values in both nsAbCardProperty and nsAddrDatabase (i.e., Mozilla address books), but does not make any of our properties labeled values (it's large enough as it is). It also adds invaluable tests for the features.

I'm splitting the work off from bug 118665 to keep bug spam down and to avoid excitement over a step most users won't notice :-)

Comment 1

[Joshua Cranmer [:jcranmer]]

Comment on attachment 352610 [details] [diff] [review]
Patch, version 1.0

In trying to build off of this patch, I realized that the approach I have for the mork address book implementation is probably going to bad for an important piece: nsIAbCollection::getCardForProperty.

Canceling review.

Comment 2

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 2.0

This will make things go much smoother for step 2.

I fixed some memory links in the first patch (by forgetting to release + free the arrays).

I also revamped the schema. It's truly a mork-specific hack: the multivalued properties (and only those properties) are stored in a separate tables, which contain but three columns per row: CardId=<card id>, Label=<label name>, <property>=<value>. Storing it like that means I can easily implement GetCardFromAttribute in a multivalued-respectful way with mork thanks to rowspace and table and API magic.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 353155 [details] [diff] [review]
Patch, version 2.0

I need to look at this in more detail yet, but here's some quickish comments:

>+ *
>+ * Although the value is presented here as an nsIVariant, the type MUST be
>+ * convertible to a string for full interoperability. The card itself does not
>+ * perform any checks of this requirement, but its violation will lead to
>+ * unexpected failures or incorrect results on other functions.

So why don't we save ourselves the nsIVariant overhead and force it to be a string?


>+  nsIVariant getPropertyWithLabel(in AUTF8String name, in AUTF8String label);
>+  /**

nit: insert a blank line after functions please (several places).

>+  /**
>+   * Make the given label the preferred label for the property.
>+   *
>+   * If there is already a preferred label, the new label takes precedence.
>+   *
>+   * @param name             The case-sensitive name of the property to set.
>+   * @param label            The case-sensitive label of the property value.
>+   * @exception NS_ERROR_NOT_AVAILABLE if either the property or the label do
>+   *                                   not exist.
>+   */
>+  void setPreferredValue(in AUTF8String name, in AUTF8String label);

As long as no-one can see us needing to use a specific order for labelled elements, I think this is fine.

>+NS_IMETHODIMP
>+nsAbCardValue::GetValue(nsIVariant **aValue)
>+{
>+  NS_ADDREF(*aValue = value);

You should check for a null aValue, and it should be NS_IF_ADDREF because value could in theory be null.

It looks like the null-argument checks have been missed in a few places.

>+class nsAbCardValue: public nsIAbCardValue
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIABCARDVALUE
>+
>+  nsAbCardValue(const nsACString &aLabel, nsIVariant *aValue);
>+  nsAbCardValue(nsIAbCardValue *property);
>+  nsCString label;
>+  nsCOMPtr<nsIVariant> value;
>+};

Please use mLabel and mValue, its much clearer that they are member variables.

Comment 4

[Joshua Cranmer [:jcranmer]]

(In reply to comment #3)
> So why don't we save ourselves the nsIVariant overhead and force it to be a
> string?

This allows people to store things closer to their type, e.g., an SQL address book can store the integer properties as SQL integers and string properties as SQL strings.

> As long as no-one can see us needing to use a specific order for labelled
> elements, I think this is fine.

The ordering is generally stable; I could make SetPreferredValue remove and then add the value instead of swapping them, if people so choose.

> You should check for a null aValue, and it should be NS_IF_ADDREF because value
> could in theory be null.

Oops.

> It looks like the null-argument checks have been missed in a few places.

I've been trying to insure that a null nsIVariant* doesn't flop around internally, but I may have missed some.

> Please use mLabel and mValue, its much clearer that they are member variables.

This was originally a simple struct, and then I added the interface. I forgot to change the variable names, my bad.

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> (In reply to comment #3)
> > So why don't we save ourselves the nsIVariant overhead and force it to be a
> > string?
> 
> This allows people to store things closer to their type, e.g., an SQL address
> book can store the integer properties as SQL integers and string properties as
> SQL strings.

Then I don't understand why they need to be convertible to a string. Even objects can be converted to strings these days via json.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 353155 [details] [diff] [review]
Patch, version 2.0


>+    // But if we have a tag, we need an array of (tag, value) tuples. nsIVariant
>+    // wants for nsISupports interfaces an array of void*, which is what
>+    // nsCOMArray more or less uses. But it doesn't give us access. So we need
>+    // to copy the array (to have it copied *again*). At least we don't need to
>+    // AddRef.
>+    nsIAbCardValue **insertArray = new nsIAbCardValue *[data->Count()];

nit: you should be checking this (and other places) for alloc failure.

>+    for (PRInt32 i = 0; i < data->Count(); i++)
>+      insertArray[i] = data->ObjectAt(i);
>+    nsCOMPtr<nsIWritableVariant> writable =
>+      do_CreateInstance(NS_VARIANT_CONTRACTID);
>+    writable->SetAsArray(nsIDataType::VTYPE_INTERFACE_IS,
>+                         &NS_GET_IID(nsIAbCardValue), (PRUint32)data->Count(),
>+                         static_cast<void *>(insertArray));

SetAsArray does a clone of the array, therefore you need to free the memory you've used.

> NS_IMETHODIMP nsAbCardProperty::GetProperty(const nsACString &name,
>                                             nsIVariant *defaultValue,
>                                             nsIVariant **value)
> {
>-  if (!m_properties.Get(name, value))
>+  nsCOMArray<nsAbCardValue> *values;
>+  if (!m_properties.Get(name, &values))
>     NS_ADDREF(*value = defaultValue);

You haven't null-checked or NS_IF_ADDREF for defaultValue

>+  else
>+    NS_ADDREF(*value = (*values)[0]->value);

Are we guaranteed this is non-null? (have we ensured all elements we add are non-null).


>+  for (PRInt32 i = 0; i < properties->Count(); i++)

nit: please make sure we're not going into Count() each time round the loop.

>+        nsIAbCardValue **data;
>+        PRUint32 length;
>+        variant->GetAsArray(&type, nsnull, &length,
>+                          reinterpret_cast<void **>(&data));
>+
>+        NS_ConvertUTF16toUTF8 propertyName(name);
>+        mdb_column propColumn;
>+        rv = m_mdbStore->StringToToken(m_mdbEnv, propertyName.get(), &propColumn);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+
>+        mdbOid propertyRowOid = { m_CardPropRowScopeToken, -1 };
>+        for (PRUint32 i = 0; i < length; i++)
>+        {
>+          nsIMdbRow *propertyRow;
>+          // Believe it or not, propertyRowOid won't be set by the method. So we
>+          // can save a few cycles and not reset the id to -1 each cal.
>+          rv = cardMultTable->NewRow(m_mdbEnv, &propertyRowOid, &propertyRow);
>+          NS_ENSURE_SUCCESS(rv, rv);
>+
>+          rv = AddIntColumn(propertyRow, rowIdColumn, rowOid.mOid_Id);
>+          NS_ENSURE_SUCCESS(rv, rv);
>+
>+          nsCAutoString value;
>+          data[i]->GetLabel(value);
>+          rv = AddCharStringColumn(propertyRow, labelColumn, value.get());
>+          NS_ENSURE_SUCCESS(rv, rv);
>+
>+          data[i]->GetValue(getter_AddRefs(variant));
>+          variant->GetAsAUTF8String(value);
>+          rv = AddCharStringColumn(propertyRow, propColumn, value.get());
>+          NS_ENSURE_SUCCESS(rv, rv);
>+        }
>+
>+        NS_FREE_XPCOM_ISUPPORTS_POINTER_ARRAY(length, data);

There seems to be quite a few failure cases here where we could exit without 

>+
>+        if (columnNumber == rowIdColumn)
>+        {
>+          // The row ID is redundant here. If it's not right, we've got some
>+          // corruption going on.

Make this an assertion?

>+        {
>+          // If we have multiple columns, there's some bad corruption. The best
>+          // thing to do is to just bail.

With an assertion so that we can track/fix it?

>+      nsCOMPtr<nsIWritableVariant> storage =
>+        do_CreateInstance(NS_VARIANT_CONTRACTID);
>+      storage->SetAsAString(value);
>+
>+      newCard->SetPropertyWithLabel(property, label, storage);
>+
>+      char *bar;
>+      storage->GetAsString(&bar);

This doesn't seem to be used/freed.

>+var correctCard = {
>+  "Email" : { "Home" : "[e-mail]",
>+              "Work" : "[e-mail]" },
>+  "AllowRemoteContent" : false,
>+  "PopularityIndex" : 0,
>+  "PreferMailFormat" : 0,
>+  "LastModifiedDate" : 0
>+};
>+
>+function check_card(valueObject, card) {
>+  var enumerator = card.properties;
>+  while (enumerator.hasMoreElements()) {
>+    check_equivalence(valueObject, enumerator.getNext().QueryInterface(
>+      Components.interfaces.nsIProperty));
>+  }

If card.properties has less properties than correctCard, then this function won't detect it.

There's also comment 5 to address/respond to.

Comment 7

[Joshua Cranmer [:jcranmer]]

(In reply to comment #5)
> Then I don't understand why they need to be convertible to a string. Even
> objects can be converted to strings these days via json.

nsIVariant can't convert objects to strings, though, which is what matters. The documentation is trying to say that a call to nsIVariant::GetAsAString MUST NOT fail.

(In reply to comment #6)
> (From update of attachment 353155 [details] [diff] [review])
> There seems to be quite a few failure cases here where we could exit without 

This got cut off, but I understand what you're trying to say. *Mutters about C++ not having a finally block*

> >+      char *bar;
> >+      storage->GetAsString(&bar);
> 
> This doesn't seem to be used/freed.

Whoops, that was stuff I stuck in for debugging at some point. Will fix.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 353155 [details] [diff] [review]
Patch, version 2.0

Removing sr request as per discussion with jcranmer.  We've figured out the allocation strategy (go back to the current standard of checking for allocation failures), r and sr to be re-requested upon new patch iteration.

Comment 9

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 3.0

This should fix all of the issues that Standard8 mentioned in review comments, including proper OOM checks (which is what took so long).

Comment 10

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 3.1

I need to remember: it's nsCOMArray *, not nsCOMArray....

Comment 11

[Joshua Cranmer [:jcranmer]]

I've updated the patch to relatively recent tip (after mconley's big AB patches landed), and have currently pushed it to trunk. ::operator new should be infallible now, so is it worth cleaning up OOM handling?

Comment 12

[Joshua Cranmer [:jcranmer]]

Attachment: Patch, version 3.2

Alas, I no longer have a patch that I can prove has an outstanding review request for 2 years. On the other hand, this patch applies, compiles, and passes try server with no leaks. So maybe this will be reviewed more quickly.

Comment 13

[Joshua Cranmer [:jcranmer]]

I should also add: I think this patch should make it into the next central->aurora merge (2011-08-16, according to wiki.m.o), so if there are major issues, those should be brought up soonish.

Comment 14

[Ludovic Hirlimann [:Usul]]

standard8 ping review

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 545322 [details] [diff] [review]
Patch, version 3.2

Sorry for never really having got to look at this. I think my issue with it was that it seemed too complicated, especially with the nsAbCardProperty changes, and switching that to a javascript implementation first may have been better and a lot simpler.

That said, there is now an effort to work on a new address book here:

https://github.com/mikeconley/thunderbird-ensemble

So I'm cancelling this review as that could replace the work necessary for this bug. If you do want to update the patch (e.g. in the case of that not progressing at the right rate), then please feel free to update and we'll see if we can find someone to review it in a more timely fashion. I would though, also strongly consider the additional possibility of switching the file to js.

Comment 16

[Magnus Melin [:mkmelin]]

I guess we can use this bug for the backend full vCard support.

Comment 17

[Geoff Lankow (:darktrojan)]

Attachment: Bug 469209 - Create classes to handle vCard data and make it available from AddrBookCard. r=mkmelin

There are two new classes: VCardProperties which represents an entire vCard, and VCardPropertyEntry which represents a single entry in a vCard.
These are based around the data structures returned by ICAL.parse and have methods for manipulating the data, then returning it to ICAL.stringify.

Two new properties are added to nsIAbCard, a boolean showing support for vCard (or lack thereof), and an instance of VCardProperties.

Depends on D139897

Comment 18

[Geoff Lankow (:darktrojan)]

Attachment: Bug 469209 - Implement address book cards as vCard. r=mkmelin

This adds the actual support for vCard to AddrBookCard. The vCard data for a card is stored as a string in the _vCard property, as is already the case for CardDAV cards.

To avoid parsing the entire vCard unnecessarily, common properties are also stored in the database and used where possible. These properties can no longer be set directly on the card to prevent problems.

Depends on D140510

Comment 19

[Geoff Lankow (:darktrojan)]

I'm going to land part one now that we've started a new version. It shouldn't really change anything, but it does mean further development can go ahead without a full build.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f721d4e43708
Create classes to handle vCard data and make it available from AddrBookCard. r=mkmelin

Comment 21

[Geoff Lankow (:darktrojan)]

Attachment: Bug 469209 - Upgrade ABView classes to handle vCard properties. r=mkmelin

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7b22f4a42874
Implement address book cards as vCard. r=mkmelin

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3883d4dbda20
Upgrade ABView classes to handle vCard properties. r=mkmelin

Comment 25

[Mitra Ardron]

I see that #118665 has been marked as a duplicate of this, will this fix actually fix that bug (not allowing more than 2 email addresses per card).

In fact, for the OSX Address Book integration it currently only shows one email address, and no other information at all, and doesn't recognize any address but that first email address in auto-completion. Should that be posted as a new bug ?

Seriously - the Thunderbird Address Book is next to useless as it currently stands, I have to cut-and-paste email address from the OSX Address Book into emails most of the time.

Comment 26

[Geoff Lankow (:darktrojan)]

This will allow more than two addresses per card but won't fix the macOS Address Book. Yes, you should file a new bug. There may already be one, check first.

Comment 27

[Mitra Ardron]

I guess it was too much to hope that it would fix a 21 year old bug# 118665 :-(

I've filed bug# 1770851 which actually documents regressions with OSX Address Book (its gone down from 2 addresses to 1), though until bug# 1720257 is fixed OSX Address Books are unavailable to all but geeks who can do the hack documented there.