Bug 27135

+  PATH_DEBUG (self, "Disconnecting signals");
+  while (priv->signal_connections != NULL)
+    {
+      tp_proxy_signal_connection_disconnect (priv->signal_connections->data);
+      priv->signal_connections = g_list_delete_link (priv->signal_connections,
+          priv->signal_connections);
+    }
+

+  priv->signal_connections = g_list_prepend (priv->signal_connections, sig_con)
 
-  tp_cli_channel_type_text_connect_to_sent (channel,
-      on_sent_signal_cb, tpl_text, NULL, NULL, &error);
+  sig_con = tp_cli_channel_type_text_connect_to_sent (channel,
+      on_sent_signal_cb, NULL, NULL, G_OBJECT (tpl_text), &error);

etc.

Do you need to do this? Pretty sure disposing the object already disconnects the signals.

-  const gchar *channel_path = tp_proxy_get_object_path (proxy);
+  TplChannelText *chan_text = TPL_CHANNEL_TEXT (weak_object);
+  const gchar *channel_path;

This looks wrong. Why are you passing chan_text as the weak object when it is already the proxy the signal is received on?

     tp_cli_channel_type_text_call_list_pending_messages (TP_CHANNEL (chan_text)
-        -1, FALSE, got_text_pending_messages_cb, ctx, NULL, NULL);
+        -1, FALSE, got_text_pending_messages_cb, ctx, NULL,
+        G_OBJECT (chan_text));

You don't need to pass chan_text here twice.

-  TplChannelText *tpl_text = TPL_CHANNEL_TEXT (user_data);
+  TplChannelText *tpl_text = TPL_CHANNEL_TEXT (weak_object);

Again here, why not just use the channel already being passed in as @proxy. TplChannel is a subclass of TpChannel.

-  tpl_log_entry_text_set_timestamp (log, (time_t) arg_Timestamp);
+  tpl_log_entry_text_set_timestamp (log, (time_t) msg_timestamp);

The timestamp is a gint64. This mistake appears twice.

+  if (conf_singleton)

!= NULL

+static gboolean
+tpl_conf_gconf_is_account_ignored (TplConf *self,
+    const gchar *account_path,
+    GError **error)

Should be _get_account_ignored, and its counterpart should be _set_account_ignored. This should be fixed throughout the APIs actually.

+      val = TRUE; /* I don't wan't to disable logging, if something's wrong */

'want'

+static void
+conf_accounts_list_update_cb (TplConf *obj,
+    GParamSpecBoolean *result,
+    gpointer user_data)
+{
+  TplObserver *observer = TPL_OBSERVER (user_data);
+
+  /* unregister and re-register all channels, so that they'll pass through the
+   * configuration filtering in observe_channel() */
+  tpl_observer_unregister_all_channels (observer);
+  tpl_observer_get_open_channels ();
+
+}

Is it possible to lose messages when doing this? It would be much nicer if it could work out what the change is, and simply add or remove that account. May need to add a new signal: accounts-list-changed(TplConf *conf, GList *added, GList *removed).

-  key = (char *) tp_proxy_get_object_path (G_OBJECT (channel));
+  /* 'key' will be freed by the hash table on key removal/destruction */
+  key = g_strdup (tp_proxy_get_object_path (TP_PROXY (channel)));

These keys don't need to be allocated (their data is held by the TpChannel and tied to the lifetime of the channel). I changed the g_hash_table_new_full to not free them, since we can use the pre-existing allocated version.

I update the code back using the proxy without the weak_object.
I just had the wrong idea that I needed the CB chain to be stopped if the TplChannel was disposed.
The point is that at TplChannel disposal stops anyway the chain since it's the *strong* object reference, *bigger* than the other one.

I fixed the rest too, I'm working on the new signal.

Just as a note about ongoing work:

It seems that the only way with GConf to obtain the actual update (what has been changed from the last 'changed' signal) is caching the previous value in TplConfGConf, in order to obtain any changed item in the list.

I don't really like the caching solution, but it seems to be the only choice.
Any other idea is appreciated.

I'm also amending the TplConf API with

enable_accounts(GArray)
disable_accounts(GArray)

while I cannot understand why you propose get/set_account_ignored.

I mean, there is already a get_accounts_ignorelist() which, beside the name which now is get_disabled_accounts() and can be changed, is to retrieve the list of disabled accounts.

then enable/disable_accouts is to add to or remove from the list a GArray of accouts.

The last API for accounts is _is_account_disabled() which checks for a single account. There nothing to return but a gboolean, imo.

(In reply to comment #3)

> It seems that the only way with GConf to obtain the actual update (what has
> been changed from the last 'changed' signal) is caching the previous value in
> TplConfGConf, in order to obtain any changed item in the list.

That's correct unfortunately. It's better than unregistering and reregistering all channels though.

It's a shame there is no TpStringSet that we could use to store the information and generate differences.

> I'm also amending the TplConf API with
> 
> enable_accounts(GArray)
> disable_accounts(GArray)

GArray is not correct, GPtrArray is, but why not some easier to create type like a GList or a NULL-terminated list of objects? You don't need random access into the array, do you? A GList would probably be easier.

> while I cannot understand why you propose get/set_account_ignored.
> 
> The last API for accounts is _is_account_disabled() which checks for a single
> account. There nothing to return but a gboolean, imo.

What I was saying is that GLib-style APIs don't use 'is' except for types. So you have TP_IS_ACCOUNT() but tpl_conf_get_account_disabled(). And the opposite of get is set. Yeah?

This appears throughout the Tpl APIs, e.g. tpl_log_store_is_readable() should be tpl_log_store_get_readable().

The branch is ready to be reviewed, I fixed everything as suggested and renamed the TplConf API to something not 1-1 to GConf keys.

I just did not rename _is_ methods with _get_ since it appears that TP and GLib are actually using _is_ for value checking a lot. After asking Sjoerd and Simon I prefer to posticipate this renaming.

The branch is at:

http://git.collabora.co.uk/?p=user/kalfa/telepathy-logger.git;a=shortlog;h=refs/heads/tpl-conf-iface-rebase

which adds the new signal to intercept added/removed account and observe/unobserve just the ones needed.

It also cache the previous value for TplConfGConf to be able to compare the current one and signal properly clients.

One doubt here: in tpl_conf_gconf_disabled_accounts_update_cb()
there is a possible race.
One idea to avoid it was use a flag priv->updating_account_cache
which when TRUE would stop a concurrent access to the same cache.

The question is: how to signal it properly? I'd go with a GError with a special code which mean "retry later". How would TP handle this issue?



Last: I heavely rebased the branch. After the last review it changed a lot of code so today I spent some time and reorganized the patches. The old branch, is:

http://git.collabora.co.uk/?p=user/kalfa/telepathy-logger.git;a=shortlog;h=refs/heads/tpl-conf-iface-rebase-OLD

which should be the same three, with the exception of a very small amount of code I removed/added since buggy or useless.
To have a reference to the last review structure.

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

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

forgot to mention: I cherry-picked

http://git.collabora.co.uk/?p=user/danni/telepathy-logger.git;a=commit;h=36618e39fffe45c769bbfb1811164904c4102974

for tpl-marshal boilerplate.

In 88932c31afa436ae28c10327df1d229898d0687b:

You convert a GSList to a GList, which seems redundant. Do you ever go backwards through the list? Why not leave it as a GSList?

Also, iterating and using a g_list_append() makes your conversion function O(n^2). Use g_list_prepend() and reverse the list afterwards.

Why is your property a GPtrArray boxed type? Is this property ever exported over D-Bus? Otherwise perhaps just make the property G_TYPE_POINTER and pass across the GSList?

In 105cc67e5cbfa0e3eeec8fa996a391474fa905e0:

Why aren't these utilities one single utility (i.e. tpl-tool). You should use the option parsing in GLib to parse your command line options, rather than doing it yourself. Why isn't the binary in the same directory as the telepathy-logger binary? Why create a new directory?

In 0ab957b0106cc205fd11854d1e5bbb886c4b1b03:

tpl_observer_conf_globally_enabled_update_cb: could 'val' have a better name? 'enabled' perhaps.

l = g_list_append (l, g_strdup (account));

g_list_prepend is better. Why are you dupping the account path? Who frees the account path and the list, why doesn't tpl_observer_get_open_channels_filtered_by_account() take a const GSList? If you didn't return GPtrArrays, you could skip all of this code.

In 204b09848a26c17b17556e5ebdefd0b468dcc4ac:

Why not just include telepathy-glib.h ?

In 19a98b51f09d7e554e83faba6e25440d398c2bc7:

You can just put one single 'tags' in the list, which will match files called 'tags' in all subdirectories. Like we do for '.*.sw?'

(In reply to comment #9)
> In 88932c31afa436ae28c10327df1d229898d0687b:
> 
> You convert a GSList to a GList, which seems redundant. Do you ever go
> backwards through the list? Why not leave it as a GSList?
> 
> Also, iterating and using a g_list_append() makes your conversion function
> O(n^2). Use g_list_prepend() and reverse the list afterwards.

Or, use a GQueue, a much-underused data structure which I think makes GList nicer to use!

Source code not available anymore (and probably out-dated anyway).

I ported it to GSettings, which IMO removes the need for additional abstraction layers, because we're no longer linking to an extra library. If someone wants to use some weird configuration system, they should write a GSettings extension point.

So I won't fix that.