[Telepathy] TelepathyQt4: State of the Connection

[Olli Salli]

Hi,

I've worked for the past few days adding features to Contact objects
on TelepathyQt4, and other Contacts misc (contact-features branch on
my personal tp-qt4 branches repository). This required adding some
more introspected things to the Connection class and ideally would
have included some new Connection::Features for a Contact representing
the connected account itself. However, during the course of this work
I've noted some rather rampant deficiencies both in the current API
and implementation of Connection (as of the recent changes related to
adding "Features" and the new becomeReady / isReady API). (tbf,
horrible hacks were required to make the Connection implementation
cope with the additions - such additions should be trivial to add
without touching existing logic so widely)

I might have understood some parts (especially in the implementation)
incorrectly, feel free to correct me if I'm wrong.

Most importantly, there are multiple notions of how ready for use the
Connection is both in the internals and in the API:
 - status() / statusReason()
   - Presumably intended to represent both the actual D-Bus Connection
status, and the fact that "everything interesting" for that status is
downloaded
    - Actually, that "everything" depends on what happens to be in a
central introspect queue by the time the D-Bus statusChanged signal is
received AND ALSO what is added before the "core" introspection is
completed
     - stuff can be added to the introspect queue by eg. becomeReady
 - mPriv->readiness
   - Inherited from the pre-featurified Connection
   - In the old code, intended for much the same use as the current
"status()" - but "everything" meant really everything (partly because
there were no features)
   - In the current code, used presumably as an implementation helper
to need less rewriting for the status() API - however, the mapping is
a bit flaky
   - rather directly linked to status() - that only changes after a
readiness change
   - used for warning about incorrect public API accessor usage, but
not really sanity-checkable from the public API
   - changes whenever continueIntrospection is called with an empty
introspect queue, whether the in-flight introspect functions are
actually finished or not
    - inherited from the old non-featurified Connection - there, only
the introspect callbacks called continueIntrospection, so there was a
clear chain of events - the introspect queue did never accidentally
advance
    - currently, also becomeReady can call continueIntrospection -
messing up a possibly currently running introspection
    - additionally, a D-Bus status change can fire up a new
introspection for the Connected status even if there is currently a
running introspection for the Disconnected status
     - this was also possible in the old implementation, but the
introspection made a distinction (using mPriv->initialIntrospection)
between the two introspectable statuses for the cases where this did
matter - this variable is no longer sufficiently set and checked,
leading to spuriously occurring races in Connection initialization
 - mPriv->ready
   - Intended to mean that at some point of the Connection's lifetime,
its status and supported interfaces have been downloaded
     - However, interfaces can be added when the state changes to Connected
      - Currently, this will be true if the interfaces were
introspected in the Disconnected state EVEN IF THEY AREN'T for the
Connected state
 - mPriv->features, mPriv->pendingFeatures, mPriv->missingFeatures
   - Intended to mean: "which Features are completely introspected, in
the process of being introspected, or found to be non-introspectable"
in ANY connection state
   - If something happens to added to this set in the offline state,
it will be there in the Connected state too
     - currently not a problem, because flags are only added in the
Connected state, but prevents adding anything which might work
pre-connected (like SimplePresence should be)
 - isReady(Features) / becomeReady(Features)
  - Should mean: "the connection core AND the requested features are
downloaded for the current status"
  - considers mPriv->ready and mPriv->features
  - doesn't consider status() and/or mPriv->readiness
  - Actually does mean: the core is downloaded for some status, and
same for features
    - although as mentioned above, features can't be there except for
status Connected
    - However, the "core" can be there from the Disconnected state
      - current behavior when the Connection has just turned
Connected: becomeReady(<no features>) returns an immediately finishing
operation when the Connection is actually still just downloading the
core for the Connected state, if the core was downloaded in the
offline state
  - not extensible to features which might not work in the offline state too

I have to run now, will provide insight on how I would cleanly
implement the desired behavior later on - stay tuned. And please
review my contacts branch :)

-- 

Br,
Olli Salli