Tpl currently installs approximately all of its headers. It should only install those intended for consumption by library users.
This requires working out what is private...
Danni, Cosimo: initially, could you let me know which headers are needed by third-party code, and which ones can be private? I'll concentrate my API review on the ones that are needed by third-party code.
Eventually, we should only install (and only export symbols from) the headers that have had an API review, and close this bug.
Refer to http://git.collabora.co.uk/?p=user/kalfa/telepathy-logger.git;a=shortlog;h=refs/heads/api-cleanup
The branches on which this branch builds are still experimental, but too much has been added in the last days which master doesn't have.
Public headers (due to being public API or their deps):
Modules that have symbols used only internally (ie among them):
All the rest are in the middle, internally used modules, as log-store.c which is never directly accessed by any client, but whose symbols the log-manager needs to see.
Here a list of the modules in the middle with their deps:
log-manager -> log-entry, log-store, log-store-xml, log-store-sqlite, datetime, util
log-store-xml -> contact, log-entry-text, log-manager, log-store, datetime, util
log-store-sqlite -> log-entry-text, datetime, util, tpl-marshal
log-entry-text -> util, log-entry
log-entry -> contact, util
conf-gconf -> util, conf
util -> log-store-sqlite
conf -> None
datetime -> None
contact -> None
tpl-marshal -> None
log-store -> None
I'm going to write a comment for each module deserving one
It has the same function of the former EmpathyContact, it needs a cleanup
for the presence methods, which are unused and thus to be removed.
Also get/set_contact might be removed.
contact_type is used by empathy, but since TpContact is never used by anything else but a User, is quite useless and probably in the removal list.
It's all in the TODO list as soon as we have properly working features.
the naming follows the ChannelType.
There are several methods duplicated, my intention is to drop most of them and keep only
tpl_log_entry_<method> for what is implemented in TplLogEntry and
tpl_log_entry_text_<method> for what is implemented in TplLogEntryText
dropping things similar to:
const gchar* (*get_log_id) (TplLogEntry *self);
in TplLogEntryClass and the related
const gchar *tpl_log_entry_text_get_log_id (TplLogEntryText *self);
which create only confusion and keeping only
const gchar* tpl_log_entry_get_log_id (TplLogEntry *self);
Unless in tp-glib is done differently.
> 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
> 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
> 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 :-)
(In reply to comment #2)
> Modules that have symbols used only internally (ie among them):
It seems that the daemon uses these, so they need to be visible to it as well as within the library, but (e.g.) Empathy and Moblin don't need these headers or the symbols they export. Am I understanding correctly?
(In reply to comment #3)
> It has the same function of the former EmpathyContact, it needs a cleanup
> for the presence methods, which are unused and thus to be removed.
> Also get/set_contact might be removed.
I think this sort of API review is good, but outside the scope of this bug. I'll file a bug for each public module and copy your comments there.
*** Bug 27109 has been marked as a duplicate of this bug. ***
> It seems that the daemon uses these, so they need to be visible to it as well
as within the library, but (e.g.) Empathy and Moblin don't need these headers
or the symbols they export. Am I understanding correctly?
Following the chat about symbols we had a week ago, I pointed them out in order to clarify what might be split into a convenience lib to be used only by the service, and what in the official shared one.
Substantially this set (the same you quoted)
contains symbols that are never accessed outside the set itself and main(), which can statically link the convenience library, including them all, and dynamically link the official library.
This is true if we exclude tests/ which I don't think is a problem.
Clients will link only the official library.
This to avoid most of the non public symbols to be appear in the official library.
(In reply to comment #9)
> Substantially this set (the same you quoted)
> contains symbols that are never accessed outside the set itself and main(),
> which can statically link the convenience library, including them all, and
> dynamically link the official library.
It's not quite that simple, because having two copies of the same code in the same process isn't going to work well; there's one in the convenience library, and another in the shared library. There are a couple of ways this can go:
1) Link the daemon against the convenience library, not the static library (dbus-daemon and friends link libdbus in this way). This would break third-party plugins into the daemon (if we want that possibility?), if those plugins linked the shared library.
2) Make libtelepathy-logger a minimal library for clients. Move the bits that only the daemon needs into src/, and have clients, the daemon, and any future plugins all link against the shared library. This would mean that any symbols needed by both src/ and the internals of the library would have to be exported. (Mission Control works somewhat like this: libmcclient is a small client library, which also contains code used by both clients and the daemon.)
This can of worms gets nastier if we have GPL vs. LGPL considerations, and want to make the library LGPL-only for maximum applicability. Is it possible to restrict the GPL (Empathy-derived) code to src/ and have libtelepathy-logger be LGPL, or is there some LGPL code that's definitely needed by clients of the logger?
> This is true if we exclude tests/ which I don't think is a problem.
Tests can always be linked against the convenience library directly if necessary - a couple of tests in telepathy-glib do that, to gain access to hidden internal symbols.
For the record, here are the API used by Empathy:
What about just make this API public and keep the rest private for now?
This branch move most of the API as internal:
(In reply to comment #12)
> This branch move most of the API as internal:
r+ but please keep this bug open; all non-public symbols should ideally be underscored.
This is the remaning ABI with this branch:
Should we keep the generated API? I guess the client side one makes sense (Empathy currently generate it but could use this one instead) but probably not the server side API.
(In reply to comment #15)
I think we can close this bug when you've merged that branch. Thanks!
> This is the remaning ABI with this branch:
Re-ordered by "group":
> Should we keep the generated API? I guess the client side one makes sense
> (Empathy currently generate it but could use this one instead) but probably not
> the server side API.
If the D-Bus API changes in ways that would break the service API, they'd break the client API too, so I think it's OK to keep all of these; just be careful about D-Bus API breaks (add + deprecate until the next library SONAME bump, rather than removing) after we've announced the library to have ABI guarantees.
Under discussion on Bug #27269.
Under discussion on Bug #27271.
Under discussion on Bug #27270.