Now that we have async APIs for TplLogManager, the GetRecentMessages method call should use them. This branch implements that. Can someone please review it. http://git.collabora.co.uk/?p=user/danni/telepathy-logger.git;a=shortlog;h=refs/heads/get-recent-messages-async
+ /* messages will be NULL, we can just continue */ This is probably fine in reality, but I think it'd be best to at least add the documentation to tpl_log_manager_get_messages_for_date_async_finish() that guarantees its return value is NULL if there was any error. Preferably, I'd also set messages to NULL just below that comment just to be explict and safer (and update the comment according). } +static void +_lookup_next_date (RecentMessagesContext *ctx) +{ Telepathy Style has two blank lines between function declarations. + } + +} (in _lookup_next_date()): extraneous blank line + ctx->ptr = g_list_last (ctx->dates); Since this relies upon the chronological ordering of the return value of tpl_log_manager_get_dates(), could you update the documentation for that function to make that guarantee? + if (ctx->dates == NULL) + { + error = g_error_new_literal (TPL_DBUS_SERVICE_ERROR, + TPL_DBUS_SERVICE_ERROR_FAILED, "Error during date list retrieving, " + "probably the account path or the identifier does not exist"); + dbus_g_method_return_error (ctx->context, error); + goto out; + } + + _lookup_next_date (ctx); + +out: + + g_clear_error (&error); +} Looks like you're leaking the ctx and its allocated members in case ctx->dates is NULL. If you agree with the changes, feel free to just make them and merge.
(In reply to comment #1) > + /* messages will be NULL, we can just continue */ > > Preferably, I'd also set messages to NULL just below that comment just to be > explict and safer (and update the comment according). Yeah. Good call. > + ctx->ptr = g_list_last (ctx->dates); > > Since this relies upon the chronological ordering of the return value of > tpl_log_manager_get_dates(), could you update the documentation for that > function to make that guarantee? The function builds sorted lists, but we actually intend to update the function at some point to allow the list to be presorted in either direction (based on an explicit enum). Will leave it for now. > Looks like you're leaking the ctx and its allocated members in case ctx->dates > is NULL. Good catch. > If you agree with the changes, feel free to just make them and merge. Thanks. Done.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.