[Bug 27178] don't install private headers, and privatize most of the API/ABI

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=27178





--- Comment #5 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2010-03-23 09:49:47 PST ---
> Enabling -Werror by default in configure.au

Change looks good, although I think you mean ".ac". At some point I'd like to
scavenge the compiler flag infrastructure from telepathy-glib/telepathy-qt4 and
use that instead (it's rather more systematic about compiler warnings).

As general points about commit messages: please be as careful when writing them
as when writing the code, and it's conventional to phrase the subject (first
line) like "enable -Werror [...]" (as opposed to "enabling -Werror" or "enabled
-Werror"). It may help to imagine that the subject gets "when you apply this
patch, it will ..." prepended to it.

> Updated TplConf as GInterfaces (formerly GObject)

I'd really prefer to skip this for now, get the small changes in first, and
come back to it. Likewise for "Using the new interface and implementation in
TplObserver", "Using TplConf signaling in TplObserver".

> channel.c: typo prefixing tpl_observer instead of tpl_channel

This looks good, although it also removes a useless finalize() which isn't
mentioned in the commit message; ideally that'd either be a separate commit, or
failing that, mentioned as "Also remove a useless finalize() implementation" in
the long commit message.

> TP style fixes

These look good, or at least, harmless. I don't like the GET_PRIV() thing, but
we can look at that project-wide later. I'd have preferred a separate commit
per class.

> Tests updated and debug improved

This contains three unrelated changes - three commits, please.

tpl_observer_observe_channels: the improved debug here can't work, because
you're checking for error != NULL *after* calling g_clear_error on it, so
you'll always claim that there was no error. So, this isn't right.

tpl_observer_register_channel: this is neither a test update nor an improvement
to debug, so it should have certainly had its own commit. It's also wrong: as
written, you'll leak @key (priv->channel_map has no key destructor).

The use of the object path is actually nearly safe, because the *value* in
priv->channel_map is reffed, and owns the object path (it's valid as long as
the TplChannel is); all you need to do to make it *actually* safe is to use
g_hash_table_replace() instead of g_hash_table_insert(). That's subtle enough
that it deserves a comment, however.

Alternatively, you could change the g_hash_table_new_full() call so it provides
g_free as the value destructor, and keep the g_strdup.

test-tpl-conf.c seems to be part of the TplConf changes, which I'd like to put
to one side anyway, so I won't review it here.

> TplChanenlText: make pendingproc_cleanup_pending_messages_db using tpl_channel_text_clean_up_stale_tokens

ITYM "TplChannelText" and "use".

The code changes look good but I think it would be worth saying explicitly, in
the comment above tpl_channel_text_clean_up_stale_tokens, that it destroys
@stale_tokens and frees its contents.

> 'make check' clean

Looks good to me, although the commit message is vague, one commit per class
would be better, and having each change squashed into the commit that caused
the problem (if they're not already in master) would be better still. The part
in log-store-sqlite.h will conflict with the cleanup changes I just pushed; I
prefer the version I pushed, since it avoids over-long lines.

> Checking NULL before unrefereing TplLogEntry

Redundant with a change I pushed; the commit message would have been better if
it gave some indication where the change was, and was spelled correctly :-)

> Removed useless #includes

Looks good.

> Moving tpl_log_manager_register_log_store to log-store-priv

Please be as precise and correct in commit messages as in code. You moved it to
log-manager-priv (saying "the private header" would also have been fine).

> Moving TplLogMessageFilter to log-manager.h so that log-store.h does not need to be public

Looks good.

> Removed account, presence status/msg methods from TplContact

It would be nice if the commit message explained why (I assume the answer is
"nothing uses this API"?), but in general we like code deletion :-)


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