[Bug 27270] TplLogManager: clean up API

[bugzilla-daemon at freedesktop.org]

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

Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|API review: TplLogManager   |TplLogManager: clean up API
             Status|NEW                         |ASSIGNED
         AssignedTo|simon.mcvittie at collabora.co |guillaume.desmottes at collabo
                   |.uk                         |ra.co.uk
                 CC|                            |guillaume.desmottes at collabo
                   |                            |ra.co.uk

--- Comment #5 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-05-28 06:05:09 PDT ---
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.

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