Bug 39377

Summary: TP_CONTACT_FEATURE_LOCATION repeatedly prepared for contacts who have no location
Product: Telepathy Reporter: Will Thompson <will>
Component: tp-glibAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium Keywords: patch
Version: git master   
Hardware: Other   
OS: All   
URL: http://cgit.collabora.com/git/user/wjt/telepathy-glib.git/log/?h=fd.o-39377-repeated-location-requests
Whiteboard: review+
i915 platform: i915 features:

Description Will Thompson 2011-07-19 08:24:28 UTC
I noticed that whenever a chat state changes in an Empathy conversation window, Empathy (well, tp-glib on behalf of Empathy) would re-request Location attributes. For instance, here's me typing a single character into a conversation window:

> method call sender=:1.657 -> dest=:1.452 serial=427
>             path=/org/freedesktop/Telepathy/Connection/gabble/jabber/will_40example_2fb756e715/ImChannel11;
>             interface=org.freedesktop.Telepathy.Channel.Interface.ChatState;
>             member=SetChatState
>    uint32 3
> signal sender=:1.452 -> dest=(null destination) serial=10051
>        path=/org/freedesktop/Telepathy/Connection/gabble/jabber/will_40example_2fb756e715/ImChannel11;
>        interface=org.freedesktop.Telepathy.Channel.Interface.ChatState;
>        member=ChatStateChanged
>    uint32 1
>    uint32 3
> method return sender=:1.452 -> dest=:1.657 reply_serial=427
> method call sender=:1.657 -> dest=:1.452 serial=428
>             path=/org/freedesktop/Telepathy/Connection/gabble/jabber/will_40example_2fb756e715;
>             interface=org.freedesktop.Telepathy.Connection.Interface.Contacts;
>             member=GetContactAttributes
>    array [
>       uint32 1
>    ]
>    array [
>       string "org.freedesktop.Telepathy.Connection.Interface.Location"
>    ]
>    boolean true
> method return sender=:1.452 -> dest=:1.657 reply_serial=428
>    array [
>       dict entry(
>          uint32 1
>          array [
>             dict entry(
>                string "org.freedesktop.Telepathy.Connection/contact-id"
>                variant                   string "will@example"
>             )
>          ]
>       )
>    ]

This happens because the internal CONTACT_FEATURE_FLAG_LOCATION flag is only set on a TpContact when a location is discovered for them (specifically, by contact_maybe_set_location(), which is called from the LocationUpdated signal and from the GetContactAttributes return callback. (And, indirectly, from the inexplicable GetLocations() fallback path, but we'll ignore that.)

contact_maybe_set_location() looks like this:

  static void
  contact_maybe_set_location (TpContact *self,
      GHashTable *location)
  {
    if (self == NULL || location == NULL)
      return;

    if (self->priv->location != NULL)
      g_hash_table_unref (self->priv->location);

    self->priv->has_features |= CONTACT_FEATURE_FLAG_LOCATION;
    self->priv->location = g_hash_table_ref (location);
    g_object_notify ((GObject *) self, "location");
  }

But the ".../location" attribute is defined to be omitted entirely if the contact has no location. <http://telepathy.freedesktop.org/spec/Connection_Interface_Location.html#Contact-Attribute:location>

TpContact should be able to tell that it requested location information, and hence that the absence of the attribute indicates that the contact has no published location information.

(Relatedly, I noticed that Empathy will crash if ::notify(location) is emitted by TpContact and tp_contact_get_location() subsequently returns NULL. So I guess we have to represent “I know this contact has no published location information” by an empty hash table. Joy!)

Some other features may have similar issues. I haven't really checked. This is the one that bothers me whenever I look at dbus-monitor. :)
Comment 1 Will Thompson 2011-07-19 09:53:37 UTC
I tweaked test_by_handle_again() to see which other features have this issue.

-  TpContactFeature feature = TP_CONTACT_FEATURE_ALIAS;
+  TpContactFeature features[] = {
+      TP_CONTACT_FEATURE_ALIAS,
+      // TP_CONTACT_FEATURE_AVATAR_TOKEN,
+      TP_CONTACT_FEATURE_PRESENCE,
+      // TP_CONTACT_FEATURE_LOCATION,
+      // TP_CONTACT_FEATURE_CAPABILITIES,
+      // TP_CONTACT_FEATURE_AVATAR_DATA,
+      // TP_CONTACT_FEATURE_CONTACT_INFO,
+      TP_CONTACT_FEATURE_CLIENT_TYPES,
+      TP_CONTACT_FEATURE_SUBSCRIPTION_STATES,
+      TP_CONTACT_FEATURE_CONTACT_GROUPS
+  };

Uncommenting any one of the commented features above makes the test fail. In some cases this could be due to misimplementation of the test CM—I haven't checked.
Comment 2 Will Thompson 2011-07-19 11:07:20 UTC
Here's a branch which fixes this for Location on the fast path, and removes the slow path for Location.

I haven't checked out the other ones yet.

It also includes a hack to disable the test timeouts under gdb, and some crucial localization changes.
Comment 3 Jonny Lamb 2011-07-21 05:45:59 UTC
Branch looks good. Epic hats off to you for writing a nice tp-glib test; they're such a pain in the ass. Just one point:

+ g_warning ("%s supports Location but not Contacts! Where did you find "

Use the WARNING macro?

but ++++++++++++++++++++++
Comment 4 Will Thompson 2011-07-21 11:17:48 UTC
(In reply to comment #3)
> Branch looks good. Epic hats off to you for writing a nice tp-glib test;
> they're such a pain in the ass.

Oh there is *way* more where this came from. Fancy reviewing http://cgit.collabora.com/git/user/wjt/telepathy-glib.git/log/?h=fd.o-39377-repeated-location-requests-moar ?

> Just one point:
> 
> + g_warning ("%s supports Location but not Contacts! Where did you find "
> 
> Use the WARNING macro?

Good point. Haven't fixed this yet.
Comment 5 Xavier Claessens 2011-07-27 02:54:59 UTC
#define TRACER_T "\nTracerPid:\t" // http://www.youtube.com/watch?v=SXmv8quf_xM

I would change that to:

/* http://www.youtube.com/watch?v=SXmv8quf_xM */
#define TRACER_T "\nTracerPid:\t"


Otherwise branch looks good.
Comment 6 Will Thompson 2011-07-27 10:36:17 UTC
Rebased onto the freshly-merged xclaesse/contacts-refactor (yum, conflicts), checked that that branch does indeed fix the one remaining feature that was broken (AVATAR_DATA), amended the test accordingly, fixed the review feedback, and merged the lot. That felt good.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.