[Bug 27270] TplLogManager: clean up API

[bugzilla-daemon at freedesktop.org]

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

--- Comment #9 from Guillaume Desmottes <guillaume.desmottes at collabora.co.uk> 2010-06-02 07:31:44 PDT ---
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

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