Bug 27270

I am thinking of internalise all blocking API for the Log Manager, unless there is a reason to keep both.

API used by empathy:

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

I moved all the other API (including the sync ones) as internal:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/log-store-api-27270

(In reply to comment #2)
> API used by empathy:
> 
> 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 ()
> 
> I moved all the other API (including the sync ones) as internal:
> http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/log-store-api-27270

I approve of that branch. Please leave this bug open, we still need to review the API.

More suggested changes:

>  /**
> - * tpl_log_manager_get_dates:
> + * _tpl_log_manager_get_dates:

Replacing /** with /* might reduce confusion in gtk-doc?

> -/* Start of tpl_log_manager_search_in_identifier_chats_new async implementation */
> +/* Start of tpl_log_manager_search_in_identifier_chats_new async
> + * implementation */

I don't like these comments; they're essentially content-free (we can see what function goes where by reading the function names!). Just delete them.

(In reply to comment #3)
> (In reply to comment #2)
> > API used by empathy:
> > 
> > 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 ()
> > 
> > I moved all the other API (including the sync ones) as internal:
> > http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/log-store-api-27270
> 
> I approve of that branch. Please leave this bug open, we still need to review
> the API.

merged.

> More suggested changes:
> 
> >  /**
> > - * tpl_log_manager_get_dates:
> > + * _tpl_log_manager_get_dates:
> 
> Replacing /** with /* might reduce confusion in gtk-doc?

What do you mean?
/** is the right way to start the documentation of a function.

> > -/* Start of tpl_log_manager_search_in_identifier_chats_new async implementation */
> > +/* Start of tpl_log_manager_search_in_identifier_chats_new async
> > + * implementation */
> 
> I don't like these comments; they're essentially content-free (we can see what
> function goes where by reading the function names!). Just delete them.

removed.

It seems to me that the current high-level functionality of TplLogManager can be summarized as:

* get all messages for a particular "chat ID" from the database, filtering them through an app-supplied predicate
* an optimized form for messages from a specified date, since in practice that's what Empathy uses

However, that's rather meaningless without a good definition of a chat ID. I think the API should look more like this:

* get all 1-1 messages (conversations?) for a particular contact, filtering through an optional app-supplied predicate
* get all messages for a particular named (XMPP/IRC/etc.) chatroom, filtering through an optional app-supplied predicate
* whatever similar accessor Empathy needs for unnamed multi-user chats (MSN/Skype/etc.)
* an optimized form of each of the above for messages from a specified date/date-range

> typedef struct
> {
>   GObjectClass parent_class;
> } TplLogManagerClass;

Needs a priv pointer, at the very least.

> gboolean tpl_log_manager_exists (TplLogManager *manager,
>     TpAccount *account, const gchar *chat_id, gboolean chatroom);

This seems to be looking for some conceptual thing that I'll call "abstract conversation" below. It might be worth having a type to represent a TplConversation - either a GObject, or a well-defined thing or tuple of things that identifies a conversation.

I don't like the name. tpl_log_manager_has_conversation()?

I don't like that this method implicitly assumes there are two sorts of conversation: chatrooms, and "the others" (presumably 1-1 chats?).

What's the intended scope of a chat ID? What's a chat ID anyway? Where would an API user obtain one? Do API users have to know implementation details about how chat IDs are formed? Can we be more formal about the namespacing guarantees we have?

I think a conversation should be identified by:

* an opaque string (or opaque object path?) that is globally unique, or
* a tuple (type_enum, string) that is globally unique, or
* some other defined tuple of things (perhaps it needs to include the backend ID?)

Of those options, I prefer the first for its simplicity, but if there are design constraints that mean we can't do that, we should have a real definition of one of the others.

Would a call be a chat ID, or be part of a thing-with-a-chat-ID, or what? How about a file transfer?

I suspect that the real situation is more like this:

* "chat ID" is secretly less opaque than it looks, and that it really means "the string form of a handle", and is secretly namespaced by a TpHandleType, except that MSN-style nameless chats have some sort of UUID (?) instead
* clients have to know how the chat IDs work in order to do a suitable query

Is that what's happening here? If so, we should just say it; referring to telepathy-spec as a canonical document is better than leaving it vague.

> tpl_log_manager_get_dates_async
>  * Retrieves a list of dates, in string form YYYYMMDD, corrisponding to each day

This seems a weird way to represent dates; it's almost as though the representation in the database is leaking through to the API :-P

I'd be inclined to parse the dates from the database with sscanf(), output them to the library user as a GList<GDate>, then let the UI use g_date_strftime() however it wants (probably with format "%x", which yields a local-specific date representation). Otherwise, provide a function to turn a string of this form into a GDate (again using sscanf()).

>   /* arg passed is not a valid GObject or in the expected format */
>   TPL_LOG_MANAGER_ERROR_BAD_ARG

This seems really strange, and its usage in tpl_log_manager_get_filtered_messages_async() worries me. You're taking something that's really a programming error that can only be fixed in the app's source code, and making it look like a recoverable runtime error. If the programmer has got it wrong and the app is broken, just use g_return_if_fail; if there is something going wrong outside the programmer's control (like a network timeout), *that's* when you need to get into structured error reporting.

The documentation from _tpl_log_manager_get_chats needs copying/adapting for tpl_log_manager_get_chats_async, since that's the public API.

In tpl_log_manager_get_filtered_messages_async, I don't think the filter callback should not be mandatory - there's a perfectly sensible interpretation for filter == NULL, which is "accept every message". To be honest I'm not even sure that the filter is needed - the caller can filter the result just as easily, and the data needs to be in-memory anyway.

I think tpl_log_manager_search_new_async() is either misnamed or wrongly implemented - either it should be called tpl_log_manager_search_async(), or it should return a TplLogManagerSearch object.

In this case I think it should be called tpl_log_manager_search_async: conceptually, it's an async method LogManager.search() which returns a GList<TplLogSearchHit>.

Either TplLogSearchHit or GList<TplLogSearchHit> should have a boxed type.

I'm not quite sure what a TplLogSearchHit is, conceptually: it seems to be the intersection of an "abstract conversation" (account + chat ID + chat type) and a date. Why does a search return this rather artificial thing, and not, for instance, an individual log entry? Conceptually, it'd seem to me to make more sense to return some individual log entries, and it'd then be the UI's decision how much context around those log entries it needed to fetch.

Is the intention of tpl_log_manager_get_chats() that, conceptually, it returns "abstract conversation" IDs, i.e. account + chat ID + chat type? I'm wary of re-using the TplLogSearchHit type for these, since it has other fields that aren't relevant.

> gchar *tpl_log_manager_get_date_readable (const gchar *date);

This looks like a job for the UI; a non-localized library is never going to produce the date format users expect in a consistent way. I'd deprecate or remove this function, and make the UIs format the date as they wish - which would be easiest if the API was in terms of GDate as I recommended above.

For the record, Empathy uses:

- tpl_log_manager_get_filtered_messages_{async,finish} in empathy-chat to retrieve the 5 last messages (backlog) when opening a new chat. It passed a filter function checking if the message  is already in the pending messages of the chat.


- tpl_log_manager_exists() in empathy-contact-menu to check if the "View previous conversation" menu entry should be sensitive or not.

- tpl_log_manager_get_messages_for_date_{async,finish} in log-window when searching all the conversations with a specific contact at a specific date.

- tpl_log_manager_search_new_{async,finish} in log-window when searching for a pattern in all the conversations. Once it got the result it uses tpl_log_manager_get_date_readable() and 
tpl_log_manager_search_free() on the results.

- tpl_log_manager_get_chats_{async,finish} in log-window to fetch the list of (chat_id, account, chatroom) having logs. They are displayed in a treeview in the "Conversations" tab.

- tpl_log_manager_get_dates_{async,finish} in log-window to mark on the calendar the dates containing logs.

The logger currently doesn't log unamed multi user chats (bug #28329). Empathy does log them but the log viewer doesn't display them for some reason.
I'd be tempted to say that's not an integration blocker for now as unamed group chats seem to be completely broken in Butterfly (which is the only CM implementing those afaik).

So let's keep in mind the API for unamed multi user chats here but we won't implement it right now.

(In reply to comment #5)
> It seems to me that the current high-level functionality of TplLogManager can
> be summarized as:
> 
> * get all messages for a particular "chat ID" from the database, filtering them
> through an app-supplied predicate
> * an optimized form for messages from a specified date, since in practice
> that's what Empathy uses

and an "optimized" searching for a specific pattern in logs.

> However, that's rather meaningless without a good definition of a chat ID. I
> think the API should look more like this:
> 
> * get all 1-1 messages (conversations?) for a particular contact, filtering
> through an optional app-supplied predicate

What about:

void tpl_log_manager_get_contact_messages_async (TplLogManager *manager,
    TpAccount *account,
    const gchar *contact_id,
    guint num_messages,
    TplLogMessageFilter filter, (????)
    gpointer filter_user_data,
    GAsyncReadyCallback callback,
    gpointer user_data);

Not sure if we still neeed the filter, as you said it doesn't really buy us anything.

or maybe _get_text_log_entries_async () to be more coherent?

> * get all messages for a particular named (XMPP/IRC/etc.) chatroom, filtering
> through an optional app-supplied predicate

void tpl_log_manager_get_chatroom_messages_async (TplLogManager *manager,
    TpAccount *account,
    const gchar *chatroom_id,
    guint num_messages,
    TplLogMessageFilter filter, (????)
    gpointer filter_user_data,
    GAsyncReadyCallback callback,
    gpointer user_data);

> * whatever similar accessor Empathy needs for unnamed multi-user chats
> (MSN/Skype/etc.)

Not sure what to do here. As said earlier Empathy doesn't currently propertly
support those.

> * an optimized form of each of the above for messages from a specified
> date/date-range

agreed.


> > typedef struct
> > {
> >   GObjectClass parent_class;
> > } TplLogManagerClass;
> 
> Needs a priv pointer, at the very least.

In the *class* struct?

> > gboolean tpl_log_manager_exists (TplLogManager *manager,
> >     TpAccount *account, const gchar *chat_id, gboolean chatroom);
> 
> This seems to be looking for some conceptual thing that I'll call "abstract
> conversation" below. It might be worth having a type to represent a
> TplConversation - either a GObject, or a well-defined thing or tuple of things
> that identifies a conversation.

What would a TpConversation? An object containing the meta data associated
with the logs (the handle type, the ID, etc) and the log entries?

> I don't like the name. tpl_log_manager_has_conversation()?
> 
> I don't like that this method implicitly assumes there are two sorts of
> conversation: chatrooms, and "the others" (presumably 1-1 chats?).
> 
> What's the intended scope of a chat ID? What's a chat ID anyway? Where would an
> API user obtain one? Do API users have to know implementation details about how
> chat IDs are formed? Can we be more formal about the namespacing guarantees we
> have?
> 
> I think a conversation should be identified by:
> 
> * an opaque string (or opaque object path?) that is globally unique, or
> * a tuple (type_enum, string) that is globally unique, or
> * some other defined tuple of things (perhaps it needs to include the backend
> ID?)
> 
> Of those options, I prefer the first for its simplicity, but if there are
> design constraints that mean we can't do that, we should have a real definition
> of one of the others.
> 
> Would a call be a chat ID, or be part of a thing-with-a-chat-ID, or what? How
> about a file transfer?
> 
> I suspect that the real situation is more like this:
> 
> * "chat ID" is secretly less opaque than it looks, and that it really means
> "the string form of a handle", and is secretly namespaced by a TpHandleType,
> except that MSN-style nameless chats have some sort of UUID (?) instead
> * clients have to know how the chat IDs work in order to do a suitable query
> 

Sounds right to me, except that, afaik, unamed chans doesn't have such UUID in
butterfly. So I don't know how we're going to identify unamed chats here :\
Maybe the spec could define that unamed chatrooms should be associated with an
unique ID in order to identify them later. We could compute such id by
truncating the time of the creation of the chat with a random number or
something.

> Is that what's happening here? If so, we should just say it; referring to
> telepathy-spec as a canonical document is better than leaving it vague.
> 
> > tpl_log_manager_get_dates_async
> >  * Retrieves a list of dates, in string form YYYYMMDD, corrisponding to each day
> 
> This seems a weird way to represent dates; it's almost as though the
> representation in the database is leaking through to the API :-P
> 
> I'd be inclined to parse the dates from the database with sscanf(), output them
> to the library user as a GList<GDate>, then let the UI use g_date_strftime()
> however it wants (probably with format "%x", which yields a local-specific date
> representation). Otherwise, provide a function to turn a string of this form
> into a GDate (again using sscanf()).
> 

Agreed, using GDate is much better.

> >   /* arg passed is not a valid GObject or in the expected format */
> >   TPL_LOG_MANAGER_ERROR_BAD_ARG
> 
> This seems really strange, and its usage in
> tpl_log_manager_get_filtered_messages_async() worries me. You're taking
> something that's really a programming error that can only be fixed in the app's
> source code, and making it look like a recoverable runtime error. If the
> programmer has got it wrong and the app is broken, just use g_return_if_fail;
> if there is something going wrong outside the programmer's control (like a
> network timeout), *that's* when you need to get into structured error
> reporting.
> 
> The documentation from _tpl_log_manager_get_chats needs copying/adapting for
> tpl_log_manager_get_chats_async, since that's the public API.
> 
> In tpl_log_manager_get_filtered_messages_async, I don't think the filter
> callback should not be mandatory - there's a perfectly sensible interpretation
> for filter == NULL, which is "accept every message". To be honest I'm not even
> sure that the filter is needed - the caller can filter the result just as
> easily, and the data needs to be in-memory anyway.
> 

Agreed, I'll change that.

> I think tpl_log_manager_search_new_async() is either misnamed or wrongly
> implemented - either it should be called tpl_log_manager_search_async(), or it
> should return a TplLogManagerSearch object.
> 
> In this case I think it should be called tpl_log_manager_search_async:
> conceptually, it's an async method LogManager.search() which returns a
> GList<TplLogSearchHit>.
>
You're right. I'll rename it.

> Either TplLogSearchHit or GList<TplLogSearchHit> should have a boxed type.
>
nod.

> I'm not quite sure what a TplLogSearchHit is, conceptually: it seems to be the
> intersection of an "abstract conversation" (account + chat ID + chat type) and
> a date. Why does a search return this rather artificial thing, and not, for
> instance, an individual log entry? Conceptually, it'd seem to me to make more
> sense to return some individual log entries, and it'd then be the UI's decision
> how much context around those log entries it needed to fetch.
> 

Empathy uses it to populate the treeview containing contacts and chatrooms
having logs in the log viewer.
We could replace that by
get_all_private_conversation_having_log () returning ID and TpAccount
and a _chatroom_ variant.

> Is the intention of tpl_log_manager_get_chats() that, conceptually, it returns
> "abstract conversation" IDs, i.e. account + chat ID + chat type? I'm wary of
> re-using the TplLogSearchHit type for these, since it has other fields that
> aren't relevant.
> 
> > gchar *tpl_log_manager_get_date_readable (const gchar *date);
> 
> This looks like a job for the UI; a non-localized library is never going to
> produce the date format users expect in a consistent way. I'd deprecate or
> remove this function, and make the UIs format the date as they wish - which
> would be easiest if the API was in terms of GDate as I recommended above.

agreed.

I've fixed most of the uncontroversial comments:
http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/manager-api-27270

(In reply to comment #8)
> > > tpl_log_manager_get_dates_async
> > >  * Retrieves a list of dates, in string form YYYYMMDD, corrisponding to each day
> > 
> > This seems a weird way to represent dates; it's almost as though the
> > representation in the database is leaking through to the API :-P
> > 
> > I'd be inclined to parse the dates from the database with sscanf(), output them
> > to the library user as a GList<GDate>, then let the UI use g_date_strftime()
> > however it wants (probably with format "%x", which yields a local-specific date
> > representation). Otherwise, provide a function to turn a string of this form
> > into a GDate (again using sscanf()).
> > 
> 
> Agreed, using GDate is much better.

done.

> > >   /* arg passed is not a valid GObject or in the expected format */
> > >   TPL_LOG_MANAGER_ERROR_BAD_ARG
> > This seems really strange, and its usage in
> > tpl_log_manager_get_filtered_messages_async() worries me. You're taking
> > something that's really a programming error that can only be fixed in the app's
> > source code, and making it look like a recoverable runtime error. If the
> > programmer has got it wrong and the app is broken, just use g_return_if_fail;
> > if there is something going wrong outside the programmer's control (like a
> > network timeout), *that's* when you need to get into structured error
> > reporting.

Removed (it wasn't used actually).

> > The documentation from _tpl_log_manager_get_chats needs copying/adapting for
> > tpl_log_manager_get_chats_async, since that's the public API.
> >

done

> > In tpl_log_manager_get_filtered_messages_async, I don't think the filter
> > callback should not be mandatory - there's a perfectly sensible interpretation
> > for filter == NULL, which is "accept every message". To be honest I'm not even
> > sure that the filter is needed - the caller can filter the result just as
> > easily, and the data needs to be in-memory anyway.

done. I didn't remove the filter for now; not sure what's best.

> > I think tpl_log_manager_search_new_async() is either misnamed or wrongly
> > implemented - either it should be called tpl_log_manager_search_async(), or it
> > should return a TplLogManagerSearch object.
> > 
> > In this case I think it should be called tpl_log_manager_search_async:
> > conceptually, it's an async method LogManager.search() which returns a
> > GList<TplLogSearchHit>.

renamed

> > Either TplLogSearchHit or GList<TplLogSearchHit> should have a boxed type.
> >
> nod.

Didn't do that yet and it's not clear what will happen to TplLogSearchHit

> > > gchar *tpl_log_manager_get_date_readable (const gchar *date);
> > 
> > This looks like a job for the UI; a non-localized library is never going to
> > produce the date format users expect in a consistent way. I'd deprecate or
> > remove this function, and make the UIs format the date as they wish - which
> > would be easiest if the API was in terms of GDate as I recommended above.
> 
> agreed.

removed

(In reply to comment #9)
> I've fixed most of the uncontroversial comments:
> http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/manager-api-27270

I have a few complaints about the date manipulation, but overall this is a big improvement.

> +      DEBUG ("Looking up date %u/%u/%u", g_date_get_day (date),
> +          g_date_get_month (date), g_date_get_year (date));

Ugh. d/m/y dates are ambiguous (Europeans interpret them as intended, Americans interpret them as m/d/y and get confused). Please use ISO-8601 in all "C locale" contexts, like debug messages:

      DEBUG ("Looking up date %04u-%02u-%02u", g_date_get_year (date),
          g_date_get_month (date), g_date_get_day (date));

> + * Returns: a GList of (GDate *), to be freed using something like
> + * g_list_foreach (lst, g_data_free, NULL);

Typo: g_date_free.

_tpl_log_manager_get_messages_for_date, tpl_log_manager_get_messages_for_date_async, tpl_log_store_get_messages_for_date and the get_messages_for_date virtual method should all take a const GDate * parameter where they formerly used const gchar.

> +#define _XOPEN_SOURCE /* glibc2 needs this for strptime */

This should be a hint that you're doing something non-portable, and indeed Windows doesn't have strptime (this has bitten us before, in Gabble and telepathy-glib). Thankfully, you don't need it: you can easily reimplement create_date_from_string() by using sscanf() (which is ISO C) to parse a single unsigned int, then using (u % 100), ((u / 100) % 100) and (u / 10000) as the day, month, year, and failing if !g_date_valid_dmy().

> +              DEBUG ("Found text:'%s' in file:'%s' on date:'%u/%u/%u'", text,
> +                  hit->filename, g_date_get_day (hit->date),
> +                  g_date_get_month (hit->date), g_date_get_year (hit->date));

ISO date again, please.

(In reply to comment #10)
> (In reply to comment #9)
> > I've fixed most of the uncontroversial comments:
> > http://git.collabora.co.uk/?p=user/cassidy/telepathy-logger;a=shortlog;h=refs/heads/manager-api-27270
> 
> I have a few complaints about the date manipulation, but overall this is a big
> improvement.
> 
> > +      DEBUG ("Looking up date %u/%u/%u", g_date_get_day (date),
> > +          g_date_get_month (date), g_date_get_year (date));
> 
> Ugh. d/m/y dates are ambiguous (Europeans interpret them as intended, Americans
> interpret them as m/d/y and get confused). Please use ISO-8601 in all "C
> locale" contexts, like debug messages:
> 
>       DEBUG ("Looking up date %04u-%02u-%02u", g_date_get_year (date),
>           g_date_get_month (date), g_date_get_day (date));

done.

> > + * Returns: a GList of (GDate *), to be freed using something like
> > + * g_list_foreach (lst, g_data_free, NULL);
> 
> Typo: g_date_free.

fixed.

> _tpl_log_manager_get_messages_for_date,
> tpl_log_manager_get_messages_for_date_async,
> tpl_log_store_get_messages_for_date and the get_messages_for_date virtual
> method should all take a const GDate * parameter where they formerly used const
> gchar.

done.

> > +#define _XOPEN_SOURCE /* glibc2 needs this for strptime */
> 
> This should be a hint that you're doing something non-portable, and indeed
> Windows doesn't have strptime (this has bitten us before, in Gabble and
> telepathy-glib). Thankfully, you don't need it: you can easily reimplement
> create_date_from_string() by using sscanf() (which is ISO C) to parse a single
> unsigned int, then using (u % 100), ((u / 100) % 100) and (u / 10000) as the
> day, month, year, and failing if !g_date_valid_dmy().

done.

> > +              DEBUG ("Found text:'%s' in file:'%s' on date:'%u/%u/%u'", text,
> > +                  hit->filename, g_date_get_day (hit->date),
> > +                  g_date_get_month (hit->date), g_date_get_year (hit->date));
> 
> ISO date again, please.

fixed.

review+ for the branch, but please leave this bug open.

(In reply to comment #12)
> review+ for the branch, but please leave this bug open.

branch has been merged.

(In reply to comment #5)
> I think a conversation should be identified by:
> 
> * an opaque string (or opaque object path?) that is globally unique, or
> * a tuple (type_enum, string) that is globally unique, or
> * some other defined tuple of things (perhaps it needs to include the backend
> ID?)
> 
> Of those options, I prefer the first for its simplicity, but if there are
> design constraints that mean we can't do that, we should have a real definition
> of one of the others.
> 
> Would a call be a chat ID, or be part of a thing-with-a-chat-ID, or what? How
> about a file transfer?

We need to abstract this somehow and stop exposing "gboolean chatroom" all over the public API. See bug #32477. They seem to be exposed so tp-logger knows whether to add "chatrooms/" to the URI to look for the logs.

We should remove that from the API, maybe adding a generic way to specify the event type if we really need that, e.g. with a TplLogType type arg, with TplLogType being an enum with TPL_LOG_TYPE_CHATROOM, _CALL, _FILE_TRANSFER, etc values.

Opinions? I can probably get to this soon.

Also what should I do when breaking API? Add new one and deprecate the old, or are we planning on breaking it for the next major release and I can just do that then? I guess the former...

> I suspect that the real situation is more like this:
> 
> * "chat ID" is secretly less opaque than it looks, and that it really means
> "the string form of a handle", and is secretly namespaced by a TpHandleType,
> except that MSN-style nameless chats have some sort of UUID (?) instead
> * clients have to know how the chat IDs work in order to do a suitable query
> 
> Is that what's happening here? If so, we should just say it; referring to
> telepathy-spec as a canonical document is better than leaving it vague.

(In reply to comment #14)
> We need to abstract this somehow and stop exposing "gboolean chatroom" all over
> the public API. See bug #32477. They seem to be exposed so tp-logger knows
> whether to add "chatrooms/" to the URI to look for the logs.
> 
> We should remove that from the API, maybe adding a generic way to specify the
> event type if we really need that, e.g. with a TplLogType type arg, with
> TplLogType being an enum with TPL_LOG_TYPE_CHATROOM, _CALL, _FILE_TRANSFER, etc
> values.
> 
> Opinions? I can probably get to this soon.

I've hacked something along these lines in http://git.collabora.co.uk/?p=user/pochu/telepathy-logger.git;a=shortlog;h=refs/heads/generic-log-manager-api-27270

I still need to port empathy to the new API and make sure everything is working fine.

>diff --git a/telepathy-logger/log-manager.h b/telepathy-logger/log-manager.h
>@@ -84,39 +92,39 @@ gboolean tpl_log_manager_get_dates_finish (TplLogManager *self,

> -gboolean tpl_log_manager_get_filtered_messages_finish (TplLogManager *self,
> +gboolean tpl_log_manager_get_filtered_events_finish (TplLogManager *self,
>     GAsyncResult *result,
>     GList **messages,
>     GError **error);

>-gboolean tpl_log_manager_get_filtered_messages_finish (TplLogManager *self,
>+gboolean tpl_log_manager_get_filtered_events_finish (TplLogManager *self,
>     GAsyncResult *result,
>     GList **messages,
>     GError **error);

Need to change GList **messages into GList **events

>diff --git a/telepathy-logger/log-manager.c b/telepathy-logger/log-manager.c
>@@ -383,36 +382,34 @@ _tpl_log_manager_register_log_store (TplLogManager *self,
>+ * Checks if @id exists for @account and is of type @type.

In this case can we have multiple types? Does it mean it exists for all type, or one of ?

>@@ -520,18 +515,18 @@ log_manager_message_date_cmp (gconstpointer a,
>   one_time = tpl_entry_get_timestamp (one);
>   two_time = tpl_entry_get_timestamp (two);
> 
>-  /* return -1, o or 1 depending on message1 is newer, the same or older than
>+  /* return -1, 0 or 1 depending on message1 is newer, the same or older than
>    * message2 */
is newer -> being newer? and message -> event?

> gint
> _tpl_log_manager_search_hit_compare (TplLogSearchHit *a,
>     TplLogSearchHit *b)

There is something weird about this function now, and removing the documentation does not help much.

>+      if (a->type == b->type)
>+        return 0;
>     }

This function seem to be used to sort the result, so it must have a finit order (whatever the order is). Maybe you should do that instead:

  if (a->type < b->type)
    return -1;
  else if (a->type > b->type)
   return 1;
  else
    return strcmp(a->id, b->id);

Take note that I don't waste time doing strcmp() if type mismatch.

>diff --git a/extensions/Logger.xml b/extensions/Logger.xml
>@@ -82,6 +82,10 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.</
>+      <!-- FIXME: we're returning a list of events that can be
>+           messages (chats), but also calls and any other event
>+           type in the future, so make the return type a list of
>+           TplLogSearchHit or something generic than a(ssx) ? -->
>       <arg direction="out" name="Messages" type="a(ssx)"
>            tp:type="Chat_Message[]" />

I think we need aa(sv). Some field will be shared between events and other specific (which will have to be documented in the protocol). But that can be kept for when we move from TplSearchHit to TplEvent (TplEvent will simply have to know how to serialise/deserialise itself into a(sv).

>diff --git a/telepathy-logger/log-store-internal.h b/telepathy-logger/log-store-internal.h
>@@ -63,21 +63,22 @@ typedef struct
>   gboolean (*add_message) (TplLogStore *self, TplEntry *message,
>       GError **error);

Is this suppose to be add_event now ?

>diff --git a/telepathy-logger/log-store-xml.c b/telepathy-logger/log-store-xml.c
>@@ -1042,6 +1043,7 @@ log_store_xml_search_in_identifier_chats_new (TplLogStore *store,
>   g_return_val_if_fail (!TPL_STR_EMPTY (text), NULL);
> 
>   account_dir = log_store_account_to_dirname (account);
>+  /* FIXME: take @type into account for the dir. */

Is it something you forgot, or it's not needed for now ?

>@@ -1120,35 +1122,35 @@ log_store_xml_get_chats_for_dir (TplLogStoreXml *self,
>-/* returns a Glist of TplEntryText instances */
>+/* returns a Glist of FIXME TplEntryText instances */

As this one used TplEntryText (sub-class of TplEntry) it should be very easy to fix, s/TplEntryText/TplEntry/ ;). Unless you wanted this note for implementation of Call support ?

(In reply to comment #16)
> >-gboolean tpl_log_manager_get_filtered_messages_finish (TplLogManager *self,
> >+gboolean tpl_log_manager_get_filtered_events_finish (TplLogManager *self,
> >     GAsyncResult *result,
> >     GList **messages,
> >     GError **error);
> 
> Need to change GList **messages into GList **events

Done, and a bunch of other s/message/event/ that made sense.

> >+ * Checks if @id exists for @account and is of type @type.
> 
> In this case can we have multiple types? Does it mean it exists for all type,
> or one of ?

I don't think so, since type there is associated to id. It's really a pair {id, type}. I could improve that documentation to e.g. "Checks if @id of type @type exists for @account".

> >@@ -520,18 +515,18 @@ log_manager_message_date_cmp (gconstpointer a,
> >   one_time = tpl_entry_get_timestamp (one);
> >   two_time = tpl_entry_get_timestamp (two);
> > 
> >-  /* return -1, o or 1 depending on message1 is newer, the same or older than
> >+  /* return -1, 0 or 1 depending on message1 is newer, the same or older than
> >    * message2 */
> is newer -> being newer? and message -> event?

Yes, fixed.

> 
> > gint
> > _tpl_log_manager_search_hit_compare (TplLogSearchHit *a,
> >     TplLogSearchHit *b)
> 
> There is something weird about this function now, and removing the
> documentation does not help much.

Well I didn't remove it, I just changed it to document what it was doing after my changes

> 
> >+      if (a->type == b->type)
> >+        return 0;
> >     }
> 
> This function seem to be used to sort the result, so it must have a finit order
> (whatever the order is). Maybe you should do that instead:

Not really. It's only used to compare (as in == , !=) to know whether to include an event in a list or not (because it's already there), so order doesn't matter. I've changed it to do that again though, since it doesn't cost us much

>   if (a->type < b->type)
>     return -1;
>   else if (a->type > b->type)
>    return 1;
>   else
>     return strcmp(a->id, b->id);
> 
> Take note that I don't waste time doing strcmp() if type mismatch.

Makes sense, changed accordingly.

> >diff --git a/extensions/Logger.xml b/extensions/Logger.xml
> >@@ -82,6 +82,10 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.</
> >+      <!-- FIXME: we're returning a list of events that can be
> >+           messages (chats), but also calls and any other event
> >+           type in the future, so make the return type a list of
> >+           TplLogSearchHit or something generic than a(ssx) ? -->
> >       <arg direction="out" name="Messages" type="a(ssx)"
> >            tp:type="Chat_Message[]" />
> 
> I think we need aa(sv). Some field will be shared between events and other
> specific (which will have to be documented in the protocol). But that can be
> kept for when we move from TplSearchHit to TplEvent (TplEvent will simply have
> to know how to serialise/deserialise itself into a(sv).

Why aa(sv) ? How do we know that will be enough for future event types?

> >diff --git a/telepathy-logger/log-store-internal.h b/telepathy-logger/log-store-internal.h
> >@@ -63,21 +63,22 @@ typedef struct
> >   gboolean (*add_message) (TplLogStore *self, TplEntry *message,
> >       GError **error);
> 
> Is this suppose to be add_event now ?

Yes, fixed.

> >diff --git a/telepathy-logger/log-store-xml.c b/telepathy-logger/log-store-xml.c
> >@@ -1042,6 +1043,7 @@ log_store_xml_search_in_identifier_chats_new (TplLogStore *store,
> >   g_return_val_if_fail (!TPL_STR_EMPTY (text), NULL);
> > 
> >   account_dir = log_store_account_to_dirname (account);
> >+  /* FIXME: take @type into account for the dir. */
> 
> Is it something you forgot, or it's not needed for now ?

I fixed it, but it's in the next commit (in the sqlite log store commit) by mistake... :(

I can squash that part of the commit into the xml log store one where it should be.

> >@@ -1120,35 +1122,35 @@ log_store_xml_get_chats_for_dir (TplLogStoreXml *self,
> >-/* returns a Glist of TplEntryText instances */
> >+/* returns a Glist of FIXME TplEntryText instances */
> 
> As this one used TplEntryText (sub-class of TplEntry) it should be very easy to
> fix, s/TplEntryText/TplEntry/ ;). Unless you wanted this note for
> implementation of Call support ?

It needs more changes, so it doesn't make a lot of sense to just do that... we also need to change the parsing in log_store_xml_get_messages_for_file()... although since there's nothing else to parse right now, I can probably change that. Done :)

Branch updated.

(In reply to comment #17)
> (In reply to comment #16)
> > >+ * Checks if @id exists for @account and is of type @type.
> > 
> > In this case can we have multiple types? Does it mean it exists for all type,
> > or one of ?
> 
> I don't think so, since type there is associated to id. It's really a pair {id,
> type}. I could improve that documentation to e.g. "Checks if @id of type @type
> exists for @account".
Fair, small doc like this would be nice.
 
> > >diff --git a/extensions/Logger.xml b/extensions/Logger.xml
> > >@@ -82,6 +82,10 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.</
> > >+      <!-- FIXME: we're returning a list of events that can be
> > >+           messages (chats), but also calls and any other event
> > >+           type in the future, so make the return type a list of
> > >+           TplLogSearchHit or something generic than a(ssx) ? -->
> > >       <arg direction="out" name="Messages" type="a(ssx)"
> > >            tp:type="Chat_Message[]" />
> > 
> > I think we need aa(sv). Some field will be shared between events and other
> > specific (which will have to be documented in the protocol). But that can be
> > kept for when we move from TplSearchHit to TplEvent (TplEvent will simply have
> > to know how to serialise/deserialise itself into a(sv).
> 
> Why aa(sv) ? How do we know that will be enough for future event types?

a(sv) is the most generic type I know in DBus, we also have bunch of tp_asv_* helpers in telepathy-glib for those. You can pretty much represent anything into that, but that's for future anyway. Sorry for the noise in this review.

 
> > >diff --git a/telepathy-logger/log-store-xml.c b/telepathy-logger/log-store-xml.c
> > >@@ -1042,6 +1043,7 @@ log_store_xml_search_in_identifier_chats_new (TplLogStore *store,
> > >   g_return_val_if_fail (!TPL_STR_EMPTY (text), NULL);
> > > 
> > >   account_dir = log_store_account_to_dirname (account);
> > >+  /* FIXME: take @type into account for the dir. */
> > 
> > Is it something you forgot, or it's not needed for now ?
> 
> I fixed it, but it's in the next commit (in the sqlite log store commit) by
> mistake... :(
> 
> I can squash that part of the commit into the xml log store one where it should
> be.

I think it would be better if it's not too hard to do.

> 
> > >@@ -1120,35 +1122,35 @@ log_store_xml_get_chats_for_dir (TplLogStoreXml *self,
> > >-/* returns a Glist of TplEntryText instances */
> > >+/* returns a Glist of FIXME TplEntryText instances */
> > 
> > As this one used TplEntryText (sub-class of TplEntry) it should be very easy to
> > fix, s/TplEntryText/TplEntry/ ;). Unless you wanted this note for
> > implementation of Call support ?
> 
> It needs more changes, so it doesn't make a lot of sense to just do that... we
> also need to change the parsing in log_store_xml_get_messages_for_file()...
> although since there's nothing else to parse right now, I can probably change
> that. Done :)
> 
> Branch updated.

Nod. So after those two small changes it's good to go.

(In reply to comment #18)
> Nod. So after those two small changes it's good to go.

Thanks. Fixed and pushed to master.

Good, closing this bug, we'll create specific bugs for other API rework steps.