Bug 35633

Created attachment 44790 [details]
application and tp-qt4 log

Created attachment 44791 [details]
dbus log

By status, you mean the presence, right? How, exactly, does showing it fail? And by "passing a features param" you mean passing those features to the ContactFactory::create() method? That should indeed work.

So, if I interpret you correctly, passing those features to the ContactFactory your Jabber account doesn't display remote contact presence correctly (though, how is it incorrect exactly?). If this is the correct interpretation, then please post the D-Bus session bus log and application log with tp-qt4 debug output *for the Jabber account only*.

(In reply to comment #3)
> By status, you mean the presence, right? 
Yes, I mean the presence.

> How, exactly, does showing it fail?
It doesn't "fail". And if we check, the feature is listed and all. It's just that all presence fields are empty. So, we get no data at all, but no error either.

> And by "passing a features param" you mean passing those features to the
> ContactFactory::create() method? That should indeed work.
Yes, that's what I mean. And it works for one protocol, but not for the other.

> 
> So, if I interpret you correctly, passing those features to the ContactFactory
> your Jabber account doesn't display remote contact presence correctly (though,
> how is it incorrect exactly?). If this is the correct interpretation, then
> please post the D-Bus session bus log and application log with tp-qt4 debug
> output *for the Jabber account only*.

By correctly, I mean that if I request the presence feature in the ContactFactory::create() statement, the presence data should be there.

I'm on travel right now, but I'll try to get you the logs as soon as possible.

Created attachment 45309 [details]
dbus-monitor log

I'm adding some logs

Created attachment 45310 [details]
application debug messages

Created attachment 45311 [details]
telepathy-gabble debug messages

Branch in URL fixes the issue. Contact::augment really needed some love. ContactManager used to call Contact::augment only on contact construction which is not true anymore with contact-fallback code in place and subsequent calls without the proper attrs was erasing already known data even if the attr was not requested.

make check passes and it fixes the issue as tested by Alvaro (Hei_Ku).

Sorry, but this is definitely a no-go. The test coverage here is insufficient, so the test suite passing in this regard is quite inconclusive. Crappy, I know. (Actually, coverage-wise lots of the code is executed, but the result to the various accessors isn't being verified enough).

+        mPriv->actualFeatures.insert(feature);
+

You can't always insert the feature to actual features. You're currently marking all features as being present as long as they're reported to be supported by the contact manager, even if the supplied attributes don't include their data (such as when we're in a fallback code path, for example the PendingContacts InspectHandles path). For e.g. FeatureAvatarToken this matches the desired behavior, but for e.g. FeatureAlias it doesn't: we're marking FeatureAlias as present although we haven't necessarily received an alias at all!

As a fix, move this to after the if pile, which has all those continues on missing data which'd then skip it? You need to keep the "it's fine if it's missing" branches there then, though. Note that (most of? all of?) the receive* methods already do the insertion to actual features, so isn't this (or that) redundant anyway?

AFAICS the only parts of the patch which actually contribute to fixing the reported problem are the added if branches which skip altering attributes if they're missing from the map. This could be done more elegantly with an array/map of (feature, property) pairs which would be checked between checking the manager supported features and declaring the feature to be in actual features (if that is even done in the current way). You need to make sure the fallbacks are filled in this case, though: such as making the reported alias equal the id if we don't have an alias yet from any earlier augment() invocation, and ditto for initializing presence to unknown.

I actually think I know what the real problem here is - ContactManager has this code:

            if ((realFeatures - contact->requestedFeatures()).isEmpty()) {
                // Contact exists and has all the requested features
                satisfyingContacts.insert(handle, contact);

And as you say, we're calling augment() all over the place with all of the ContactManager features but with only the id in attrs, so the contact's requested features will contain a lot of things, and ContactManager will skip actually fetching the attributes. Should this perhaps be changed to check for (realFeatures ∩ supportedFeatures()) \ contact->actualFeatures() being empty? It worked before because we only called augment() when we actually had all of the attributes retrieved, so contact->requestedFeatures() didn't grow prematurely.

And please, try to keep the "cleanup" and the functional changes in separate commits.

So ignore the whole thing. Discussed with Olli on IRC and we came up with a different solution. Branch updated accordingly. Alvaro also tested the new solution and it works.
As a plus we now have regression tests for this issue and also roster example is updated to be more exemplary.

Thanks, the new fix is much better. The test and tp-qt4 parts are OK to merge, but the example needs a small adjustment:

you're currently downloading allKnownContacts immediately after receiving a new Connection, with no regard to the ContactManager's state. This works if the connection was there already when starting the example, but is racy on connection changes - and a racy implementation in example code is a big no-no. You need to only download allKnownContacts when the state is Success.

This again furthers my thinking that the ContactList D-Bus API should have separate TemporaryFailure and PermanentFailure states instead of the current Failure state, so that we could have a feature which only completes until the ContactManager moves away from the TemporaryFailure state (at which point allKnownContacts() will be populated, if ever) which apps like this could use.

(In reply to comment #11)
> Thanks, the new fix is much better. The test and tp-qt4 parts are OK to merge,
> but the example needs a small adjustment:
> 
> you're currently downloading allKnownContacts immediately after receiving a new
> Connection, with no regard to the ContactManager's state. This works if the
> connection was there already when starting the example, but is racy on
> connection changes - and a racy implementation in example code is a big no-no.
> You need to only download allKnownContacts when the state is Success.
> 
> This again furthers my thinking that the ContactList D-Bus API should have
> separate TemporaryFailure and PermanentFailure states instead of the current
> Failure state, so that we could have a feature which only completes until the
> ContactManager moves away from the TemporaryFailure state (at which point
> allKnownContacts() will be populated, if ever) which apps like this could use.

How could I forget that? I am all for a PermanentFailure state and a feature to make this easier. In the meantime updated the roster example to listen to ContactManager state change signal.

Uh... with your latest patch, what if the initial state is already Success? Exactly, it stays forever waiting for a change to the success state. So check the initial state too...

Patience, my friend!

(In reply to comment #13)
> Uh... with your latest patch, what if the initial state is already Success?
> Exactly, it stays forever waiting for a change to the success state. So check
> the initial state too...
>
You missed this call:
+ onContactManagerStateChanged(conn->contactManager()->state()); 
It's done right after connecting to the signal, so it would catch initial state also.

Indeed I did! The branch is good, merge away!

The branch was so good, I merged it myself. Happy easter.