[Bug 35633] ContactFactory does not introspect Contacts correctly for all protocols

[bugzilla-daemon at freedesktop.org]

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

Olli Salli <ollisal at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|                            |review-

--- Comment #9 from Olli Salli <ollisal at gmail.com> 2011-04-22 03:39:17 PDT ---
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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.