Bug 39381

I think the behaviour this branch implements is definitely more correct.

First up: the problem with adding/changing the tests before the code changes to make them pass is that it breaks rebase.

+++ b/tests/twisted/account-manager/nickname-empty-cm-alias.py
@@ -0,0 +1,74 @@
+# Copyright (C) 2009 Nokia Corporation
+# Copyright (C) 2009 Collabora Ltd.

Wrong year.

+import dbus
+import dbus
+import dbus.service

I bet only the first of those three are needed.

+from servicetest import EventPattern, tp_name_prefix, tp_path_prefix, \

The tp_*_prefix imports there are not used.


+def test(q, bus, mc):
+    # second round: we get a canonical name from the server and our
+    # local name is _not_ the canonical name, so we should propagate that:

+def test(q, bus, mc):
+    # second round: we get a canonical name from the server and our
+    # local name is _not_ the canonical name, so we should propagate that:

“second round” of what? What does “canonical name” mean? Given that MC has a specific name for the user's own identifier—namely “normalized name”, as seen as a property on Account—I think consistently referring to it as that would be better.

+    account_iface = dbus.Interface(account, cs.ACCOUNT)
+    account_props = dbus.Interface(account, cs.PROPERTIES_IFACE)

You should be able to just use 'account' and 'account.Properties' in place of these two new variables.

+    assert account_props.Get(cs.ACCOUNT, 'Nickname') == 'whatever'

Please use assertEquals() and friends; they give more useful messages when they fail.

Is tests/twisted/account-manager/nickname-empty-cm-alias.py essentially a verbatim copy of the modified version of tests/twisted/account-manager/nickname.py? Could the latter's test() be generalized to avoid the copy-pasta?

+    q.dbus_return(get_aliases.message, { conn.self_handle: 'myself' },

Use conn.self_ident rather than hard-coding 'myself'.

+static void _take_alias (McdConnectionPrivate *priv, gchar *alias)

So this … sometimes takes ownership of 'alias', and sometimes does not and hence 'alias' leaks? And the coding style of the quoted line is wrong: there should be a newline after 'void', at least.

     if (error != NULL)
     {
         DEBUG ("GetAliases([SelfHandle]) failed: %s", error->message);
-        return;
+        goto set_nickname;
     }
 
     self_handle = tp_connection_get_self_handle (proxy);
 
-    alias = g_hash_table_lookup (aliases,GUINT_TO_POINTER (self_handle));
+    server_alias = g_hash_table_lookup (aliases,GUINT_TO_POINTER (self_handle));
+    DEBUG ("got alias %s from cache", server_alias);
 
-    if (alias != NULL &&
-        (priv->alias == NULL || tp_strdiff (priv->alias, alias)))
+set_nickname:

Blech! Why use 'goto' when you could put the three skipped lines of code into an else {} block?

+    DEBUG ("got alias %s from cache", server_alias);

What cache? It's not been fetched from a cache, it's been fetched from the CM.

+    /* We didn't get an alias from the CM, use the account's value */
+    if (tp_str_empty (server_alias))

+    /* we did get a value, but it's the canonicalised value (eg XMPP JID)
+       and the account value is not the same (ie definitely from the user) */
+    else if (!tp_strdiff (server_alias, canon_alias) &&
+             tp_strdiff (server_alias, local_alias))

As far as I can tell, the bodies of these two conditions are identical. They should not be duplicated.

+        DEBUG ("ALIASING INTERFACE SUPPORT");

Ahem.

-    if (priv->has_alias_if)
 	_mcd_connection_setup_alias (connection);

The indentation should be corrected.

+    else if (names != NULL && names[0] != NULL)
+    {
+        DEBUG ("names[]: %p [%s ...]", names, names ? names[0] : NULL);

How is the pointer address of the array relevant? I don't think this debug message is really necessary, given that _mcd_account_set_normalized_name already has one.

(In reply to comment #1)
> First up: the problem with adding/changing the tests before the code changes to
> make them pass is that it breaks rebase.

By which I mean "bisect", not "rebase".

After some refactoring on Bug #55668, this became almost a one-line fix (although I took a few more lines to add some debug messages), so I fixed it there.

(In reply to comment #1)
> Is tests/twisted/account-manager/nickname-empty-cm-alias.py essentially a
> verbatim copy of the modified version of
> tests/twisted/account-manager/nickname.py? Could the latter's test() be
> generalized to avoid the copy-pasta?

I did this on Bug #55668.

> Use conn.self_ident rather than hard-coding 'myself'.

... and this.

Fixed in git for 5.15.0, as part of Bug #55668.