Bug 32477

Please review. More branches will follow soon.

http://git.collabora.co.uk/?p=user/pochu/telepathy-logger.git;a=shortlog;h=refs/heads/abstract-log-entries

As the type is construct-only (which totally makes sense), it would be good to
not expose _tpl_entry_set_type() and force to pass it during object creation.
I know that most of this code is not doing that, but best to try to get away
from this bad pattern. :)

Ideally, couldn't we get rid of TplEntryType, make TplEntry abstract and use
inheritance to get the type?

Except that looks good.

(In reply to comment #2)
> As the type is construct-only (which totally makes sense), it would be good to
> not expose _tpl_entry_set_type() and force to pass it during object creation.
> I know that most of this code is not doing that, but best to try to get away
> from this bad pattern. :)

Makes sense.

> Ideally, couldn't we get rid of TplEntryType, make TplEntry abstract and use
> inheritance to get the type?

But this is even better. TplEntry was already an abstract type. I've now got rid of TplEntryType and I'm using TPL_IS_ENTRY_FOO to distinguish between different types. Is that what you mean with using inheritance to get the type?

TplLogStoreInterface exposes 'gboolean chatroom' in many methods. We need to abstract that where it makes sense.

(In reply to comment #4)
> TplLogStoreInterface exposes 'gboolean chatroom' in many methods. We need to
> abstract that where it makes sense.

log-manager.h has some API which uses those methods. We cannot just change them since they are public API, see bug 27270 for the API clean up bug.

(In reply to comment #3)
> But this is even better. TplEntry was already an abstract type. I've now got
> rid of TplEntryType and I'm using TPL_IS_ENTRY_FOO to distinguish between
> different types. Is that what you mean with using inheritance to get the type?

I have just merged that branch.

I've been doing some refactoring / API changes in https://bugs.freedesktop.org/show_bug.cgi?id=27270 to make the log-manager API generic and not chat centric, so it's useful to retrieve call events (and other types when we add them).

I've just rebased my branch on top of the recent log manager changes. This is still WIP:

http://git.collabora.co.uk/?p=user/pochu/telepathy-logger.git;a=shortlog;h=refs/heads/log-calls-32477

Adding a link to my own branch:

http://git.collabora.co.uk/?p=user/nicolas/telepathy-logger.git;a=shortlog;h=refs/heads/call-event

Most of it is good, except storage that will be slightly modified. After discussion with others, the consensus is that we should log whatever Call interface provide (as long as it meaningful for logs). Thus the ENDED state transition emulation need a bit more work, but the idea is there. Any comments will be appreciated.

It's finally ready for review !

+/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */

Not really (in lots of files).

+++ b/telepathy-logger/call-event-internal.h
+#include <telepathy-logger/streamed-media-channel-internal.h>

Seems unnecessary and wrong, remove it.

+++ b/telepathy-logger/call-event.h
+  TPL_CALL_END_REASON_FORWARED,

FORWARDED

+++ b/telepathy-logger/call-event.c
+#define DEBUG_FLAG TPL_DEBUG_LOG_STORE

TPL_DEBUG_LOG_EVENT sounds more appropriate.

+ * An object representing a text log event.

s/text/call/

+    "forwared",

forwarded

+static void
+tpl_call_event_finalize (GObject * obj)
+{
+  G_OBJECT_CLASS (tpl_call_event_parent_class)->finalize (obj);
+}

Don't override finalize for nothing.

+  switch (param_id) {

put the brace on another line

+++ b/telepathy-logger/streamed-media-channel.c
+  /* user_data is a TplChannel instance */
+  tp_proxy_prepare_async (chan, chan_features, proxy_prepared_cb, ctx);

I'm not sure that comment is correct?

+  if (error != NULL)
+    {
+      _tpl_action_chain_terminate (ctx, error);
+      return;
+    }

Add a DEBUG with error->message.

+  if (my_handle == 0)
+      my_handle = tp_connection_get_self_handle (tp_conn);

That should be indented 2 spaces

+      "Unkown",

Unknown

+      /* Workaround missing rejected reason. A call is rejected when the
+       * receiver terminates that call before accepting it, and no other
+       * reason was provided. Also, even if the call was not answered, the
+       * spec inforces that the end_reason must be user_requested */

enforces

+  _tpl_log_manager_add_event (logmanager, TPL_EVENT (call_log), &error);
+
+  if (error != NULL)
+    {
+      PATH_DEBUG (self, "LogStore: %s", error->message);

LogStore seems wrong here. StreamedMediaChannel ?


+  DEBUG ("New call, timestamp=%i-%02i-%02i %02i:%02i:%02i UTC",
+      g_date_time_get_year (self->priv->timestamp),
+      g_date_time_get_month (self->priv->timestamp),
+      g_date_time_get_day_of_month (self->priv->timestamp),
+      g_date_time_get_hour (self->priv->timestamp),
+      g_date_time_get_minute (self->priv->timestamp),
+      g_date_time_get_second (self->priv->timestamp));

g_date_time_format may come handy here.


+ * TplStreamedMediaChannel is actually a subclass of the abstract
+ * TplChannel which is a subclass of TpChannel.

It's rather a subclass of TpChannel, implementing TplChannel.


+++ b/telepathy-logger/entity.c
-      "avatar-token", avatar_token,
+      "avatar-token", avatar_token == NULL ? "" : avatar_token,

I'm not sure of the benefits of that change. Can you explain them to me? It looks a bit ABI break too, though that's a bit borderline.

+++ b/telepathy-logger/log-store-xml.c

+  log_str = g_strdup_printf ("<call time='%s' "
+      "id='%s' name='%s' isuser='%s' token='%s' "
+      "duration='%" G_GINT64_FORMAT "' "
+      "actor='%s' actortype='%s' "
+      "actorname='%s' actortoken='%s' "
+      "reason='%s' detail='%s'/>\n"

what is actor here? Also, is this multiconference-ready? If not, I guess that's not too important. We can probably extend it when we start supporting that in any clients, and maybe by then we're already using sqlite...

+        tpl_entity_get_avatar_token (actor) == NULL ?
+           "" : tpl_entity_get_avatar_token (actor),

I guess you should escape that too.

+++ b/telepathy-logger/call-channel.c
+G_DEFINE_TYPE_WITH_CODE (TplCallChannel, _tpl_call_channel,
+    TP_TYPE_CHANNEL,
+    G_IMPLEMENT_INTERFACE (TPL_TYPE_CHANNEL, tpl_call_channel_iface_init))

Have you thought about inheriting from TpyCallChannel instead as you did with TpTextChannel? That could ease our job.

+#define TPL_IFACE_CHANNEL_TYPE_CALL \
+  "org.freedesktop.Telepathy.Channel.Type.Call.DRAFT"
+#define TPL_IFACE_QUARK_CHANNEL_TYPE_CALL tpl_get_channel_type_call_interface ()

+static GQuark
+tpl_get_channel_type_call_interface (void)
+{
+  static GQuark interface = 0;
+
+  if (G_UNLIKELY (interface == 0))
+    interface = g_quark_from_static_string (TPL_IFACE_CHANNEL_TYPE_CALL);
+
+  return interface;
+}

We should use the tpy APIs here, e.g. TPY_IFACE_QUARK_CHANNEL_TYPE_CALL.

+typedef enum
+{
+  PENDING_INITIATOR_STATE = 1,
+  PENDING_RECEIVER_STATE,
+  ACCEPTED_STATE,
+  ENDED_STATE,
+} CallState;

TpyCallState


+  /* user_data is a TplChannel instance */
+  tp_proxy_prepare_async (chan, chan_features, proxy_prepared_cb, ctx);

not sure about the comment

+  GList *it;

It looks so much readable to do *l, but that's a personal preference :)

+      "Unkown",

Again :p


+    default:
+      /* just wait */
+      break;

a DEBUG may be useful there


+  dbus_g_proxy_add_signal (proxy,
+    "CallStateChanged",
+    G_TYPE_UINT,
+    G_TYPE_UINT,
+    dbus_g_type_get_struct ("GValueArray",
+        G_TYPE_UINT,
+        G_TYPE_UINT,
+        G_TYPE_STRING,
+        G_TYPE_INVALID),
+    dbus_g_type_get_map ("GHashTable", G_TYPE_STRING, G_TYPE_VALUE),
+    G_TYPE_INVALID);
+
+  dbus_g_proxy_add_signal (proxy,
+      "CallMembersChanged",
+      dbus_g_type_get_map ("GHashTable", G_TYPE_UINT, G_TYPE_UINT),
+      DBUS_TYPE_G_UINT_ARRAY,
+      G_TYPE_INVALID);
+
+  dbus_g_proxy_connect_signal (proxy,
+     "CallStateChanged",
+      G_CALLBACK (call_state_changed_cb), self, NULL);
+
+  dbus_g_proxy_connect_signal (proxy,
+     "CallMembersChanged",
+      G_CALLBACK (call_members_changed_cb), self, NULL);
+
+  tp_g_signal_connect_object (TP_CHANNEL (self), "group-members-changed",
+      G_CALLBACK (chan_members_changed_cb), self, 0);

Use TpyCallChannel and connect to its GObject signals. If something is missing, file a bug. There's also the tpy_cli_* APIs that would be better than this calls.

+  DEBUG ("New call, timestamp=%i-%02i-%02i %02i:%02i:%02i UTC",

Same comment as before


+  dbus_g_object_register_marshaller (tpl_marshal_VOID__UINT_UINT_BOXED_BOXED,
+      G_TYPE_NONE,
+      G_TYPE_UINT, G_TYPE_UINT, G_TYPE_BOXED, G_TYPE_BOXED,
+      G_TYPE_INVALID);
+
+  dbus_g_object_register_marshaller (tpl_marshal_VOID__BOXED_BOXED,
+      G_TYPE_NONE,
+      G_TYPE_BOXED, G_TYPE_BOXED,
+      G_TYPE_INVALID);

And I guess you can then remove that.

That's for the first few commits, I'll continue tomorrow.

(In reply to comment #11)
> +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */
> 
> Not really (in lots of files).

Right, it seems wrong, I don't really know which editor uses that, this has been copy-pasted over-and-over. Shall we simply remove them all ?

> 
> +++ b/telepathy-logger/call-event-internal.h
> +#include <telepathy-logger/streamed-media-channel-internal.h>
> 
> Seems unnecessary and wrong, remove it.

Fixed.

> 
> +++ b/telepathy-logger/call-event.h
> +  TPL_CALL_END_REASON_FORWARED,
> 
> FORWARDED

Fixed.

> 
> +++ b/telepathy-logger/call-event.c
> +#define DEBUG_FLAG TPL_DEBUG_LOG_STORE
> 
> TPL_DEBUG_LOG_EVENT sounds more appropriate.

Copy paste ... fixed.

> 
> + * An object representing a text log event.
> 
> s/text/call/

Fixed

> 
> +    "forwared",
> 
> forwarded

Been consistent in my typos, fixed. Also found missing ',' causing crash, same in Call.

> 
> +static void
> +tpl_call_event_finalize (GObject * obj)
> +{
> +  G_OBJECT_CLASS (tpl_call_event_parent_class)->finalize (obj);
> +}
> 
> Don't override finalize for nothing.

Hmm, made me notice I don't free anything. Replaced with _dispose and
now tp_clear_object() and tp_clear_pointer() the end_actor and detailed_end_reason. Will run this under valgrind and fix remaining if they exists.

> 
> +  switch (param_id) {
> 
> put the brace on another line

Right, fixed.

> 
> +++ b/telepathy-logger/streamed-media-channel.c
> +  /* user_data is a TplChannel instance */
> +  tp_proxy_prepare_async (chan, chan_features, proxy_prepared_cb, ctx);
> 
> I'm not sure that comment is correct?

Actually the user_data is NULL, we use the object stored in the chain. This is copy paste from Text channel, I've removed it from Text/Call/StreamedMediaChannel.

> 
> +  if (error != NULL)
> +    {
> +      _tpl_action_chain_terminate (ctx, error);
> +      return;
> +    }
> 
> Add a DEBUG with error->message.

We give the error to the caller, it's up to the caller to decide if the error is worth logging. In this case the caller do log it.

> 
> +  if (my_handle == 0)
> +      my_handle = tp_connection_get_self_handle (tp_conn);
> 
> That should be indented 2 spaces

Fixed.

> 
> +      "Unkown",
> 
> Unknown

I've fixed them all.

> 
> +      /* Workaround missing rejected reason. A call is rejected when the
> +       * receiver terminates that call before accepting it, and no other
> +       * reason was provided. Also, even if the call was not answered, the
> +       * spec inforces that the end_reason must be user_requested */
> 
> enforces
> 
> +  _tpl_log_manager_add_event (logmanager, TPL_EVENT (call_log), &error);
> +
> +  if (error != NULL)
> +    {
> +      PATH_DEBUG (self, "LogStore: %s", error->message);
> 
> LogStore seems wrong here. StreamedMediaChannel ?

Fixed.

> 
> 
> +  DEBUG ("New call, timestamp=%i-%02i-%02i %02i:%02i:%02i UTC",
> +      g_date_time_get_year (self->priv->timestamp),
> +      g_date_time_get_month (self->priv->timestamp),
> +      g_date_time_get_day_of_month (self->priv->timestamp),
> +      g_date_time_get_hour (self->priv->timestamp),
> +      g_date_time_get_minute (self->priv->timestamp),
> +      g_date_time_get_second (self->priv->timestamp));
> 
> g_date_time_format may come handy here.

Didn't want to alloc/free, but I agree it looks ugly, fixed, also fixed in CallChannel.

> 
> 
> + * TplStreamedMediaChannel is actually a subclass of the abstract
> + * TplChannel which is a subclass of TpChannel.
> 
> It's rather a subclass of TpChannel, implementing TplChannel.

Also fixed in CallChannel, was changed very recently.

> 
> 
> +++ b/telepathy-logger/entity.c
> -      "avatar-token", avatar_token,
> +      "avatar-token", avatar_token == NULL ? "" : avatar_token,
> 
> I'm not sure of the benefits of that change. Can you explain them to me? It
> looks a bit ABI break too, though that's a bit borderline.

The reason is that for XML log store there is no difference between NULL and empty string. Meaning that if you write NULL you will endup reading "". I'm changing un-obfuscate the unit test and make the representation unique. This change does not break API since it affect the writing part (you can't create an event from the outside). The reading part was always getting "" anyway.

> 
> +++ b/telepathy-logger/log-store-xml.c
> 
> +  log_str = g_strdup_printf ("<call time='%s' "
> +      "id='%s' name='%s' isuser='%s' token='%s' "
> +      "duration='%" G_GINT64_FORMAT "' "
> +      "actor='%s' actortype='%s' "
> +      "actorname='%s' actortoken='%s' "
> +      "reason='%s' detail='%s'/>\n"
> 
> what is actor here? Also, is this multiconference-ready? If not, I guess that's
> not too important. We can probably extend it when we start supporting that in
> any clients, and maybe by then we're already using sqlite...

It's the actor as found in the Call_State_Reason, see http://telepathy.freedesktop.org/spec/Channel_Type_Call.html#Struct:Call_State_Reason . It's required to interpret the reason and detail. This actor could be anyone in the conference room, e.g. an room admin kicked you. I'm not sure what's your point about multiconference-ready + sqlite, if you mean interfaces Conference, Splittable and MergeableConference, no they are not represented.

> 
> +        tpl_entity_get_avatar_token (actor) == NULL ?
> +           "" : tpl_entity_get_avatar_token (actor),
> 
> I guess you should escape that too.

escape ? that too ? Maybe you mean it's not required since I now create entity with "" as default avatar_token ? I agree, but it's also an extra safety in the case somebody break it in the future.

> 
> +++ b/telepathy-logger/call-channel.c
> +G_DEFINE_TYPE_WITH_CODE (TplCallChannel, _tpl_call_channel,
> +    TP_TYPE_CHANNEL,
> +    G_IMPLEMENT_INTERFACE (TPL_TYPE_CHANNEL, tpl_call_channel_iface_init))
> 
> Have you thought about inheriting from TpyCallChannel instead as you did with
> TpTextChannel? That could ease our job.

Yeah, as I already explained, I pretty much dislike the submodules and having unstable static libraries being put in more then 1 project. I'll port it, but unstable library like Yell are not my thing.

> 
> +#define TPL_IFACE_CHANNEL_TYPE_CALL \
> +  "org.freedesktop.Telepathy.Channel.Type.Call.DRAFT"
> +#define TPL_IFACE_QUARK_CHANNEL_TYPE_CALL tpl_get_channel_type_call_interface
> ()
> 
> +static GQuark
> +tpl_get_channel_type_call_interface (void)
> +{
> +  static GQuark interface = 0;
> +
> +  if (G_UNLIKELY (interface == 0))
> +    interface = g_quark_from_static_string (TPL_IFACE_CHANNEL_TYPE_CALL);
> +
> +  return interface;
> +}
> 
> We should use the tpy APIs here, e.g. TPY_IFACE_QUARK_CHANNEL_TYPE_CALL.
> 
> +typedef enum
> +{
> +  PENDING_INITIATOR_STATE = 1,
> +  PENDING_RECEIVER_STATE,
> +  ACCEPTED_STATE,
> +  ENDED_STATE,
> +} CallState;
> 
> TpyCallState
> 
> 
> +  /* user_data is a TplChannel instance */
> +  tp_proxy_prepare_async (chan, chan_features, proxy_prepared_cb, ctx);
> 
> not sure about the comment
> 
> +  GList *it;
> 
> It looks so much readable to do *l, but that's a personal preference :)

Btw, 'it' mean iterator, pretty much the same as 'l' for list.

> 
> +      "Unkown",
> 
> Again :p
> 
> 
> +    default:
> +      /* just wait */
> +      break;
> 
> a DEBUG may be useful there

From the logger stand point, the other states does not mean anything, have a look at the CM traces to debug CM.

> 
> 
> +  dbus_g_proxy_add_signal (proxy,
> +    "CallStateChanged",
> +    G_TYPE_UINT,
> +    G_TYPE_UINT,
> +    dbus_g_type_get_struct ("GValueArray",
> +        G_TYPE_UINT,
> +        G_TYPE_UINT,
> +        G_TYPE_STRING,
> +        G_TYPE_INVALID),
> +    dbus_g_type_get_map ("GHashTable", G_TYPE_STRING, G_TYPE_VALUE),
> +    G_TYPE_INVALID);
> +
> +  dbus_g_proxy_add_signal (proxy,
> +      "CallMembersChanged",
> +      dbus_g_type_get_map ("GHashTable", G_TYPE_UINT, G_TYPE_UINT),
> +      DBUS_TYPE_G_UINT_ARRAY,
> +      G_TYPE_INVALID);
> +
> +  dbus_g_proxy_connect_signal (proxy,
> +     "CallStateChanged",
> +      G_CALLBACK (call_state_changed_cb), self, NULL);
> +
> +  dbus_g_proxy_connect_signal (proxy,
> +     "CallMembersChanged",
> +      G_CALLBACK (call_members_changed_cb), self, NULL);
> +
> +  tp_g_signal_connect_object (TP_CHANNEL (self), "group-members-changed",
> +      G_CALLBACK (chan_members_changed_cb), self, 0);
> 
> Use TpyCallChannel and connect to its GObject signals. If something is missing,
> file a bug. There's also the tpy_cli_* APIs that would be better than this
> calls.
> 
> +  DEBUG ("New call, timestamp=%i-%02i-%02i %02i:%02i:%02i UTC",
> 
> Same comment as before
> 
> 
> +  dbus_g_object_register_marshaller (tpl_marshal_VOID__UINT_UINT_BOXED_BOXED,
> +      G_TYPE_NONE,
> +      G_TYPE_UINT, G_TYPE_UINT, G_TYPE_BOXED, G_TYPE_BOXED,
> +      G_TYPE_INVALID);
> +
> +  dbus_g_object_register_marshaller (tpl_marshal_VOID__BOXED_BOXED,
> +      G_TYPE_NONE,
> +      G_TYPE_BOXED, G_TYPE_BOXED,
> +      G_TYPE_INVALID);
> 
> And I guess you can then remove that.
> 
> That's for the first few commits, I'll continue tomorrow.

Ok, I'll have a look at having static yell copy inside logger, you don't have to mention each part that would go away, I understand very well what I did, and what the code generator puts in tpy_cli, and what the TpyCallChannel wrapper should do.

I'm still highly concerned by recent issues found in the call API and potential effect on the log format. I'll poke people tomorrow to figure-out if there is intention to change the call state reasons, detailed reasons, etc.

(In reply to comment #12)
> (In reply to comment #11)
> > +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */
> > 
> > Not really (in lots of files).
> 
> Right, it seems wrong, I don't really know which editor uses that, this has
> been copy-pasted over-and-over. Shall we simply remove them all ?

I think vi (or Emacs), but the problem is that it's wrong, that says the coding style uses tabs, but we use two spaces... so just get rid of them everywhere, if they are like that.

> > +static void
> > +tpl_call_event_finalize (GObject * obj)
> > +{
> > +  G_OBJECT_CLASS (tpl_call_event_parent_class)->finalize (obj);
> > +}
> > 
> > Don't override finalize for nothing.
> 
> Hmm, made me notice I don't free anything. Replaced with _dispose and
> now tp_clear_object() and tp_clear_pointer() the end_actor and
> detailed_end_reason. Will run this under valgrind and fix remaining if they
> exists.

make check-valgrind could be useful here.

> > +  if (error != NULL)
> > +    {
> > +      _tpl_action_chain_terminate (ctx, error);
> > +      return;
> > +    }
> > 
> > Add a DEBUG with error->message.
> 
> We give the error to the caller, it's up to the caller to decide if the error
> is worth logging. In this case the caller do log it.

You're right.

> > +  DEBUG ("New call, timestamp=%i-%02i-%02i %02i:%02i:%02i UTC",
> > +      g_date_time_get_year (self->priv->timestamp),
> > +      g_date_time_get_month (self->priv->timestamp),
> > +      g_date_time_get_day_of_month (self->priv->timestamp),
> > +      g_date_time_get_hour (self->priv->timestamp),
> > +      g_date_time_get_minute (self->priv->timestamp),
> > +      g_date_time_get_second (self->priv->timestamp));
> > 
> > g_date_time_format may come handy here.
> 
> Didn't want to alloc/free, but I agree it looks ugly, fixed, also fixed in
> CallChannel.

Yeah, that's why I said may. I just wanted to point it out and let you decide if it was more clear and worth it or not.

> > +++ b/telepathy-logger/entity.c
> > -      "avatar-token", avatar_token,
> > +      "avatar-token", avatar_token == NULL ? "" : avatar_token,
> > 
> > I'm not sure of the benefits of that change. Can you explain them to me? It
> > looks a bit ABI break too, though that's a bit borderline.
> 
> The reason is that for XML log store there is no difference between NULL and
> empty string. Meaning that if you write NULL you will endup reading "". I'm
> changing un-obfuscate the unit test and make the representation unique. This
> change does not break API since it affect the writing part (you can't create an
> event from the outside). The reading part was always getting "" anyway.

ok

> > +++ b/telepathy-logger/log-store-xml.c
> > 
> > +  log_str = g_strdup_printf ("<call time='%s' "
> > +      "id='%s' name='%s' isuser='%s' token='%s' "
> > +      "duration='%" G_GINT64_FORMAT "' "
> > +      "actor='%s' actortype='%s' "
> > +      "actorname='%s' actortoken='%s' "
> > +      "reason='%s' detail='%s'/>\n"
> > 
> > what is actor here? Also, is this multiconference-ready? If not, I guess that's
> > not too important. We can probably extend it when we start supporting that in
> > any clients, and maybe by then we're already using sqlite...
> 
> It's the actor as found in the Call_State_Reason, see
> http://telepathy.freedesktop.org/spec/Channel_Type_Call.html#Struct:Call_State_Reason
> . It's required to interpret the reason and detail. This actor could be anyone
> in the conference room, e.g. an room admin kicked you.

ok, so it's who ended the call.

> I'm not sure what's your
> point about multiconference-ready + sqlite, if you mean interfaces Conference,
> Splittable and MergeableConference, no they are not represented.

With Call we can (if the client supports it) have calls with several people at the same time. I was wondering if this was designed with that in mind, to e.g. log who participated. Doesn't need to be done now as no clients support that yet, but it'd be good if we make sure we can easily extend the log format in the future to support this use case. The sqlite comment is that maybe when we need to support that, we're using the sqlite log store instead of the xml one to write events, and so the xml format we're using here doesn't matter too much.

> > +        tpl_entity_get_avatar_token (actor) == NULL ?
> > +           "" : tpl_entity_get_avatar_token (actor),
> > 
> > I guess you should escape that too.
> 
> escape ? that too ? Maybe you mean it's not required since I now create entity
> with "" as default avatar_token ? I agree, but it's also an extra safety in the
> case somebody break it in the future.

I said "too" because a few lines above you escape another avatar-token. So either escape both or don't escape any, I'd say.

> > +++ b/telepathy-logger/call-channel.c
> > +G_DEFINE_TYPE_WITH_CODE (TplCallChannel, _tpl_call_channel,
> > +    TP_TYPE_CHANNEL,
> > +    G_IMPLEMENT_INTERFACE (TPL_TYPE_CHANNEL, tpl_call_channel_iface_init))
> > 
> > Have you thought about inheriting from TpyCallChannel instead as you did with
> > TpTextChannel? That could ease our job.
> 
> Yeah, as I already explained, I pretty much dislike the submodules and having
> unstable static libraries being put in more then 1 project. I'll port it, but
> unstable library like Yell are not my thing.

Yes, ideally it'd be in tp-glib (which will be when it becomes stable). The stability point shouldn't be too important as we're not breaking it unless we bump the draft, in which case it's a different channel and you're not logging them anymore.

> > +#define TPL_IFACE_CHANNEL_TYPE_CALL \
> > +  "org.freedesktop.Telepathy.Channel.Type.Call.DRAFT"
> > +#define TPL_IFACE_QUARK_CHANNEL_TYPE_CALL tpl_get_channel_type_call_interface
> > ()
> > 
> > +static GQuark
> > +tpl_get_channel_type_call_interface (void)
> > +{
> > +  static GQuark interface = 0;
> > +
> > +  if (G_UNLIKELY (interface == 0))
> > +    interface = g_quark_from_static_string (TPL_IFACE_CHANNEL_TYPE_CALL);
> > +
> > +  return interface;
> > +}
> > 
> > We should use the tpy APIs here, e.g. TPY_IFACE_QUARK_CHANNEL_TYPE_CALL.
> > 
> > +typedef enum
> > +{
> > +  PENDING_INITIATOR_STATE = 1,
> > +  PENDING_RECEIVER_STATE,
> > +  ACCEPTED_STATE,
> > +  ENDED_STATE,
> > +} CallState;
> > 
> > TpyCallState
> > 
> > 
> > +  /* user_data is a TplChannel instance */
> > +  tp_proxy_prepare_async (chan, chan_features, proxy_prepared_cb, ctx);
> > 
> > not sure about the comment
> > 
> > +  GList *it;
> > 
> > It looks so much readable to do *l, but that's a personal preference :)
> 
> Btw, 'it' mean iterator, pretty much the same as 'l' for list.

Ah, I thought it meant 'it' as in 'it' :-) I still prefer l, but that's a bit more clear now.

+++ b/telepathy-logger/log-store-xml.c
+  if (!regex)

if (regex != NULL)

+      if (event != NULL)
+        events = g_list_append (events, event);

Just thought we could do g_list_prepend() as an optimization, and if order matters (not sure it does), g_list_reverse at the end of the function. We can probably do this in more places.

+ This implementation is not correct on purpose to ensure a constent time
+ of execution. See bug #35549 for more details.

constant (in a commit message)

+  /* FIXME This method is exposed synchronously in the log manager API and
+   * thus we need a constant time reply. The implementation is not 100%
+   * correct here, but provide this constant time. See fd.o but #35549. */

s/but/bug/

+ Make regex mathing in file ...

matching (in a commit message)

+      if (type_mask & (TPL_EVENT_MASK_TEXT | TPL_EVENT_MASK_CALL)
+          || log_store_xml_match_in_file (filename, regex))

I think this should be &&

+++ b/tests/dbus/test-log-manager.c
+  /* We got 6 events in old Empathy and 6 in new TpLogger storage */
+  g_assert_cmpint (g_list_length (fixture->ret), ==, 10);

Hmm. 6 + 6 = 12, not 10. Something is wrong or just confusing there.

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */
> > > 
> > > Not really (in lots of files).
> > 
> > Right, it seems wrong, I don't really know which editor uses that, this has
> > been copy-pasted over-and-over. Shall we simply remove them all ?
> 
> I think vi (or Emacs), but the problem is that it's wrong, that says the coding
> style uses tabs, but we use two spaces... so just get rid of them everywhere,
> if they are like that.

Ok, removed.

> 
> > > +static void
> > > +tpl_call_event_finalize (GObject * obj)
> > > +{
> > > +  G_OBJECT_CLASS (tpl_call_event_parent_class)->finalize (obj);
> > > +}
> > > 
> > > Don't override finalize for nothing.
> > 
> > Hmm, made me notice I don't free anything. Replaced with _dispose and
> > now tp_clear_object() and tp_clear_pointer() the end_actor and
> > detailed_end_reason. Will run this under valgrind and fix remaining if they
> > exists.
> 
> make check-valgrind could be useful here.

I've run it, this was the only leak the unit tests covers.


> > > +        tpl_entity_get_avatar_token (actor) == NULL ?
> > > +           "" : tpl_entity_get_avatar_token (actor),
> > > 
> > > I guess you should escape that too.
> > 
> > escape ? that too ? Maybe you mean it's not required since I now create entity
> > with "" as default avatar_token ? I agree, but it's also an extra safety in the
> > case somebody break it in the future.
> 
> I said "too" because a few lines above you escape another avatar-token. So
> either escape both or don't escape any, I'd say.

Ah escaping in order to put it in the XML file, make sense. I also removed the if NULL for avatar token and alias has this is no longer possible.

> 
> > > +++ b/telepathy-logger/call-channel.c
> > > +G_DEFINE_TYPE_WITH_CODE (TplCallChannel, _tpl_call_channel,
> > > +    TP_TYPE_CHANNEL,
> > > +    G_IMPLEMENT_INTERFACE (TPL_TYPE_CHANNEL, tpl_call_channel_iface_init))
> > > 
> > > Have you thought about inheriting from TpyCallChannel instead as you did with
> > > TpTextChannel? That could ease our job.
> > 
> > Yeah, as I already explained, I pretty much dislike the submodules and having
> > unstable static libraries being put in more then 1 project. I'll port it, but
> > unstable library like Yell are not my thing.
> 
> Yes, ideally it'd be in tp-glib (which will be when it becomes stable). The
> stability point shouldn't be too important as we're not breaking it unless we
> bump the draft, in which case it's a different channel and you're not logging
> them anymore.

I tried, but it didn't work, I've stored the work in a branch and I'll come back later to see what's wrong with TpyCallChannel implementation.

(In reply to comment #14)
> +++ b/telepathy-logger/log-store-xml.c
> +  if (!regex)
> 
> if (regex != NULL)
> 
> +      if (event != NULL)
> +        events = g_list_append (events, event);
> 
> Just thought we could do g_list_prepend() as an optimization, and if order
> matters (not sure it does), g_list_reverse at the end of the function. We can
> probably do this in more places.

Most cases switching to prepend was fine except in XML store where I've used a GQueue.

> 
> + This implementation is not correct on purpose to ensure a constent time
> + of execution. See bug #35549 for more details.
> 
> constant (in a commit message)

Fixed.

> 
> +  /* FIXME This method is exposed synchronously in the log manager API and
> +   * thus we need a constant time reply. The implementation is not 100%
> +   * correct here, but provide this constant time. See fd.o but #35549. */
> 
> s/but/bug/
> 
> + Make regex mathing in file ...
> 
> matching (in a commit message)

Fixed.

> 
> +      if (type_mask & (TPL_EVENT_MASK_TEXT | TPL_EVENT_MASK_CALL)
> +          || log_store_xml_match_in_file (filename, regex))
> 
> I think this should be &&

Not exactly, I was trying to optimize the case all supported types are selected to avoid reading the file. But this code is not right, I've fixed it. Also, I just notice the filename was not an absolute path, the broken optimization was hiding this bug, I also fixed that.

> 
> +++ b/tests/dbus/test-log-manager.c
> +  /* We got 6 events in old Empathy and 6 in new TpLogger storage */
> +  g_assert_cmpint (g_list_length (fixture->ret), ==, 10);
> 
> Hmm. 6 + 6 = 12, not 10. Something is wrong or just confusing there.

Just grepped and got 10, 4 in Empathy, 4 in TpLogger and 2 in Pidgin, so I'm fixing doc.

Everything has been squashed into http://git.collabora.co.uk/?p=user/nicolas/telepathy-logger.git;a=shortlog;h=refs/heads/call-event. Only remaining is to add compile option that will enable call observer only if enabled at compile time. This is requires as Call API is not stable, thus this feature in the logger must be considered experimental.

Some items mentioned here where not directly related to CallEvent, so I've brought them together in trivia branch. Can be reviewed at:

http://git.collabora.co.uk/?p=user/nicolas/telepathy-logger.git;a=shortlog;h=refs/heads/trivia

+if ENABLE_CALL
+clientfile_DATA = Logger.call.client
+EXTRA_DIST = Logger.client
+else # ENABLE_CALL
 clientfile_DATA = Logger.client
+EXTRA_DIST = Logger.call.client
+endif # ENABLE_CALL

You could also have a Logger.client.in and generate Logger.client from it, with the call channels on it if ENABLE_CALL, so you don't have two copies of the file. Not a big issue though, as we'll remove this when Call is stable.

Looks good to me.

(In reply to comment #15)
> > Yes, ideally it'd be in tp-glib (which will be when it becomes stable). The
> > stability point shouldn't be too important as we're not breaking it unless we
> > bump the draft, in which case it's a different channel and you're not logging
> > them anymore.
> 
> I tried, but it didn't work, I've stored the work in a branch and I'll come
> back later to see what's wrong with TpyCallChannel implementation.

I'd love to hear about problems with it in the form of bug reports :-)

Pushed, will be in 0.2.7.