[Bug 27270] TplLogManager: clean up API

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=27270

--- Comment #8 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2010-05-31 06:45:51 PDT ---
(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.

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