[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

--- Comment #4 from Andre Moreira Magalhaes <andrunko at gmail.com> 2011-12-15 16:45:34 PST ---
(In reply to comment #3)
> 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: 

It was working before cause most distros install dbus-glib in /usr/include, so
even with pkg_check_modules failing it was getting the headers from
/usr/include/dbus-1.0/*
But now it properly uses pkg-config for tips on where to find the headers, as
it should.

> > 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.

Most dbus-glib headers are installed in /usr/include/dbus-1.0/dbus here
but the dbus-arch-deps.h header is installed in
/usr/lib64/dbus-1.0/include/dbus
The issue is that we need to use dbus_g_method_get_sender which is defined in
dbus-lowlevel.h that includes dbus-arch-deps.h.
We didn't need it before cause we don't include dbus-lowlevel.h anywhere else,
just in this new test CM I wrote.

But as agreed over IRC I merged both variables into one DBUS_GLIB_INCLUDE_DIRS
(plural).

> > 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.
Yeah, I wrote it myself heh, renamed to use TpTests, etc as namespace

> +    // 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?
Done.

> @@ -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?
The code here is similar but has some few differences, so no change here.

> + *
> + * 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.
Yes, I agree the "See ..." line is vague, but it's used everywhere else, even
in all other contact features docs. So for keeping consistency I didn't change
this.

> + * 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.
Done.

> + * address in Contact::*() that can identify it.
> 
> -> "address that can identify it in Contact::*"
Done

> 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.
Yeah, we need to do that someday, the code is getting bigger and bigger and
difficult to maintain. Added mental note.

I rebased the branch again on top of proto-addressing as I did some changes
there. The last review commit is d9eaf0.

-- 
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.