Merge branch 'aliases' into telepathy-gabble-0.12

Reviewed-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

6 files changed, 165 insertions, 78 deletions

diff --git a/src/conn-aliasing.c b/src/conn-aliasing.c
index eeaa66516..782fb82cf 100644
--- a/src/conn-aliasing.c
+++ b/src/conn-aliasing.c
@@ -880,14 +880,35 @@ _gabble_connection_get_cached_alias (GabbleConnection *conn,
   GabblePresence *pres;
   const gchar *tmp, *jid;
   gchar *resource = NULL;
+  gboolean roster_alias_was_jid = FALSE;
 
   g_return_val_if_fail (NULL != conn, GABBLE_CONNECTION_ALIAS_NONE);
   g_return_val_if_fail (GABBLE_IS_CONNECTION (conn), GABBLE_CONNECTION_ALIAS_NONE);
   g_return_val_if_fail (tp_handle_is_valid (contact_handles, handle, NULL),
       GABBLE_CONNECTION_ALIAS_NONE);
 
+  jid = tp_handle_inspect (contact_handles, handle);
+  g_assert (NULL != jid);
+
   tmp = gabble_roster_handle_get_name (conn->roster, handle);
-  if (NULL != tmp)
+  if (!tp_strdiff (tmp, jid))
+    {
+      /* Normally, we prefer whatever we've cached on the roster, to avoid
+       * wasting bandwidth checking for aliases by repeatedly fetching the
+       * vCard, and (more importantly) to prefer anything the local user set
+       * over what the contact says their name is.
+       *
+       * However, if the alias stored on the roster is just the contact's JID,
+       * we check for better aliases that we happen to have received from other
+       * sources (maybe a PEP nick update, or a vCard we've fetched for the
+       * avatar, or whatever). If we can't find anything better, we'll use the
+       * JID, and still say that it came from the roster: this means we don't
+       * defeat negative caching for contacts who genuinely don't have an
+       * alias.
+       */
+      roster_alias_was_jid = TRUE;
+    }
+  else if (!tp_str_empty (tmp))
     {
       maybe_set (alias, tmp);
       return GABBLE_CONNECTION_ALIAS_FROM_ROSTER;
@@ -925,10 +946,6 @@ _gabble_connection_get_cached_alias (GabbleConnection *conn,
         }
     }
 
-  jid = tp_handle_inspect (contact_handles, handle);
-  g_assert (NULL != jid);
-
-
   /* MUC handles have the nickname in the resource */
   if (gabble_decode_jid (jid, NULL, NULL, &resource) &&
       NULL != resource)
@@ -949,9 +966,11 @@ _gabble_connection_get_cached_alias (GabbleConnection *conn,
         }
     }
 
-  /* otherwise just take their jid */
+  /* otherwise just take their jid, which may have been specified on the roster
+   * as the contact's alias. */
   maybe_set (alias, jid);
-  return GABBLE_CONNECTION_ALIAS_FROM_JID;
+  return roster_alias_was_jid ? GABBLE_CONNECTION_ALIAS_FROM_ROSTER
+      : GABBLE_CONNECTION_ALIAS_FROM_JID;
 }
 
 static void
diff --git a/tests/twisted/gabbletest.py b/tests/twisted/gabbletest.py
index e14c82e32..f72389e6a 100644
--- a/tests/twisted/gabbletest.py
+++ b/tests/twisted/gabbletest.py
@@ -248,45 +248,44 @@ class XmppAuthenticator(GabbleAuthenticator):
     def sessionIq(self, iq):
         self.xmlstream.send(make_result_iq(self.xmlstream, iq))
 
-def make_stream_event(type, stanza, stream):
-    event = servicetest.Event(type, stanza=stanza)
-    event.stream = stream
-    event.to = stanza.getAttribute("to")
-    return event
-
-def make_iq_event(stream, iq):
-    event = make_stream_event('stream-iq', iq, stream)
-    event.iq_type = iq.getAttribute("type")
-    event.iq_id = iq.getAttribute("id")
-    query = iq.firstChildElement()
-
-    if query:
-        event.query = query
-        event.query_ns = query.uri
-        event.query_name = query.name
-
-        if query.getAttribute("node"):
-            event.query_node = query.getAttribute("node")
-    else:
-        event.query = None
-
-    return event
-
-def make_presence_event(stream, stanza):
-    event = make_stream_event('stream-presence', stanza, stream)
-    event.presence_type = stanza.getAttribute('type')
+class StreamEvent(servicetest.Event):
+    def __init__(self, type_, stanza, stream):
+        servicetest.Event.__init__(self, type_, stanza=stanza)
+        self.stream = stream
+        self.to = stanza.getAttribute("to")
+
+class IQEvent(StreamEvent):
+    def __init__(self, stream, iq):
+        StreamEvent.__init__(self, 'stream-iq', iq, stream)
+        self.iq_type = iq.getAttribute("type")
+        self.iq_id = iq.getAttribute("id")
+
+        query = iq.firstChildElement()
+
+        if query:
+            self.query = query
+            self.query_ns = query.uri
+            self.query_name = query.name
+
+            if query.getAttribute("node"):
+                self.query_node = query.getAttribute("node")
+        else:
+            self.query = None
 
-    statuses = xpath.queryForNodes('/presence/status', stanza)
+class PresenceEvent(StreamEvent):
+    def __init__(self, stream, stanza):
+        StreamEvent.__init__(self, 'stream-presence', stanza, stream)
+        self.presence_type = stanza.getAttribute('type')
 
-    if statuses:
-        event.presence_status = str(statuses[0])
+        statuses = xpath.queryForNodes('/presence/status', stanza)
 
-    return event
+        if statuses:
+            self.presence_status = str(statuses[0])
 
-def make_message_event(stream, stanza):
-    event = make_stream_event('stream-message', stanza, stream)
-    event.message_type = stanza.getAttribute('type')
-    return event
+class MessageEvent(StreamEvent):
+    def __init__(self, stream, stanza):
+        StreamEvent.__init__(self, 'stream-message', stanza, stream)
+        self.message_type = stanza.getAttribute('type')
 
 class StreamFactory(twisted.internet.protocol.Factory):
     def __init__(self, streams, jids):
@@ -381,11 +380,11 @@ class BaseXmlStream(xmlstream.XmlStream):
         xmlstream.XmlStream.__init__(self, authenticator)
         self.event_func = event_func
         self.addObserver('//iq', lambda x: event_func(
-            make_iq_event(self, x)))
+            IQEvent(self, x)))
         self.addObserver('//message', lambda x: event_func(
-            make_message_event(self, x)))
+            MessageEvent(self, x)))
         self.addObserver('//presence', lambda x: event_func(
-            make_presence_event(self, x)))
+            PresenceEvent(self, x)))
         self.addObserver('//event/stream/authd', self._cb_authd)
         if self.handle_privacy_lists:
             self.addObserver("/iq/query[@xmlns='%s']" % ns.PRIVACY,
diff --git a/tests/twisted/pubsub.py b/tests/twisted/pubsub.py
index 5ee6546ec..298106d95 100644
--- a/tests/twisted/pubsub.py
+++ b/tests/twisted/pubsub.py
@@ -4,6 +4,17 @@ from gabbletest import exec_test, elem, sync_stream
 import constants as cs
 import ns
 
+def make_pubsub_event(from_, node, *contents):
+    return elem('message', from_=from_)(
+        elem((ns.PUBSUB_EVENT), 'event')(
+            elem('items', node=node)(
+                elem('item')(
+                    *contents
+                )
+            )
+        )
+    )
+
 def test(q, bus, conn, stream):
     # event node without NS
     message = elem('message', from_='bob@foo.com')(
diff --git a/tests/twisted/roster/test-save-alias-to-roster.py b/tests/twisted/roster/test-save-alias-to-roster.py
index 7772b5d94..6012df0c9 100644
--- a/tests/twisted/roster/test-save-alias-to-roster.py
+++ b/tests/twisted/roster/test-save-alias-to-roster.py
@@ -5,10 +5,28 @@ Test that updating an alias saves it to the roster.
 
 import dbus
 
-from servicetest import EventPattern, call_async
-from gabbletest import acknowledge_iq, exec_test, make_result_iq
+from servicetest import EventPattern, call_async, assertEquals
+from gabbletest import (
+    acknowledge_iq, exec_test, make_result_iq, sync_stream, elem
+    )
 import constants as cs
 import ns
+from rostertest import expect_contact_list_signals, send_roster_push
+from pubsub import make_pubsub_event
+
+def send_pep_nick_reply(stream, stanza, nickname):
+    result = make_result_iq(stream, stanza)
+    pubsub = result.firstChildElement()
+    items = pubsub.addElement('items')
+    items['node'] = ns.NICK
+    item = items.addElement('item')
+    item.addElement('nick', ns.NICK, content=nickname)
+    stream.send(result)
+
+def check_roster_write(event, jid, name):
+    item = event.query.firstChildElement()
+    assertEquals(jid, item['jid'])
+    assertEquals(name, item['name'])
 
 def test(q, bus, conn, stream):
     event, event2 = q.expect_many(
@@ -19,17 +37,9 @@ def test(q, bus, conn, stream):
     acknowledge_iq(stream, event.stanza)
     acknowledge_iq(stream, event2.stanza)
 
-    while True:
-        event = q.expect('dbus-signal', signal='NewChannel')
-        path, type, handle_type, handle, suppress_handler = event.args
-
-        if type != cs.CHANNEL_TYPE_CONTACT_LIST:
-            continue
-
-        chan_name = conn.InspectHandles(handle_type, [handle])[0]
-
-        if chan_name == 'subscribe':
-            break
+    signals = expect_contact_list_signals(q, bus, conn, lists=['subscribe'])
+    old_signal, new_signal = signals[0]
+    path = old_signal.args[0]
 
     # request subscription
     chan = bus.get_object(conn.bus_name, path)
@@ -42,32 +52,74 @@ def test(q, bus, conn, stream):
     item = event.query.firstChildElement()
 
     acknowledge_iq(stream, event.stanza)
-    # FIXME: when we depend on a new enough tp-glib, expect AddMembers
-    # to return here
+    q.expect('dbus-return', method='AddMembers')
 
     call_async(q, conn.Aliasing, 'RequestAliases', [handle])
 
     event = q.expect('stream-iq', iq_type='get',
         query_ns='http://jabber.org/protocol/pubsub',
         to='bob@foo.com')
-
-    result = make_result_iq(stream, event.stanza)
-    pubsub = result.firstChildElement()
-    items = pubsub.addElement('items')
-    items['node'] = 'http://jabber.org/protocol/nick'
-    item = items.addElement('item')
-    item.addElement('nick', 'http://jabber.org/protocol/nick',
-        content='Bobby')
-    stream.send(result)
+    send_pep_nick_reply(stream, event.stanza, 'Bobby')
 
     event, _ = q.expect_many(
         EventPattern('stream-iq', iq_type='set', query_ns=ns.ROSTER),
         EventPattern('dbus-return', method='RequestAliases',
         value=(['Bobby'],)))
-
-    item = event.query.firstChildElement()
-    assert item['jid'] == 'bob@foo.com'
-    assert item['name'] == 'Bobby'
+    check_roster_write(event, 'bob@foo.com', 'Bobby')
+
+    # We get a roster push for a contact who for some reason has their alias
+    # set on our roster to the empty string (maybe a buggy client?). It's never
+    # useful for Gabble to say that someone's alias is the empty string (given
+    # the current semantics where the alias is always meant to be something you
+    # could show, even if it's just their JID), so let's forbid that.
+    jid = 'parts@labor.lit'
+    handle = conn.RequestHandles(cs.HT_CONTACT, [jid])[0]
+    q.forbid_events([EventPattern('dbus-signal', signal='AliasesChanged',
+        args=[[(handle, '')]])])
+
+    send_roster_push(stream, jid, 'both', name='')
+    # I don't really have very strong opinions on whether Gabble should be
+    # signalling that this contact's alias has *changed* per se, so am not
+    # explicitly expecting that.
+    q.expect('dbus-signal', signal='MembersChanged')
+
+    # But if we ask for it, Gabble should probably send a PEP query.
+    assertEquals(jid, conn.Aliasing.GetAliases([handle])[handle])
+    event = q.expect('stream-iq', iq_type='get', query_ns=ns.PUBSUB, to=jid)
+    nick = 'Constant Future'
+
+    send_pep_nick_reply(stream, event.stanza, nick)
+    _, roster_write = q.expect_many(
+        EventPattern('dbus-signal', signal='AliasesChanged',
+            args=[[(handle, nick)]]),
+        EventPattern('stream-iq', iq_type='set', query_ns=ns.ROSTER),
+        )
+    check_roster_write(roster_write, jid, nick)
+
+    # Here's another contact, whose alias is set on our roster to their JID:
+    # because we've cached that they have no alias. Gabble shouldn't make
+    # unsolicited PEP or vCard queries to them.
+    jid = 'friendly@faith.plate'
+    handle = conn.RequestHandles(cs.HT_CONTACT, [jid])[0]
+
+    q.forbid_events([
+        EventPattern('stream-iq', query_ns=ns.PUBSUB, to=jid),
+        EventPattern('stream-iq', query_ns=ns.VCARD_TEMP, to=jid),
+    ])
+    send_roster_push(stream, jid, 'both', name=jid)
+    q.expect('dbus-signal', signal='AliasesChanged', args=[[(handle, jid)]])
+    sync_stream(q, stream)
+
+    # But if we get a PEP nickname update for this contact, Gabble should use
+    # the new nickname, and write it back to the roster.
+    nick = u'The Friendly Faith Plate'
+    stream.send(make_pubsub_event(jid, ns.NICK, elem(ns.NICK, 'nick')(nick)))
+    _, roster_write = q.expect_many(
+        EventPattern('dbus-signal', signal='AliasesChanged',
+            args=[[(handle, nick)]]),
+        EventPattern('stream-iq', iq_type='set', query_ns=ns.ROSTER),
+        )
+    check_roster_write(roster_write, jid, nick)
 
 if __name__ == '__main__':
     exec_test(test)
diff --git a/tests/twisted/rostertest.py b/tests/twisted/rostertest.py
index 335da18d2..3d31ad454 100644
--- a/tests/twisted/rostertest.py
+++ b/tests/twisted/rostertest.py
@@ -7,7 +7,7 @@ from servicetest import (assertEquals, assertLength, EventPattern,
 import constants as cs
 import ns
 
-def make_roster_push(stream, jid, subscription, ask_subscribe=False):
+def make_roster_push(stream, jid, subscription, ask_subscribe=False, name=None):
     iq = IQ(stream, "set")
     iq['id'] = 'push'
     query = iq.addElement('query')
@@ -16,14 +16,17 @@ def make_roster_push(stream, jid, subscription, ask_subscribe=False):
     item['jid'] = jid
     item['subscription'] = subscription
 
+    if name is not None:
+        item['name'] = name
+
     if ask_subscribe:
         item['ask'] = 'subscribe'
 
     return iq
 
-def send_roster_push(stream, jid, subscription, ask_subscribe=False):
+def send_roster_push(stream, jid, subscription, ask_subscribe=False, name=None):
     iq = make_roster_push(stream, jid, subscription,
-        ask_subscribe=ask_subscribe)
+        ask_subscribe=ask_subscribe, name=name)
     stream.send(iq)
 
 def get_contact_list_event_patterns(q, bus, conn, expected_handle_type, name):
diff --git a/tests/twisted/servicetest.py b/tests/twisted/servicetest.py
index 976622c8d..b2ac9c64d 100644
--- a/tests/twisted/servicetest.py
+++ b/tests/twisted/servicetest.py
@@ -40,16 +40,19 @@ class DictionarySupersetOf (object):
         except TypeError: # other is not iterable
             return False
 
-class Event:
+class Event(object):
     def __init__(self, type, **kw):
         self.__dict__.update(kw)
         self.type = type
         (self.subqueue, self.subtype) = type.split ("-", 1)
 
+    def __str__(self):
+        return '\n'.join([ str(type(self)) ] + format_event(self))
+
 def format_event(event):
     ret = ['- type %s' % event.type]
 
-    for key in dir(event):
+    for key in sorted(dir(event)):
         if key != 'type' and not key.startswith('_'):
             ret.append('- %s: %s' % (
                 key, pprint.pformat(getattr(event, key))))