Bug 27178

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):
log-manager
log-entry-text
log-entry
contact
conf
conf-gconf


Modules that have symbols used only internally (ie among them):

observer.c
dbus-service.c
channel.c
channel-factory.c
channel-text.c
log-store-factory.c
debug.c
action-chain.c


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

contact.c

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.

log-entry.c/log-entry-text.c

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

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

(In reply to comment #2)
> Modules that have symbols used only internally (ie among them):
> 
> observer.c
> dbus-service.c
> channel.c
> channel-factory.c
> channel-text.c
> log-store-factory.c
> debug.c
> action-chain.c

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)
> contact.c
> 
> 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?

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

observer.c
dbus-service.c
channel.c
channel-factory.c
channel-text.c
log-store-factory.c
debug.c
action-chain.c

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)
> 
> observer.c
> dbus-service.c
> channel.c
> channel-factory.c
> channel-text.c
> log-store-factory.c
> debug.c
> action-chain.c
> 
> 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:

TPL_CONTACT_USER
TPL_IS_CONTACT ()
TPL_IS_LOG_ENTRY ()
TPL_IS_LOG_ENTRY_TEXT ()
TplContact
TplLogEntry
TplLogEntryText
TplLogManager
TplLogSearchHit
tpl_contact_get_alias ()
tpl_contact_get_avatar_token ()
tpl_contact_get_contact_type ()
tpl_contact_get_identifier ()
tpl_log_entry_get_account_path ()
tpl_log_entry_get_timestamp ()
tpl_log_entry_text_get_log_id ()
tpl_log_entry_text_get_message ()
tpl_log_entry_text_get_receiver ()
tpl_log_entry_text_get_sender ()
tpl_log_manager_dup_singleton ()
tpl_log_manager_exists ()
tpl_log_manager_get_chats_async ()
tpl_log_manager_get_chats_async_finish ()
tpl_log_manager_get_date_readable ()
tpl_log_manager_get_dates_async ()
tpl_log_manager_get_dates_async_finish ()
tpl_log_manager_get_filtered_messages_async ()
tpl_log_manager_get_filtered_messages_async_finish ()
tpl_log_manager_get_messages_for_date_async ()
tpl_log_manager_get_messages_for_date_async_finish ()
tpl_log_manager_search_free ()
tpl_log_manager_search_new_async ()
tpl_log_manager_search_new_async_finish ()

What about just make this API public and keep the rest private for now?

This branch move most of the API as internal:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/remove-headers-27178

(In reply to comment #12)
> This branch move most of the API as internal:
> http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/remove-headers-27178

r+ but please keep this bug open; all non-public symbols should ideally be underscored.

merged

http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/abi-27178

This is the remaning ABI with this branch:


tpl_cli_init
tpl_cli_logger_call_add_favourite_contact
tpl_cli_logger_call_get_favourite_contacts
tpl_cli_logger_call_get_recent_messages
tpl_cli_logger_call_remove_favourite_contact
tpl_cli_logger_connect_to_favourite_contacts_changed
tpl_contact_get_alias
tpl_contact_get_avatar_token
tpl_contact_get_contact_type
tpl_contact_get_identifier
tpl_contact_get_type
tpl_iface_quark_logger
tpl_log_entry_get_account_path
tpl_log_entry_get_pending_msg_id
tpl_log_entry_get_receiver
tpl_log_entry_get_sender
tpl_log_entry_get_timestamp
tpl_log_entry_get_type
tpl_log_entry_text_get_message
tpl_log_entry_text_get_type
tpl_log_manager_dup_singleton
tpl_log_manager_exists
tpl_log_manager_get_chats_async
tpl_log_manager_get_chats_finish
tpl_log_manager_get_dates_async
tpl_log_manager_get_dates_finish
tpl_log_manager_get_filtered_messages_async
tpl_log_manager_get_filtered_messages_finish
tpl_log_manager_get_messages_for_date_async
tpl_log_manager_get_messages_for_date_finish
tpl_log_manager_get_type
tpl_log_manager_search_async
tpl_log_manager_search_finish
tpl_log_manager_search_free
tpl_svc_logger_emit_favourite_contacts_changed
tpl_svc_logger_get_type
tpl_svc_logger_implement_add_favourite_contact
tpl_svc_logger_implement_get_favourite_contacts
tpl_svc_logger_implement_get_recent_messages
tpl_svc_logger_implement_remove_favourite_contact
tpl_type_dbus_array_ssx
tpl_type_dbus_struct_ssx


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)
> http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/abi-27178

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.
>
> tpl_iface_quark_logger
> tpl_cli_init
> tpl_cli_logger_call_add_favourite_contact
> tpl_cli_logger_call_get_favourite_contacts
> tpl_cli_logger_call_get_recent_messages
> tpl_cli_logger_call_remove_favourite_contact
> tpl_cli_logger_connect_to_favourite_contacts_changed
> tpl_svc_logger_emit_favourite_contacts_changed
> tpl_svc_logger_get_type
> tpl_svc_logger_implement_add_favourite_contact
> tpl_svc_logger_implement_get_favourite_contacts
> tpl_svc_logger_implement_get_recent_messages
> tpl_svc_logger_implement_remove_favourite_contact
> tpl_type_dbus_array_ssx
> tpl_type_dbus_struct_ssx

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.

> tpl_contact_get_alias
> tpl_contact_get_avatar_token
> tpl_contact_get_contact_type
> tpl_contact_get_identifier
> tpl_contact_get_type
> tpl_log_entry_get_receiver
> tpl_log_entry_get_sender

Under discussion on Bug #27269.

> tpl_log_entry_get_account_path
> tpl_log_entry_get_pending_msg_id
> tpl_log_entry_get_timestamp
> tpl_log_entry_get_type
> tpl_log_entry_text_get_message
> tpl_log_entry_text_get_type

Under discussion on Bug #27271.

> tpl_log_manager_dup_singleton
> tpl_log_manager_exists
> tpl_log_manager_get_chats_async
> tpl_log_manager_get_chats_finish
> tpl_log_manager_get_dates_async
> tpl_log_manager_get_dates_finish
> tpl_log_manager_get_filtered_messages_async
> tpl_log_manager_get_filtered_messages_finish
> tpl_log_manager_get_messages_for_date_async
> tpl_log_manager_get_messages_for_date_finish
> tpl_log_manager_get_type
> tpl_log_manager_search_async
> tpl_log_manager_search_finish
> tpl_log_manager_search_free

Under discussion on Bug #27270.

Branch merged.