Bug 33015

I started porting to Message interface (removing a lot of code), then figure-out that cache model was badly adapted. So I dropped all the cache code, ported to TpTextChannel (removing more code again) and re-implement the cache in a maintainable way. The result is a slightly bigger change then expected, but I do think it's for the best:

        43 files changed, 1040 insertions(+), 4518 deletions(-)

telepathy-logger/text-channel.c:

+  else if (!tp_proxy_has_interface_by_id (TP_PROXY (chan),
+        TP_IFACE_QUARK_CHANNEL_INTERFACE_MESSAGES))
+    {
+      error = g_error_new (TPL_TEXT_CHANNEL_ERROR,
+          TPL_TEXT_CHANNEL_ERROR_NEED_MESSAGE_INTERFACE,
+          "The text channel does not implement Message interface.");
+      _tpl_action_chain_terminate (ctx, error);
+      g_error_free (error);
+    }
+
+  _tpl_action_chain_continue (ctx);
+}

You should return after g_error_free


I don't like http://git.collabora.co.uk/?p=user/nicolas/telepathy-logger.git;a=commitdiff;h=18be97017eb658b14c5069bb17c03145f8dc3eb4

You should still print that the event may not have been logged. To not crash, you can do loc_error ? loc_error->message : "something", or an if (loc_error) else, or whatever, but still do a CRITICAL.

Building it:
text-channel.c: In function ‘pendingproc_store_pending_messages’:
text-channel.c:568:26: error: ‘cached’ may be used uninitialized in this function

The easy fix is to bring cached = cached_it->data; a bit up.

Most of my other comments are fixed in later commits. I'm afraid of the regressions this could bring, but the test suite passes, so if something breaks, we know the test suite is not as good as we would like it to be and need to improve it.

(In reply to comment #2)
> telepathy-logger/text-channel.c:
> 
> +  else if (!tp_proxy_has_interface_by_id (TP_PROXY (chan),
> +        TP_IFACE_QUARK_CHANNEL_INTERFACE_MESSAGES))
> +    {
> +      error = g_error_new (TPL_TEXT_CHANNEL_ERROR,
> +          TPL_TEXT_CHANNEL_ERROR_NEED_MESSAGE_INTERFACE,
> +          "The text channel does not implement Message interface.");
> +      _tpl_action_chain_terminate (ctx, error);
> +      g_error_free (error);
> +    }
> +
> +  _tpl_action_chain_continue (ctx);
> +}
> 
> You should return after g_error_free

Oops, will fix.

> 
> 
> I don't like
> http://git.collabora.co.uk/?p=user/nicolas/telepathy-logger.git;a=commitdiff;h=18be97017eb658b14c5069bb17c03145f8dc3eb4
> 
> You should still print that the event may not have been logged. To not crash,
> you can do loc_error ? loc_error->message : "something", or an if (loc_error)
> else, or whatever, but still do a CRITICAL.

My logic was the if you get that, it's because you have hit a g_return_val_if_fail() which print stuff in the critical level anyway. I personally find it annoying when too many errors get displayed for the same thing. But I don't mind doing it this way, and clearly state that an event was lost.

> 
> Building it:
> text-channel.c: In function ‘pendingproc_store_pending_messages’:
> text-channel.c:568:26: error: ‘cached’ may be used uninitialized in this
> function
> 
> The easy fix is to bring cached = cached_it->data; a bit up.

Oops! I've moved the initialization in between the two if. This is indeed a really rare case, I'm trying to produce that case at the moment, but it's hard to do manually. 

> 
> Most of my other comments are fixed in later commits. I'm afraid of the
> regressions this could bring, but the test suite passes, so if something
> breaks, we know the test suite is not as good as we would like it to be and
> need to improve it.

Well I can guaranty tests need's improvement, but I'm still removing 2K lines of potentially untested code.

Pushed upstream.

Oh, and will be in next release 0.2.6