Bug 34316

So this would add an implicit requirement that DBus_Property ⇒ Has_Default?

(In reply to comment #1)
> So this would add an implicit requirement that DBus_Property ⇒ Has_Default?

I don't think that has to be true.

Perhaps it only calls Set() if Has_Default. Otherwise it adds the property to Reconnect_Required.

Already noted in the source:

        /* pessimistically assume that removing any parameter merits
         * reconnection (in a perfect implementation, if the
         * Has_Default flag was set we'd check whether the current
         * value is the default already) */

I believe this applies equally to parameters that are not a DBusProperty: if you have an account with { 'ignore-ssl-errors': False } in Parameters, the default is also False, and you unset 'ignore-ssl-errors', then we shouldn't ask for reconnnection.

Taking this to clarify the spec and fix the implementation.

Here's a (remarkably large) branch which cleans up the UpdateParameters() implementation and fixes this bug (as well as a couple of related bugs and a leak I spotted along the way).

% git diff --stat master src
 src/mcd-account-manager.c |    6 -
 src/mcd-account.c         |  358 ++++++++++++++++++++-------------------------
 src/mcd-account.h         |    7 +-
 3 files changed, 164 insertions(+), 207 deletions(-)
% git diff --stat master tests
 tests/twisted/account-manager/update-parameters.py |   97 +++++++++++++++++---
 tests/twisted/telepathy/managers/fakecm.manager    |    4 +-
 2 files changed, 87 insertions(+), 14 deletions(-)

I'll fix up the spec on Monday.

I like this. One niggle to prove I really read it:

+    if (!check_parameters (account, protocol, params, unset, dbus_properties,
+                           not_yet, &error))
+    {
+        g_hash_table_unref (dbus_properties);
+        g_ptr_array_unref (not_yet);
+        goto error;

+    g_hash_table_unref (dbus_properties);
+    g_ptr_array_unref (not_yet);
     tp_connection_manager_protocol_free (protocol);
     return;

It might be nice to have goto finally; and then error: then finally: so that all the memory is released in one location -- since my first question was whether or not the error path free'd @protocol.

(In reply to comment #6)
> I like this. One niggle to prove I really read it:
> 
> +    if (!check_parameters (account, protocol, params, unset, dbus_properties,
> +                           not_yet, &error))
> +    {
> +        g_hash_table_unref (dbus_properties);
> +        g_ptr_array_unref (not_yet);
> +        goto error;
> 
> +    g_hash_table_unref (dbus_properties);
> +    g_ptr_array_unref (not_yet);
>      tp_connection_manager_protocol_free (protocol);
>      return;
> 
> It might be nice to have goto finally; and then error: then finally: so that
> all the memory is released in one location -- since my first question was
> whether or not the error path free'd @protocol.

Quite right. Fixed slightly differently, at Nicolas’ suggestion: <http://git.collabora.co.uk/?p=telepathy-mission-control.git;a=commitdiff;h=718d297>

… and merged to master as <http://git.collabora.co.uk/?p=telepathy-mission-control.git;a=commit;h=a4ca0e6>. Sadly I introduced a regression. There's a fix at <http://git.collabora.co.uk/?p=user/wjt/telepathy-mission-control-wjt.git;a=commitdiff;h=a137d57>.

Here's a spec branch: <http://git.collabora.co.uk/?p=user/wjt/telepathy-spec-wjt.git;a=shortlog;h=refs/heads/unset-DBus_Property-parameter>

Nicolas reviewed the regression fix in MC. Just the spec clarification to go.

(In reply to comment #8)
> Here's a spec branch:
> <http://git.collabora.co.uk/?p=user/wjt/telepathy-spec-wjt.git;a=shortlog;h=refs/heads/unset-DBus_Property-parameter>

Review++ :)

Done!

The spec fix will be in 0.21.10; the MC fix will be in 5.7.4.

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