Bug 32902

> +++ b/telepathy-logger/log-store-pidgin-internal.h
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright (C) 2008-2009 Collabora Ltd.
> + *

We are in 2011, same in .c file.

>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. */

This escaping is defined in RFC3986 (http://www.ietf.org/rfc/rfc3986.txt) and is implemented in GLib, see g_uri_escape_string/g_uri_unescape_string. There is also helpers for filesname to/from URI.

>+static GList *
>+log_store_pidgin_get_dates (EmpathyLogStore *self,
>+  if (directory == NULL)
>+    {
>+      g_free (directory);
>+      return NULL;
>+    }
No need to free directory, it's NULL (remove scope after)

Ok, I'll stop there for now. I see bunch of stuff that should be rebased/squashed before asking for review. Please cleanup your patchset so every changes mean somtehing. We don't need to know about all your intermediate steps, especially for new files.

Applied just the two first fixes, I'll apply the escaping suggestion ASAP.

URL changed to another branch, rebased, stashed, etc: http://git.collabora.co.uk/?p=user/kalfa/telepathy-logger.git;a=shortlog;h=refs/heads/pidgin_store_allinone

That should be suitable.

>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.

*** Bug 26907 has been marked as a duplicate of this bug. ***

> >+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;
> >+    }

I see this problem only in get_dates (fixed).

> 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 ?

Not 100% accurate, direction is anyway useless and will be removed, but so far I'll set it only if is_html==TRUE, which means I can deduct the direction from the logs. Otherwise leave it to NONE.

The same is valid for tpl_entity_set_entity_type(), with the difference that, when is_html==FALSE, it's not possible to understand if the log is actually being send by SELF or not.
This for example will result in Empathy wrongly colouring the log window as it won't understand the user's log. 

> >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 ?

The way to fix it would be export symbols in a .h just for testing uses.

tp-glib has some testes that use this terrible hack, it's from which I copied it actually. It does not make it any better but I think that it can go with it.

The only side effect is that DEBUG() won't work properly with TPL_DEBUG=testsuite as DEBUG_FLAG has been set in the included file.

(In reply to comment #5)
> > >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 ?
> 
> The way to fix it would be export symbols in a .h just for testing uses.

JFTR, when I did the log manager refactoring I noticed some symbols where in -internal.h headers just for the tests. It probably doesn't hurt to do that...

Ship it !

As the tree has changed significantly I've rebased/fixed everything. I was mainly interested in the tests with real data. So far the new test won't pass and doing this work showed me couple of things I don't like:

- Test should not output debugging by default
- Test should run with valgrind without leaks
- the dbus directoy is annoying since it's Makefile contains bunch of duplicates, I suggest using a DBUS_TEST_EXETRUABLE variable instead
- Tests should always run through the public API (yes the _aynsc/_finish)
- We should probably post-fix test program with -test instead of prefixing them
- etc.

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

(In reply to comment #8)
> As the tree has changed significantly I've rebased/fixed everything. I was
> mainly interested in the tests with real data. So far the new test won't pass
> and doing this work showed me couple of things I don't like:
> 
> - Test should not output debugging by default
> - Test should run with valgrind without leaks
> - the dbus directoy is annoying since it's Makefile contains bunch of
> duplicates, I suggest using a DBUS_TEST_EXETRUABLE variable instead
> - Tests should always run through the public API (yes the _aynsc/_finish)
> - We should probably post-fix test program with -test instead of prefixing them
> - etc.

I updated the branch, skipped couple of listed items since they can be carried later. I decided to keep the dbus folder since it's simplier.

ship it

sidenote: it was hard to see what was my bits and what yours in the logstore itself, but they seem to be ok.

Just remember (you or Emilio) to fix the use the SEARCH flag as flags, before Empathy uses them

Pushed.