Integrating TP-Logger with KDE

[George Kiagiadakis]

On Wed, Feb 23, 2011 at 11:18 PM, Stefano Sanfilippo
<stefano.k.sanfilippo at gmail.com> wrote:
> Since Reviewboard is not appliable to the sketch repo I'm working into, here
> is the direct web link:
>
> http://quickgit.kde.org/?p=scratch%2Fsanfilippo%2Ftelepathy-logger-
> query.git&a=summary
>
> It's still a proof of concept (I'm studying empathy code, which, to some
> extent, is not documented) and is not working yet, but shows the general
> mechanism of the thing. Reviews and (not too harsh) criticism is welcomed.

I understand this is still a proof-of-concept, but I'll write some
comments here for you to have in mind while you keep developing.

* You should split the classes in multiple files and separate the
public API from the private internal API. This will also help review,
since putting them all together in one file makes it difficult to
visualize the components in the mind.

* A Qt-style public API should never include headers from the
underlying library that provides the implementation. What I mean here
is that supposing query-log.h is a public header, it should NOT
include <telepathy-logger/*.h> or other glib headers for any reason.

* Have a look at the kdelibs coding style guidelines and try to adapt
your code to them.

* Use const references for function and signal arguments. i.e. instead
of "foo(QString a)", write "foo(const QString & a)". For all
non-native types.

* For public API, use private classes to store the class members.

* Do not throw pointers of objects that are allocated on the heap.
i.e. instead of "throw new Error(foo);" write "throw Error(foo);" and
catch with a const reference: "catch (const Error & e)". Btw, you
might want to use QtGLib here to avoid doing such stuff yourself and
in the wrong way. class Error could be completely replaced with
QGlib::Error for example.

* Throwing exceptions inside C callbacks is a terribly bad idea imho.
To catch exceptions, the calling code needs to have a stack compiled
with exception support (afaik), and C libraries definitely do not do
that. Most likely, the code will crash. Those callbacks are called
from the event loop anyway, so if you throw exceptions, they will be
delivered to the event loop and Qt doesn't handle exceptions very well
in there.

* Speaking of an event loop... you don't seem to have one in your
main(). I am sure this is why it is not working ;) I guess you haven't
understood how this async model works.

* I see no calls to g_object_unref, although you are using GObjects
extensively. This is certainly a memory leak. If you decide not to use
QGlib::Object to wrap them, you MUST unref them when you are done
using them.

* Your code MUST have a copyright & license, otherwise other people
cannot use it without breaking copyright law.

> In the template-based branch, I've put a template callback function - reducing
> of some 100 lines the code - which compiles fine, but gives linker errors. I'd
> be grateful to anyone who could point me in the right direction - the people
> on ##c++ were sporting an aggravating strictness on answering my question :(
> They required a test case I did not manage to create... quite unfriendly. I
> know it's slightly OT, but I don't really know where to seek help.

--- [snip] ---
static const gcb datesforconvcb = (gcb)
finishedcb<ConversationDatesQuery, GDate, QGDate,
tpl_log_manager_get_dates_finish>;
---------------

Strange that this thing compiles. Try:

static const gcb *datesforconvcb = &finishedcb<ConversationDatesQuery,
GDate, QGDate, tpl_log_manager_get_dates_finish>;


Regards,
George