[Bug 39381] MC unconditionally pushes its accounts' nicknames to the CM as aliases

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=39381

Will Thompson <will.thompson at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|                            |review-

--- Comment #1 from Will Thompson <will.thompson at collabora.co.uk> 2011-10-04 09:38:30 PDT ---
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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.