Bug 22231

<http://git.collabora.co.uk/?p=user/jonny/telepathy-mission-control.git;a=commitdiff;h=a744d9904 account_remove_cb>:
> +    if (error != NULL)
>      {
> -       if (!error)
> -           g_set_error (&error, TP_ERRORS, TP_ERROR_NOT_AVAILABLE,
> -                        "Internal error");
> -       dbus_g_method_return_error (context, error);
> -       g_error_free (error);
> -       return;
> +        error_copy = g_error_copy (error);
> +        dbus_g_method_return_error (data->context, error_copy);
> +        return;
>      }

I think this leaks the error? dbus_g_method_return_error doesn't steal it.

<http://git.collabora.co.uk/?p=user/jonny/telepathy-mission-control.git;a=commitdiff;h=436c5535c83>:
+/**
  * _mcd_account_set_parameters:

This is deliberately not gtkdoc'd - even when MC is compiled as a library, it's not part of the library.

+static GHashTable *
+hash_table_copy (GHashTable *orig)

Use tp_g_hash_table_update instead of g_hash_table_foreach, please.

+            /* we assume that the TpConnectionManager won't get
+             * freed */

Couldn't you, you know, ref it? I know the assumption is initially mine, but it becomes more questionable when you're doing things asynchronously.

(I'm not 100% sure how McdManager works...)

+static void
+get_parameter (McdAccount *account, const gchar *name,
+               McdAccountGetParameterCb callback, gpointer user_data,

I don't like the way this has a "goto error" passing a GError that can be NULL to the callback.

Review still in progress...

(In reply to comment #1)
> I think this leaks the error? dbus_g_method_return_error doesn't steal it.

Right. I'm not quite sure I didn't see the "const" on the error parameter.

> This is deliberately not gtkdoc'd - even when MC is compiled as a library, it's
> not part of the library.

Okay, removed extra asterisk.

> Use tp_g_hash_table_update instead of g_hash_table_foreach, please.

Done.

> Couldn't you, you know, ref it? I know the assumption is initially mine, but it
> becomes more questionable when you're doing things asynchronously.

Okay, I just duped the strings. This was actually already fixed in 2/3 of the places where the comment was.

> I don't like the way this has a "goto error" passing a GError that can be NULL
> to the callback.

Already fixed.

> Review still in progress...

I'm hot on your heels!

I'm just cleaning up some commit messages as you requested.

+static void
+_mcd_account_delete (McdAccount *account,
+                     McdAccountDeleteCb callback,
+                     gpointer user_data)
 {
     McdAccountPrivate *priv = account->priv;
     gchar *data_dir_str;
     GDir *data_dir;
     GError *kf_error = NULL;
+    AccountDeleteData *delete_data;
 
     if (!g_key_file_remove_group (priv->keyfile, priv->unique_name,
                                   &kf_error))
@@ -614,8 +637,8 @@ _mcd_account_delete (McdAccount *account, GError **error)
         else
         {
             g_warning ("Could not remove group (%s)", kf_error->message);
-            g_propagate_error (error, kf_error);
-            return FALSE;
+            callback (account, kf_error, user_data);
+            return;

This leaks kf_error, I think?

+complete_account_creation_set_cb (McdAccount *account, GPtrArray *not_yet,
...
+    if (cad->ok)
+    {
+        add_account (account_manager, account);
+        mcd_account_check_validity (account, complete_account_creation_finish, cad);
+    }
+    else
+    {
+        complete_account_creation_finish (account, TRUE, cad);

Is this TRUE meant to be FALSE?

get_parameter() doesn't always set an error, which is apparently fixed in a later patch. I should check that based on what's at HEAD.

"mcd-account: copy strings in not_yet callback argument" should be squashed into the patch(es) that it fixes; so should "mcd-account: on error never give not_yet as it's not filled".

How's the test coverage?

Please reinstate patch keyword when this is ready for review again.

(In reply to comment #3)
> This leaks kf_error, I think?

Fixed.

> Is this TRUE meant to be FALSE?

No. This would report to complete_account_creation_finish that the CM parameters supplied are invalid, which isn't actually the problem. I had, however, neglected to report an error back. This is now fixed.

> get_parameter() doesn't always set an error, which is apparently fixed in a
> later patch. I should check that based on what's at HEAD.

Yeah, it's fixed in HEAD.

> "mcd-account: copy strings in not_yet callback argument" should be squashed
> into the patch(es) that it fixes; so should "mcd-account: on error never give
> not_yet as it's not filled".

These have been squashed into "mcd-account: refactored set_parameters to be async and iteratively set params".

> How's the test coverage?

I haven't added any tests because in my opinion, I haven't added any new features that aren't already being tested -- the tests you already wrote appear pretty good. I guess the tests take different code paths depending on --enable-gnome-keyring.

It should be noted that all the tests still pass. :-)

Any progress on this? I'd really like to have this feature for the GNOME 2.28 release.

Jonny: in http://git.collabora.co.uk/?p=user/jonny/telepathy-mission-control.git;a=commitdiff;h=918b38554c you're patching the wrong main() :-) The same change should be applied to server/mc-server.c (in current master I've deleted mcd-main.c to avoid confusion).

Other than that I think this branch is probably OK, but I'd like to test it more before saying "yes". We can discuss it on Monday.

Fixed in git for 5.3.0

5.3.0