Bug 41743

Created attachment 52288 [details] [review]
I don't know how best to describe this patch.

Fixes: <https://bugs.freedesktop.org/show_bug.cgi?id=41743>

Well here is a patch which fixes this bug but I don't like it.

Really, it's pretty crazy that the presence cache is responsible for remembering nicknames which we got from a message. Given that we already have like 75 different alias sources, I think a better approach would probably be to make GabbleImFactory stash them, rather than having these weird interactions with presence.

Created attachment 52341 [details] [review]
configure: depend on tp-glib with Subject.

Actually, a slightly earlier version of telepathy-glib, on the unstable
0.15.x branch, includes Subject, but we may as well depend on the stable
branch: it includes fixes for Empathy bugs.

Created attachment 52342 [details] [review]
adium themes: crash less hard when Template.html is missing

If Template.html is missing, then you haven't installed Empathy properly
(or set EMPATHY_SRCDIR or whatever in my case). But previously we would
just crash later down this function when passing template_html to
string_with_format() without checking if it's NULL. This patch makes
it fail a little less inscrutably.

Comment on attachment 52341 [details] [review]
configure: depend on tp-glib with Subject.

wrong bug.

Comment on attachment 52342 [details] [review]
adium themes: crash less hard when Template.html is missing

wrong bug.

Comment on attachment 52288 [details] [review]
I don't know how best to describe this patch.

>From 4a7bf6ca56656a684f2138002a79cc4c7f867cd9 Mon Sep 17 00:00:00 2001
>From: Will Thompson <will.thompson@collabora.co.uk>
>Date: Thu, 13 Oct 2011 11:33:08 +0100
>Subject: [PATCH] I don't know how best to describe this patch.
>
>Fixes: <https://bugs.freedesktop.org/show_bug.cgi?id=41743>
>---
> src/roster.c                                       |   24 +++++++++++++++++--
> tests/twisted/presence/initial-contact-presence.py |   12 +++++++++-
> 2 files changed, 32 insertions(+), 4 deletions(-)
>
>diff --git a/src/roster.c b/src/roster.c
>index 8d8c3b7..d7c0527 100644
>--- a/src/roster.c
>+++ b/src/roster.c
>@@ -1398,11 +1398,29 @@ got_roster_iq (GabbleRoster *roster,
>         {
>           GabbleRosterItem *item = v;
>           TpHandle contact = GPOINTER_TO_UINT (k);
>+          GabblePresence *presence = gabble_presence_cache_get (
>+              priv->conn->presence_cache, contact);
> 
>           if (item->subscribe == TP_SUBSCRIPTION_STATE_YES &&
>-              gabble_presence_cache_get (roster->priv->conn->presence_cache,
>-                  contact) == NULL)
>-            g_array_append_val (members, contact);
>+              (presence == NULL || presence->status == GABBLE_PRESENCE_UNKNOWN))
>+            {
>+              /* The contact might be in the presence cache with UNKNOWN
>+               * presence if we've received a message from them before the
>+               * roster arrived: an item is forcibly added to stash the
>+               * nickname which might have been included in the <message/> in
>+               * the presence cache. (This seems like a rather illogical place
>+               * to stash such nicknamesâif anything, they should live in
>+               * GabbleImFactoryâbut there we go.)
>+               *
>+               * So if this is the case, we flip their status to OFFLINE. We
>+               * don't use gabble_presence_update() because we want to signal
>+               * all the unknownâoffline transitions together.
>+               */
>+              if (presence != NULL)
>+                presence->status = GABBLE_PRESENCE_OFFLINE;
>+
>+              g_array_append_val (members, contact);
>+            }
> 
>           if (item->unsent_edits != NULL)
>             edited_items = g_slist_prepend (edited_items, item);
>diff --git a/tests/twisted/presence/initial-contact-presence.py b/tests/twisted/presence/initial-contact-presence.py
>index ab52881..881316c 100644
>--- a/tests/twisted/presence/initial-contact-presence.py
>+++ b/tests/twisted/presence/initial-contact-presence.py
>@@ -8,7 +8,7 @@ This serves as a regression test for
> <https://bugs.freedesktop.org/show_bug.cgi?id=38603>, among other bugs.
> """
> 
>-from gabbletest import exec_test, make_presence, sync_stream
>+from gabbletest import exec_test, make_presence, sync_stream, elem
> from servicetest import assertEquals, EventPattern, sync_dbus
> 
> import constants as cs
>@@ -58,6 +58,16 @@ def test(q, bus, conn, stream):
>     stream.send(make_presence('eve@foo.com'))
>     q.expect('dbus-signal', signal='PresencesChanged', args=[{eve: AVAILABLE}])
> 
>+    # We also get a message from a contact before we get the roster (presumably
>+    # they sent this while we were offline?). This shouldn't affect the contact
>+    # being reported as offline when we finally do get the roster, but it used
>+    # to: <https://bugs.freedesktop.org/show_bug.cgi?id=41743>.
>+    stream.send(
>+        elem('message', from_='amy@foo.com', type='chat')(
>+          elem('body')(u'why are you never online?')
>+        ))
>+    q.expect('dbus-signal', signal='MessageReceived')
>+
>     event.stanza['type'] = 'result'
>     event.query.addChild(make_roster_item('amy@foo.com', 'both'))
>     event.query.addChild(make_roster_item('bob@foo.com', 'from'))
>-- 
>1.7.7

Created attachment 52535 [details] [review]
[PATCH 1/2] Test grabbing <nick/> from <message/>

If a contact is not on your roster, you typically have no idea what
their nickname is: no roster, no PEP, no vCard (assuming the server
doesn't let random people fetch your vCard). In this situation, contacts
who message you out of the blue can include <nick/> in the message
itself.

This is implemented in a kind of dodgy way in Gabble at the moment: the
IM channel forcibly retains an entry for the contact in the presence
cache, and then the presence cache stashes the nickname as if it came
from presence… It was also previously untested, so I thought it worth
adding a test before I even thought about fixing how it's implemented.

Created attachment 52537 [details] [review]
[PATCH 2/2] Fix offline contacts showing up as unknown, not offline

Due to a weird interaction between the presence cache, IM channels, and
scraping nicknames out of <message/>s, receiving a message from an
offline contact before the roster is received would cause their status
to erroneously show up as unknown, not offline.

This fix is a bit of a hack, but it is much smaller than refactoring to
make the IM channel store the alias (which would allow us to expunge
keep_unavailable).

This regressed as a side-effect of <http://cgit.freedesktop.org/telepathy/telepathy-gabble/commit/?id=e0cda61>.

YES GOGOGOGOGOGOGOGOGOGOGO

Merged to master, will be in 0.13.8 (which may be called 0.14.0).