Closed
Bug 469209
Opened 16 years ago
Closed 3 years ago
Add labeled properties (multivalued attributes) to nsIAbCard
Categories
(MailNews Core :: Address Book, defect, P1)
MailNews Core
Address Book
Tracking
(Not tracked)
RESOLVED
FIXED
101 Branch
People
(Reporter: jcranmer, Assigned: darktrojan)
References
Details
Attachments
(4 files, 4 obsolete files)
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 :-)
Attachment #352610 -
Flags: superreview?(dmose)
Attachment #352610 -
Flags: review?(bugzilla)
|
Reporter | |
Comment 1•16 years ago
|
||
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.
Attachment #352610 -
Flags: superreview?(dmose)
Attachment #352610 -
Flags: review?(bugzilla)
|
|
Reporter | |
Comment 2•16 years ago
|
||
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.
Attachment #352610 -
Attachment is obsolete: true
Attachment #353155 -
Flags: superreview?(dmose)
Attachment #353155 -
Flags: review?(bugzilla)
|
||
Comment 3•16 years ago
|
||
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.
|
|
Reporter | |
Comment 4•16 years ago
|
||
(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•16 years ago
|
||
(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•16 years ago
|
||
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" : "test@invalid.net",
>+ "Work" : "test@invalid.com" },
>+ "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.
Attachment #353155 -
Flags: review?(bugzilla) → review-
|
|
Reporter | |
Comment 7•16 years ago
|
||
(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.
|
||
Updated•16 years ago
|
Attachment #353155 -
Flags: superreview?(dmose)
|
|
||
Comment 8•16 years ago
|
||
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.
|
|
||
Updated•16 years ago
|
Flags: wanted-thunderbird3+
Whiteboard: [needs new patch]
Target Milestone: Thunderbird 3.0b2 → ---
|
|
Reporter | |
Comment 9•16 years ago
|
||
This should fix all of the issues that Standard8 mentioned in review comments, including proper OOM checks (which is what took so long).
Attachment #353155 -
Attachment is obsolete: true
Attachment #371078 -
Flags: review?(bugzilla)
|
|
Reporter | |
Updated•16 years ago
|
Whiteboard: [needs new patch] → [needs review]
|
|
Reporter | |
Comment 10•16 years ago
|
||
I need to remember: it's nsCOMArray *, not nsCOMArray....
Attachment #371078 -
Attachment is obsolete: true
Attachment #371087 -
Flags: review?(bugzilla)
Attachment #371078 -
Flags: review?(bugzilla)
|
|
Reporter | |
Comment 11•13 years ago
|
||
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?
|
|
Reporter | |
Comment 12•13 years ago
|
||
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.
Attachment #371087 -
Attachment is obsolete: true
Attachment #545322 -
Flags: review?(mbanner)
Attachment #371087 -
Flags: review?(mbanner)
|
|
Reporter | |
Comment 13•13 years ago
|
||
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.
|
||
Updated•13 years ago
|
Summary: Add labeled properties to nsIAbCard → Add labeled properties (multivalued attributes) to nsIAbCard
|
||
Comment 14•12 years ago
|
||
standard8 ping review
|
|
||
Comment 15•12 years ago
|
||
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.
Attachment #545322 -
Flags: review?(mbanner)
|
|
Reporter | |
Updated•12 years ago
|
Assignee: Pidgeot18 → nobody
|
|
Reporter | |
Updated•12 years ago
|
Status: ASSIGNED → NEW
|
|
||
Comment 16•3 years ago
|
||
I guess we can use this bug for the backend full vCard support.
Assignee: nobody → geoff
Priority: -- → P1
|
Assignee | |
Comment 17•3 years ago
|
||
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
|
|
Assignee | |
Comment 18•3 years ago
|
||
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
|
||
Updated•3 years ago
|
Attachment #9266824 -
Attachment description: WIP: Bug 469209 - Create classes to handle vCard data and make it available from AddrBookCard. → Bug 469209 - Create classes to handle vCard data and make it available from AddrBookCard. r=mkmelin
|
|
Assignee | |
Updated•3 years ago
|
Depends on: AB-card-UI
|
|
||
Updated•3 years ago
|
Attachment #9266825 -
Attachment description: WIP: Bug 469209 - Implement address book cards as vCard. → Bug 469209 - Implement address book cards as vCard. r=mkmelin
|
|
Assignee | |
Comment 19•3 years ago
|
||
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.
Status: NEW → ASSIGNED
Keywords: leave-open
Whiteboard: [needs review]
Target Milestone: --- → 101 Branch
|
||
Comment 20•3 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/f721d4e43708
Create classes to handle vCard data and make it available from AddrBookCard. r=mkmelin
|
|
Assignee | |
Comment 21•3 years ago
|
||
|
|
||
Comment 22•3 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/7b22f4a42874
Implement address book cards as vCard. r=mkmelin
|
|
||
Updated•3 years ago
|
Attachment #9274912 -
Attachment description: WIP: Bug 469209 - Upgrade ABView classes to handle vCard properties. r=mkmelin → Bug 469209 - Upgrade ABView classes to handle vCard properties. r=mkmelin
|
|
Assignee | |
Updated•3 years ago
|
Keywords: leave-open → checkin-needed-tb
|
|
||
Comment 23•3 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/3883d4dbda20
Upgrade ABView classes to handle vCard properties. r=mkmelin
|
||
Comment 25•3 years ago
|
||
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.
|
|
Assignee | |
Comment 26•3 years ago
|
||
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•3 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•