[Bug 43599] Add high-level API for Conn.I.Addressing interface

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=43599

Olli Salli <ollisal at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|                            |review+

--- Comment #3 from Olli Salli <ollisal at gmail.com> 2011-12-15 08:16:31 PST ---
Mmm, I wonder

> cmake: Fix FindDBusGLib.cmake to properly search for dbus-glib-1 instead of dbus-glib.

How did this work before? Didn't we use it at all or something? Or is there now
some chance that an installation where it worked before, is now broken?cmake: 

> Also export DBUS_GLIB_LOWLEVEL_INCLUDE_DIR so one can include <dbus/dbus-glib-lowlevel.h>.

Uhm, usually library include dir variables are per-library, not per-header. Is
there some chance that they aren't in the same directory? If not, we don't do
QT_QDBUS_ARGUMENT_INCLUDE_DIR, QT_QDBUS_CONNECTION_INCLUDE_DIR etc either so I
don't see why we should do this.

> future/conn-addressing: Add test glib connection implementing Conn.I.Addressing.

Did you write this yourself, such that it's not a tp-glib example CM (which it
doesn't look like, because it depends on TpTests stuff?). If so, please rename
to TESTS_... like the other tests-only bits.

Note that some of our test CMs are named differently specifically because
they're (derived from or directly) tp-glib example (parts of documentation for
the library) CMs.

+    // FeatureAvatarData depends on FeatureAvatarToken
+    if (realFeatures.contains(Contact::FeatureAvatarData) &&
+        !realFeatures.contains(Contact::FeatureAvatarToken)) {

.... and the lines following it, are duplicated between the various
ContactManager contact requesting functions. Utility fn taking features, making
the real features out of them and acting (ensuring tracking and returning
ifaces) accordingly?

@@ -565,10 +570,22 @@ void
PendingContacts::onAddressingGetContactsFinished(PendingOperation *operatio

This code extracting the attributes and the referenced handles etc also looks
awfully familiar. Could you please refactor it so that it shares the code with
the codepath I originally wrote that in, rather than copypasting?

+ *
+ * See addressesable addresses specific methods' documentation for more
details.
+ *
+ * \sa addressableVCardAddresses(), addressableUris()

The "See..." line is rather vague, but also redundant with the semantical \sa
line, so I'd just remove it. The actual \sa's in this commit look very good in
general though.

+ * Requesting contacts using ContactManager::contactsForVCardAddresses()
+ * by passing one of the various vCard addresses returned here
+ * will succeed and include this contact in the returned PendingContacts.

I don't think that's the point. If you already have the Contact, why would you
still request it? These accessors are more so used to link the contacts by
addresses in some other system, aren't they? So you could document in the
ContactManager request functions that the returned contacts will likely have
matching addresses, but using this information in the other way around makes
less sense to me.

You can preserve the \sa here though - generally, I don't want \sa's which are
redundant with autolinks in the full text description, but as that description
doesn't make sense here, as much as in ContactManager, I think you should move
the description away from here (completely, or merging with related
ContactManager docs), but keep the \sa which is good.

+ * address in Contact::*() that can identify it.

-> "address that can identify it in Contact::*"

Otherwise looks good. We should make a mental note, however, of splitting
PendingContacts to a set of internal sub-classes, with one for each (or a few
related) different kinds of requests - but sharing most of the looking up
cached contacts etc code, and that signaling the results. These huge if-else
mazes for "which type of an object I REALLY am this time around" would
disappear with that. But don't need to do that now, as long as the current code
works and has reasonable test coverage.

Fine to merge after considering these notes - but of course, proto.i.addressing
branch is now blocking this. Will finish reviewing that next.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA Contact for the bug.
You are the assignee for the bug.