Bug 37112 - Doesn't wait to get contact alias before adding event
Doesn't wait to get contact alias before adding event
Status: RESOLVED FIXED
Product: Telepathy
Classification: Unclassified
Component: logger
git master
Other All
: medium normal
Assigned To: Nicolas Dufresne
Telepathy bugs list
:
Depends on: 38142
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-11 06:28 UTC by Jonny Lamb
Modified: 2011-10-31 10:55 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Select feature, and cleanup singleton allocator (6.67 KB, patch)
2011-10-14 11:10 UTC, Nicolas Dufresne
Details | Splinter Review
Fix observer singleton and select features (7.10 KB, patch)
2011-10-14 14:52 UTC, Nicolas Dufresne
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonny Lamb 2011-05-11 06:28:02 UTC
Sometimes the TpContact for the sender of a text message is not prepared enough to actually have the alias, so in the logs instead of name="Nicolas Dufresne" it stores name="nicolas@dufresne.ca". Not sure why this happens only sometimes.

I'm not familiar with any of the logger code, but looking at the XML of the text chats logged, the TpContact should have at least the TP_CONTACT_FEATURE_ALIAS feature somewhere.

This was highlighted as I just fixed Empathy[0] to actually show aliases for logged messages so when you open a chat with a contact you see the last five messages sent between you, but aliases are used instead of their contact ID.

0. http://git.gnome.org/browse/empathy/commit/?id=acde6c67cdd3c
Comment 1 Jonny Lamb 2011-05-11 08:19:40 UTC
See my branch, with some other fixes.
Comment 2 Nicolas Dufresne 2011-05-11 12:07:04 UTC
> text-channel: fix typo

There was other case around where the same typo was made. Also the GError where not freed. I've allowed myself rewriting it correctly and merged.

> all: fix -Wunused-but-set-variable warnings
> log-store-xml: fix -Wuninitialized warnings

All good, merged. I did that since I realized that some change on the 32bit builder would cause those to come out.

> text-channel: use the TpContact we've already prepared for received messages

This one is not a proper fix. Each message may contains sender-nickname header, which must be used as alias. If not present, probably take one of our prepared account, and if we don't have one, then maybe we should call _prepare_async() to finish preparation ?
Comment 3 Jonny Lamb 2011-05-12 01:25:40 UTC
(In reply to comment #2)
> There was other case around where the same typo was made. Also the GError where
> not freed. I've allowed myself rewriting it correctly and merged.
[snip]
> All good, merged. I did that since I realized that some change on the 32bit
> builder would cause those to come out.

Thanks, but you know I have commit access and could have done this myself like a big boy?

> This one is not a proper fix. Each message may contains sender-nickname header,
> which must be used as alias. If not present, probably take one of our prepared
> account, and if we don't have one, then maybe we should call _prepare_async()
> to finish preparation ?

True that, I forgot about sender-nickname (even though I was the one who added it!) Butterfly is the only CM which makes use of it. I think we should just fall back to using the priv->remote contact if sender-nickname is not present.

I've no idea what you mean about "take one of our prepared account" and "call _prepare_async() to finish preparation".

I updated my branch.
Comment 4 Nicolas Dufresne 2011-05-12 07:46:24 UTC
(In reply to comment #3)
> (In reply to comment #2)
> Thanks, but you know I have commit access and could have done this myself like
> a big boy?

Don't take offense, I do that only when build is broken on the buildbot.

> 
> > This one is not a proper fix. Each message may contains sender-nickname header,
> > which must be used as alias. If not present, probably take one of our prepared
> > account, and if we don't have one, then maybe we should call _prepare_async()
> > to finish preparation ?
> 
> True that, I forgot about sender-nickname (even though I was the one who added
> it!) Butterfly is the only CM which makes use of it. I think we should just
> fall back to using the priv->remote contact if sender-nickname is not present.
> 
> I've no idea what you mean about "take one of our prepared account" and "call
> _prepare_async() to finish preparation".

"take one of our prepared account" == priv->remote

"call _prepare_async() ..."

What I mean is that at last resort, we should do the async prepare call on the TpContact sender object, even for rooms. Otherwise there will still be cases where it's not the alias that is being logged.

> 
> I updated my branch.

Will have a look.
Comment 5 Jonny Lamb 2011-05-12 07:55:48 UTC
(In reply to comment #4)
> What I mean is that at last resort, we should do the async prepare call on the
> TpContact sender object, even for rooms. Otherwise there will still be cases
> where it's not the alias that is being logged.

Ah I see. Yeah I'm aware this does not fix it in the MUC case, but it's better than before, and certainly no worse. It was just bugging me.

In what case would priv->remote not be set, apart from the MUC case where it's obviously not set to something useful?
Comment 6 Nicolas Dufresne 2011-05-12 08:03:51 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > What I mean is that at last resort, we should do the async prepare call on the
> > TpContact sender object, even for rooms. Otherwise there will still be cases
> > where it's not the alias that is being logged.
> 
> Ah I see. Yeah I'm aware this does not fix it in the MUC case, but it's better
> than before, and certainly no worse. It was just bugging me.

That's look good for 1:1 chat, but Why don't we also try sender-nickname for chat rooms ?

> 
> In what case would priv->remote not be set, apart from the MUC case where it's
> obviously not set to something useful?

priv->remote is always set and perepared. The Observer will not return until this one is prepared. I'm really focusing on the MUC case in my comment.

Originally we would keep a hash of all prepared contact, I've removed that hash
because we could not guaranty they where all prepared before the first message
comes in and I was assuming TpSignalledMessage was giving prepared TpAccount. I
think at the end, the only reliable solution for MUC would be:

if (chatroom)
  if (sender-nickname)
     create entity
   else
      _prepare_async()
else
  ...

Accepting that message may get miss-ordered if preparation does not return in
same order. Also, I still think this has to do with some bug in tp-glib, have
you spoke to Guillaume about that ?
Comment 7 Jonny Lamb 2011-05-12 08:53:32 UTC
(In reply to comment #6)
> Accepting that message may get miss-ordered if preparation does not return in
> same order. Also, I still think this has to do with some bug in tp-glib, have
> you spoke to Guillaume about that ?

What tp-glib bug?
Comment 8 Guillaume Desmottes 2011-05-13 00:28:10 UTC
One could argue that TpTextChannel should have some API to ask him "Please prepare these contact features before announcing messages". IIRC smcv wasn't keen about that, I don't remember the exact reason but that was probably the message re-ordering issue.
Comment 9 Nicolas Dufresne 2011-05-16 09:35:28 UTC
(In reply to comment #8)
> One could argue that TpTextChannel should have some API to ask him "Please
> prepare these contact features before announcing messages". IIRC smcv wasn't
> keen about that, I don't remember the exact reason but that was probably the
> message re-ordering issue.

Then I think we need a queue to guaranty storage ordered, and do call prepare if required. If we don't have to, and queue is empty, we store right away.
Comment 10 Xavier Claessens 2011-06-22 08:27:48 UTC
For the record, in my contact-list branch[1] I introduced features on TpAccountManager, TpAccount, TpConnection and TpChannel to wait before their "child" objects are prepared before exposing them. The factory contains the desired features to be prepared on all those objects, including TpContact.

So for example TpAccount would wait for its TpConnection to be prepared before emitting notify::connection signal and tp_account_get_connection() would still return NULL until the connection is ready.

In the same spirit, I suggest adding a feature TP_TEXT_CHANNEL_FEATURE_CONTACTS that ensure that all messages exposed have TpContact sender ready with all features desired on the factory, until then they are queued.
Comment 12 Xavier Claessens 2011-08-17 04:47:59 UTC
Now all the factory stuff needed are in master. bug #38248 is similar to this one to prepare TpContact objects for all TpChannel-related contacts (group members, initiator, etc).
Comment 13 Guillaume Desmottes 2011-09-07 02:47:11 UTC
Would be good to have this fixed for Empathy 3.2. A lot of users are facing alias issues in logs.
Comment 14 Nicolas Dufresne 2011-09-07 06:50:38 UTC
IIRC there is new stuff that will let us ensure that all Contact are prepared with a certain set of features. Can anyone brief me on how that work ?
Comment 15 Nicolas Dufresne 2011-10-14 11:10:48 UTC
Created attachment 52345 [details] [review]
Select feature, and cleanup singleton allocator

Sorry for mixing two things, but the implementation of this single was a real aberration.
Comment 16 Jonny Lamb 2011-10-14 11:25:30 UTC
The patch looks fine if it works.
Comment 17 Nicolas Dufresne 2011-10-14 14:34:55 UTC
(In reply to comment #16)
> The patch looks fine if it works.

I tested it with the latest version of tp-glib, but that reminds me I may have to update the tp-glib required version to match the first one where it works.
Comment 18 Nicolas Dufresne 2011-10-14 14:38:47 UTC
Just tested with Fedora 16 currently stock version 0.15.5, and this version does not work (while the API is there), so it must be a newer one.
Comment 19 Nicolas Dufresne 2011-10-14 14:52:23 UTC
Created attachment 52350 [details] [review]
Fix observer singleton and select features

The magic working TP Glib version was 0.15.6, so I've bumped the minimal version to that.
Comment 20 Xavier Claessens 2011-10-15 01:34:58 UTC
It is indeed .6 who added that. +1
Comment 21 Nicolas Dufresne 2011-10-31 10:55:03 UTC
Will be in 0.2.11