-       -DG_LOG_DOMAIN=\"libtelepathy-logger\"          \
+       -DG_LOG_DOMAIN=\"tp-logger\"                    \

From looking at Gabble, this should just be 'logger'.

+ * passed as first arg. prepending '_' not not shadow any local variable */

"not not" ?

+  /* observer_channels has been called by the Channel Dispatcher */

Comment belongs inside the brace.

+  const GQuark features[2] = { TP_ACCOUNT_MANAGER_FEATURE_CORE, 0 };

You can use features[] = { ... } here. Alternatively, since you only want the core feature. I think you can just pass NULL.

+      if (!tp_account_is_enabled (acc))
+        continue;
+
+      conn = tp_account_get_connection (acc);

You need to prepare the account before making these requests.

+  TplObserver *observer = tpl_observer_new ();

You should pass this as user_data/weak_obj instead. Also tp_observer_new should probably be renamed to tp_observer_dup.

+  tpl_observer_observe_channels (TP_SVC_CLIENT_OBSERVER (observer),
+      tp_proxy_get_object_path (TP_PROXY (acc)),
+      tp_proxy_get_object_path (TP_PROXY (conn)),
+      channels, NULL, NULL, NULL, NULL);

It's probably worth splitting tpl_observer_observe_channels up so there is a version that takes the prepared objects you already have which can be called by both entry points. It can return a gboolean (whether to return success or error) and take a GError, so that it doesn't have to worry about the DBusContext.

The cleanup patches all seem fine. I'm going to merge them now.

(In reply to comment #1)
> -       -DG_LOG_DOMAIN=\"libtelepathy-logger\"          \
> +       -DG_LOG_DOMAIN=\"tp-logger\"                    \
> 
> From looking at Gabble, this should just be 'logger'.

Although logger is ambiguous, and tp-glib has tp-glib, then tp-logger sounds fine. Ignore this one.

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

> You can use features[] = { ... } here. Alternatively, since you only want the
> core feature. I think you can just pass NULL.

In the doubt I prefer to be clear in what I do, and pass it explicitly.


> +      if (!tp_account_is_enabled (acc))
> +        continue;
> +
> +      conn = tp_account_get_connection (acc);
> 
> You need to prepare the account before making these requests.

It should already be prepared, I'm calling these funcs in
tpl_observer_prepared_account_manager_cb() which is the prepare_async CB.


I've repushed (with a small rebase to squash a commit) it to:

http://git.collabora.co.uk/?p=user/kalfa/telepathy-logger.git;a=shortlog;h=refs/heads/open-channel-retrieving-rebase

(In reply to comment #4)
> > You can use features[] = { ... } here. Alternatively, since you only want the
> > core feature. I think you can just pass NULL.
> 
> In the doubt I prefer to be clear in what I do, and pass it explicitly.

It's documented. At least remove the '2', the compiler can figure this out.

> > +      if (!tp_account_is_enabled (acc))
> > +        continue;
> > +
> > +      conn = tp_account_get_connection (acc);
> > 
> > You need to prepare the account before making these requests.
> 
> It should already be prepared, I'm calling these funcs in
> tpl_observer_prepared_account_manager_cb() which is the prepare_async CB.

You need to prepare the accounts separately. Preparing the account manager is not enough.

From the manual:
"""
Returns a newly allocated GList of valid accounts in manager. The list must be freed with g_list_free() after used. ***None of the accounts in the returned list are guaranteed to be ready.***
"""

> I've repushed (with a small rebase to squash a commit) it to:
> 
> http://git.collabora.co.uk/?p=user/kalfa/telepathy-logger.git;a=shortlog;h=refs/heads/open-channel-retrieving-rebase

This branch is not based against master, it diverges from master after commit db094e346bdc4ddc2f2bb782586ae7e263ff2d74.

Now the branch is in sync with master, I've prepared and referenced each TpAccount and TpConnection.

As I understood tp_account_get_connection returns an unreferenced pointed, so I need to ref and take care of unreferencing both the connection obtained by it and each account returned by the AM, which are not referenced as well.

I could not pass the observer as weak_object this time, since the prepare methods for account and connection do not have it.
I had to roll back obtaining the obs. singleton and unreferencing it after use.

I might have passed the TpAccount as a weak_object in tp_cli_dbus_properties_call_get(), but a doubt rose:

in tpl_observer_got_channel_list_cb() I need to unref both the connection and the account on succeess AND on errors, what does it happen when the account, passed as weak_obj, is destroyed and the call cancelled?
Does tp_cli_dbus_properties_call_get() takes care of unref'ng the proxy (in this case the connection)? If yes, I'll pass it as weak_obj.

+/* Get open channels API */
+static void tpl_observer_get_open_channels (void);
+static void tpl_observer_prepared_account_manager_cb (GObject *obj,
+    GAsyncResult *result, gpointer user_data);
+static void tpl_observer_got_channel_list_cb (TpProxy *proxy,
+    const GValue *out_Value, const GError *error, gpointer user_data,
+    GObject *weak_object);
+static void tpl_observer_get_open_channels_prepare_account_cb (GObject *proxy,
+    GAsyncResult *result, gpointer user_data);
+static void tpl_observer_get_open_channels_prepared_connection (TpConnection *c
+    const GError *error, gpointer user_data);
+
+
+
+/* end of Get open channels API */

If you write these the other way up you don't need all of these prototypes :)

I'm a bit skeptical of the referencing...

+  TpAccountManager *acc_man = tp_account_manager_dup ();
+  tp_account_manager_prepare_async (acc_man, NULL,
+      tpl_observer_prepared_account_manager_cb, NULL);

You leak this ref to 'acc_man'.

+      tp_account_prepare_async (g_object_ref (acc), NULL,
+          tpl_observer_get_open_channels_prepare_account_cb, NULL);

Pretty sure you don't need to ref 'acc' here.

+  tp_connection_call_when_ready (g_object_ref (conn),
+      tpl_observer_get_open_channels_prepared_connection, account);

You don't need to ref conn here, but you probably do need to reference account.

+  tp_cli_dbus_properties_call_get (conn, -1,
+      TP_IFACE_CONNECTION_INTERFACE_REQUESTS, "Channels",
+      tpl_observer_got_channel_list_cb, account, NULL, NULL);

Use @destroy = g_object_unref to unref account here.

+  /* not use g_return_* or it will leak refs */
+  if (!TP_IS_ACCOUNT (account))
+    {
+      DEBUG ("account is not TP_ACCOUNT");
+      goto err;
+    }

Not really logical, if we fail this test what are we unreffing? Probably something we didn't want to.

I have cleaned this up and merged it for the release.