[Bug 55668] use the self-contact to handle Aliasing and Avatars

[bugzilla-daemon at freedesktop.org]

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

--- Comment #18 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
(In reply to comment #13)
> McdAccount: take responsibility for setting the nickname

> ::: src/mcd-account.c
> @@ +1318,5 @@
> > +    gpointer user_data,
> > +    GObject *weak_object)
> > +{
> > +  if (error)
> > +    WARNING ("%s", error->message);
> 
> Do we want a warning here? We usually uses DEBUG() for failing dbus calls

Yeah, perhaps. MC is pretty inconsistent about this; it uses WARNING() in at
least some situations where it really shouldn't, like failed channel requests.

> @@ +4546,5 @@
> > +      if (self_contact == self->priv->self_contact)
> > +        {
> > +          tp_g_signal_connect_object (self_contact, "notify::alias",
> > +              G_CALLBACK (mcd_account_self_contact_notify_alias_cb),
> > +              self, G_CONNECT_SWAPPED);
> 
> swapped is nice to avoid the GParamSpec useless arg, but in this case you
> use both self and self_contact, so swapped is not needed.

I feel as though the swapped form is nicer in situations where you'll call the
callback explicitly, like this:

+          tp_g_signal_connect_object (self_contact, "notify::alias",
+              G_CALLBACK (mcd_account_self_contact_notify_alias_cb),
+              self, G_CONNECT_SWAPPED);
+          mcd_account_self_contact_notify_alias_cb (self, NULL, self_contact);

but if you disagree, I can change this.

> @@ +4560,5 @@
> > +      g_ptr_array_unref (contacts);
> > +    }
> > +  else
> > +    {
> > +      WARNING ("failed to prepare self-contact: %s", error->message);
> 
> Same here, should we downgrade to DEBUG?

Yeah, probably.

(In reply to comment #14)
> Don't set the nickname to the normalized name on connect
...
> Wondering what happens if I set my nickname to empty string. Won't
> "notify::alias" be emitted to normalized id, and then
> mcd_account_self_contact_notify_alias_cb() will set normalized id as account
> nickname?

Yes. I think that's probably a feature - setting an empty nickname makes so
little sense that I think we should quietly "correct" it.

I should maybe write a regression test for this, though.

(In reply to comment #15)
> McdAccount: cope with self-contact changes; follow the  self-contact

> I would early return if self->priv->self_contact == self_contact, this would
> avoid useless extra work.

Yeah, fair.

> swapped is not useful here.

As above.

(In reply to comment #16)
> Knock out most of the IRC test until fd.o #55666 is  fixed
...
> It is fixed now

As I said in the comment, it isn't really fixed until it's fixed in a
telepathy-glib release, and we depend on that release.

(In reply to comment #17)
> Track Avatars using McdAccount, and use the self-contact  for it

> Actually you don't need the unused_param_spec and self_contact args, which
> is the point of connect_swapped.

This callback does use self_contact, just as a way to not have to fish it out
of self->priv again.

Strictly speaking we should be early-returning if self-contact !=
self->priv->self_contact too, to cope with our unique ID changing; in principle
we might have to rename ourselves in telepathy-salut, which has avatars
(although I doubt we implement it correctly in Salut anyway).

> > +          tp_g_signal_connect_object (self_contact, "notify::avatar-file",
> > +              G_CALLBACK (mcd_account_self_contact_notify_avatar_file_cb),
> > +              self, G_CONNECT_SWAPPED);
> > +          mcd_account_process_initial_avatar_token (self, self_contact);
> 
> You don't need that self_contact arg, it is self->priv->self_contact

If it isn't an argument, then mcd_account_process_initial_avatar_token() will
need to assert that self->priv->self_contact is non-NULL. That seems
reasonable, though.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.