[Bug 32477] Log Call events

bugzilla-daemon at freedesktop.org bugzilla-daemon at freedesktop.org
Wed Mar 23 18:35:19 CET 2011


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

--- Comment #11 from Emilio Pozuelo Monfort <pochu27 at gmail.com> 2011-03-23 10:35:18 PDT ---
+/* -*- 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.

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



More information about the telepathy-bugs mailing list