+++ This bug was initially created as a clone of Bug #55391 +++ On that bug, I wrote: > Other things to do include: > > McdConnection has quite a lot of tp_connection_get_self_handle(). The > simple fix would be to copy that function's implementation every time > we call it - but it would be better if we used a TpContact for the > self-contact, attached it to the McdAccount instead of the McdConnection, > and used high-level APIs instead. This should be done separately for > aliasing, presence, avatars, and any other offending interfaces. Let's do Aliasing first. It turns out to be a small net code-deletion: src/mcd-account.c | 183 ++++++++++++++++++++++++++++++++++++++------- src/mcd-connection-priv.h | 3 - src/mcd-connection.c | 147 ------------------------------------ 3 files changed, 157 insertions(+), 176 deletions(-) (when tests are ignored)
Created attachment 68126 [details] [review] McdAccount: store the self-contact
Created attachment 68127 [details] [review] McdAccount: take responsibility for setting the nickname As well as being implemented in a more streamlined way (using the self-contact instead of calling D-Bus methods directly), this reduces the circular dependencies between McdConnection and McdAccount.
Created attachment 68128 [details] [review] Don't set the nickname to the normalized name on connect We still allow the nickname to be set to the normalized name by user action later, because in that case, the intention is unambiguous: the user really does want their nickname to take that value. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=39381 --- The observant will notice that this change is much, much smaller than the one on Bug #39381 :-)
Created attachment 68129 [details] [review] enable_fakecm_account: copy parameters and alter the copy Mutable defaults for arguments are dangerous. If we call enable_fakecm_account twice with default arguments, when we alter expect_before_connect/expect_after_connect, we are actually altering the default! Python doesn't construct a new empty list for us every time. The C equivalent would be a static GPtrArray * or something.
Created attachment 68130 [details] [review] nickname test: verify that a trivial nickname is not set on connect --- This is Bug #39381 really.
Created attachment 68131 [details] [review] McdAccount: cope with self-contact changes; follow the self-contact --- This doesn't actually gain us any correctness (but is harmless) until we fix Bug #55666 in telepathy-glib. Software is hard.
Created attachment 68132 [details] [review] Add a regression test for an IRC-like protocol IRC has plenty of oddities, but the one we're interested in here is that your nickname and your unique identifier are inextricably linked.
Created attachment 68133 [details] [review] Knock out most of the IRC test until fd.o #55666 is fixed
Created attachment 68136 [details] [review] Emulate the Aliasing interface a little more realistically
Created attachment 68137 [details] [review] Track Avatars using McdAccount, and use the self-contact for it --- This removes what appears to be a completely redundant call to GetKnownAvatarTokens() whenever we receive AvatarUpdated. That's pointless - we just got the avatar token! - so we don't need to do it. Unfortunately, one of the tests asserted that we did. The previous commit is needed because without it, the TpContact for the self-contact in account-manager/auto-connect.py will fail to prepare in a reasonable time (TpContact calls GetAliases() but the test never replies to it), and we'll never actually upload the new avatar. The test would work if it took longer than 25 seconds to give up, but that would be ridiculous.
Did not read all the patches in details, just noticed that you upgrade the self contact manually. I don't know how far we got with introducing factory into MC, but we could define those contact features on the factory so they become part of TpConnection's CORE feature. otoh, we need to be careful here. With tp-glib master no other contacts than connection's self will get prepared (TpChannel's contacts are prepared only if TP_CHANNEL_FEATURE_CONTACTS). But in next, TpChannel's target/initiator are part of CORE and group contacts are prepared as part of GROUP (which is not CORE anymore). I've also seen that MC prepares GROUP to leave channels so that will be an issue if we put contact features on the factory.
(In reply to comment #11) > Did not read all the patches in details, just noticed that you upgrade the > self contact manually. Yeah, that was deliberate, to not upgrade any other contacts that might exist. I think it's right for the self-contact to be a bit of a special case. Unless we add a separate set of self-handle-features to the factory, I think what I'm doing here is more appropriate than using the factory. > But in next, TpChannel's target/initiator > are part of CORE and group contacts are prepared as part of GROUP (which is > not CORE anymore). Right, that's a waste of time for MC.
Comment on attachment 68127 [details] [review] McdAccount: take responsibility for setting the nickname Review of attachment 68127 [details] [review]: ----------------------------------------------------------------- ::: 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 @@ +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. @@ +4560,5 @@ > + g_ptr_array_unref (contacts); > + } > + else > + { > + WARNING ("failed to prepare self-contact: %s", error->message); Same here, should we downgrade to DEBUG?
Comment on attachment 68128 [details] [review] Don't set the nickname to the normalized name on connect Review of attachment 68128 [details] [review]: ----------------------------------------------------------------- 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?
Comment on attachment 68131 [details] [review] McdAccount: cope with self-contact changes; follow the self-contact Review of attachment 68131 [details] [review]: ----------------------------------------------------------------- ::: src/mcd-account.c @@ +4590,5 @@ > + if (self_contact != self->priv->self_contact) > + { > + g_clear_object (&self->priv->self_contact); > + self->priv->self_contact = g_object_ref (self_contact); > + } I would early return if self->priv->self_contact == self_contact, this would avoid useless extra work. @@ +4631,5 @@ > tp_connection, dbus_error, details); > > + tp_g_signal_connect_object (tp_connection, "notify::self-contact", > + G_CALLBACK (mcd_account_self_contact_changed_cb), account, > + G_CONNECT_SWAPPED); swapped is not useful here.
Comment on attachment 68133 [details] [review] Knock out most of the IRC test until fd.o #55666 is fixed Review of attachment 68133 [details] [review]: ----------------------------------------------------------------- ::: tests/twisted/account-manager/irc.py @@ +68,5 @@ > q.dbus_return(set_aliases.message, signature='') > > + # FIXME: fd.o #55666 in telepathy-glib breaks the rest of this test. > + # Reinstate it when we depend on a version that has that fixed. > + return It is fixed now
Comment on attachment 68137 [details] [review] Track Avatars using McdAccount, and use the self-contact for it Review of attachment 68137 [details] [review]: ----------------------------------------------------------------- ::: src/mcd-account.c @@ +1382,5 @@ > +static void > +mcd_account_self_contact_notify_avatar_file_cb (McdAccount *self, > + GParamSpec *unused_param_spec G_GNUC_UNUSED, > + TpContact *self_contact) > +{ Actually you don't need the unused_param_spec and self_contact args, which is the point of connect_swapped. @@ +1429,5 @@ > + g_free (uri); > + return; > + } > + > + if (G_UNLIKELY (len > G_MAXUINT)) You're probably already OOM-killed at this point, but ok :) @@ +4769,5 @@ > + > + 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
(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.
(In reply to comment #18) > > 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. mcd_account_set_string_val() "normalizes" an empty string to NULL anyway, so using tp_str_empty() here wasn't actually a behaviour change. When we connect, if we have no nickname stored locally, I think it's correct to try to get it from the remote connection - this means we do the right thing when creating e.g. a Gabble account and not specifically setting a nickname (we'll get the one from the server). I think it makes sense to standardize on "nickname of '' is bad, try to do something better". I'll append a patch to make Set("Nickname", "") use the normalized name too.
Created attachment 68339 [details] [review] Downgrade failure to set alias/avatar from WARNING to DEBUG
Created attachment 68340 [details] [review] Downgrade failure to prepare self-contact from WARNING to DEBUG
Created attachment 68341 [details] [review] nickname test: test what happens when we set an empty nickname
Created attachment 68342 [details] [review] Implement Set("Nickname", "") by using the normalized name instead This is consistent with what we do when we first connect, and ensures that we always have some sort of nickname.
Created attachment 68343 [details] [review] Ignore changes to the avatar of former self-contacts
Created attachment 68344 [details] [review] mcd_account_process_initial_avatar_token: don't take self_contact argument It can be retrieved from the McdAccount.
Created attachment 68345 [details] [review] mcd_account_self_contact_changed_cb: ignore no-op changes
Things I haven't changed from Xavier's review: use of swapped signal arguments; semi-reversion of IRC test until we depend on a good enough telepathy-glib.
Ship it :)
Shipped it. It'll be in 5.15.0.
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.