[Bug 32477] Log Call events

[bugzilla-daemon at freedesktop.org]

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

--- Comment #13 from Emilio Pozuelo Monfort <pochu27 at gmail.com> 2011-03-24 03:17:54 PDT ---
(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.

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