Bug 37112

See my branch, with some other fixes.

> 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 ?

(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.

(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.

(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?

(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 ?

(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?

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.

(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.

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.

forgot:

[1] http://cgit.collabora.com/git/user/xclaesse/telepathy-glib.git/log/?h=contact-list

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).

Would be good to have this fixed for Empathy 3.2. A lot of users are facing alias issues in logs.

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 ?

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.

The patch looks fine if it works.

(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.

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.

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.

It is indeed .6 who added that. +1

Will be in 0.2.11