[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 #5 from Olli Salli <ollisal at gmail.com> 2011-12-17 03:01:21 UTC ---
(In reply to comment #4)
> (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.

/usr/include/dbus-1.0 is not /usr/include, though. Actually I think that
might've been because some other library pkgconfig (telepathy-glib I guess?)
pulled in the correct include path as a dependency or something. Dunno, anyway
nice that it's now fixed.

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

Yeah, good now. Weird that the dbus-glib-1 pkgconfig file doesn't pull in this
arch deps header correctly. Could be the distro packaging which is a bit faulty
here - anyway, your new cmake lookup rules should find it in any case.

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

++ for now, until PendingContacts is otherwise refactored some time in the
future. Could you add a fixme to do that please? I believe we can then
rearrange the code so that actually the total number of lines goes up slightly,
but the complexity goes down severely and it's much easier to follow and make
changes in isolation, including sharing the parts of code like this which can
be shared.

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

Well. Frankly put, you shouldn't duplicate crap just to be consistently crappy.
We've tried to weed zero-information "see also" things out as much as possible,
e.g. making the references in TextChannel reasonable as part of the last big
doc cleanup you admirably carried out.

The important point here is that *there are no more details regarding the
feature in the accessor documentation* so you can't "see" it to get more
details on the feature. For some other features, there is, though yeah, some of
them are just as wrong as this one which has now been written. But let's not
worry about cleaning up them now, though still, let's not add MORE misleading
references.

Anyway, do whatever you please with the other accessors, but let's not make
these newly written docs misleading just for consistency. Merge after proto
addressing branch is merged (just now last pass -reviewed it), and cleaning
this thing up one way or another, please.

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