[Bug 32902] review Pidgin log store and dbus testsuite

[bugzilla-daemon at freedesktop.org]

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

Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|review- (need to cleanup    |review- (nothing major,
                   |patchset)                   |need fixes)

--- Comment #3 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> 2011-01-10 14:13:59 PST ---
>diff --git a/telepathy-logger/log-store-pidgin.c b/telepathy-logger/log-store-pidgin.c
>+static void
>+tpl_log_store_pidgin_dispose (GObject *self)
>+{
>+  TplLogStorePidginPriv *priv = TPL_LOG_STORE_PIDGIN (self)->priv;
>+
>+  if (priv->basedir != NULL)
>+    {
>+      g_free (priv->basedir);
>+      priv->basedir = NULL;
>+    }
>+
>+  if (priv->name != NULL)
>+    {
>+      g_free (priv->name);
>+      priv->name = NULL;
>+    }

name and basedir are string, you don't need to do NULL check here, just free
them.

>+/* returns am absolute path for the base directory of LogStore */

"am => an" and maybe "for the LogStore's base directory" ?

>+  /* set default based on name if NULL, see prop's comment about it in
>+   * class_init method */
I think I understand this comment but it would benefit some work.

One general style note, the number of white lines between function does not
seem constant, I think it should be two.

>diff --git a/telepathy-logger/log-store-pidgin.c b/telepathy-logger/log-store->pidgin.c
>+static gchar *
>+log_store_pidgin_get_dir (EmpathyLogStore *self,
>+  /* The escaping behaviour here was taken from Pidgin, obviously. */

I already mentionned this one, just to make sure you don't forget to use glib
utility instead.

>+static GDate *
>+log_store_pidgin_get_time (const gchar *filename)
>+  dir = g_dir_open (directory, 0, NULL);
>+  if (dir == NULL)
>+    {
>+      DEBUG ("Could not open directory:'%s'", directory);
>+      return NULL;
>+    }
You need to free 'directory' inside the if. Also same thing in
log_store_pidgin_get_dates().

>+          /* FIXME do we need to set the entity type as well? */
>+
>+          /* FIXME a better way to obtain a log-id from purlple? */
>+          if (TRUE)
>+            {
Could you elaborate more about those, also I don't like so much this runtime if
(TRUE).

>+              TPL_ENTRY_DIRECTION_NONE);
Can't we get the direction here ?

>diff --git a/tests/dbus/test-tpl-log-store-pidgin.c b/tests/dbus/test-tpl-log-store-pidgin.c
>+++ b/tests/dbus/test-tpl-log-store-pidgin.c
>@@ -0,0 +1,576 @@
>+/* FIXME: hugly kludge: we need to include all the declarations which are used
>+ * by the GInterface and thus not in the -internal.h */
>+#include "../../telepathy-logger/log-store-pidgin.c"

It's really hugly, anything we could do about that ?

Don't forget to rebase about Emilio's refactoring.

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