[Bug 29451] Too many asynchronous setup steps needed for basic usage

[bugzilla-daemon at freedesktop.org]

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

--- Comment #6 from Olli Salli <ollisal at gmail.com> 2010-08-18 03:50:40 PDT ---
Thanks for your input. Judging the alternatives, I'm settling towards (lost
this writeup already once thanks to a kernel crash, good times):
 - specify Channel subclass and features through the Client API, it's the only
place where one gets those anyway
   - so a ChannelFactory (made more dynamic from the current one) per Client
API "big object" (this may change from ClientRegistrar / the AbstractClient
instance to *something* whenever I have time to think about fitting the
requestAndHandle API in the picture, probably similarly to Tp-Glib though so
I'll just call it Client for now)
 - allow specifying Account / Connection (subclass and) features both through
AM and the Client API
   - tube clients and alike have Client or sometimes both, account editing,
presence etc applications/widgets have just AM, while bigger applications
likely have both, we should support all of these (and it's quite simple to do
so)
   - AM options must include "don't make any Connections ready please" for
account editing UIs etc which don't quite care about them
   - Client API delays calling the application until the objects are ready
   - new AM features {Accounts,Connections}Ready
   - AM delays becomeReady(FeatureAccountsReady) until the accounts are ready
(currently delays AM::FeatureCore until the Account::FeatureCore is ready for
all accounts, do we still want this even if the feature isn't specified?
otherwise backwards compat which we're about to break anyway suffers unless we
have becomeReady auto-add that feature and have an extra becomeReadyEarlyGoer
variant)
   - AM delays becomeReady(FeatureConnectionsReady) until both the contained
accounts and their connections are ready
   - new signal newReadyAccount(AccountPtr) which is delayed until the account
in question AND its connection, if any, are ready, newAccount retains current
semantics BUT still see my desperate arguments against this below
     - I don't think we need newReadyAccountButNotItsConnection - as previously
explained, in tp-qt4 this is not at odds with eg. preconnection auth channels
and I don't think the contact lists either
 - everything is object instance-specific, no library-global or
bus-connection-wide state
 - allow setting an AM to use on the Client API "big object", whatever it is in
the end
   - in this scenario, AM only waits for features specified on it to be ready,
while the Client first gets objects from the AM and upgrades any extra features
set on it before handing the objects over to the application
approving/handling/observing function implementation
     - allows for more granularity, specifying some extra features only for
accounts / connections the application handles channels from which sounds
useful
   - Client always waits the AM and sub-objects to be ready before attempting
to look-up Account/Connection objects to share - them being ready and a channel
to handle coming for an account/connection we don't see through the AM is
probably warning() zone
   - for tube clients operating without an AM, we probably want the following
though:
     - implement weak-pointer cache of Accounts constructed for previous
channel handling invocations
       - and Connections? probably not, and should just document that the
application should always store the Account rather than just the Connection, as
the Account already has a strong-ref to the Connection
     - if the application currently has an Account / Connection pair from a
previous channel, this avoids state re-download and enables handle/contact
interoperability between the two channels' contexts

The above leads me to the conclusion that AccountFactory and ConnectionFactory
should actually be one object (Called AccountObjectFactory? constructing
Connections is always Account-specific using it), because they always exist in
the same place (AM and/or Client). ChannelFactory should be separate though, as
it has somewhat different API due to being driven by the immutable properties,
and only exists in Client.

Open issue: at which stages of the program's lifetime do we want to allow
setting the subclasses and features?
 - AM semantics for setting features could be either
   a) just once, before calling becomeReady(Feature*sReady) on it for the first
time, this way it can be synchronous, and it's guaranteed non-co-operative
plugins etc can't add harmful extra delays semi-globally because just the host
application can do it
    or
   b) always, and make it upgrade existing objects - this way it'll have to be
a PendingOperation - is more flexible but also more dangerous? I'm not entirely
convinced the latter holds true though
    - make it add-only though to not confuse somebody having set it earlier on
with new objects not having their desired features, and document that if the
application wants say an "show avatars" menu toggle, it shouldn't use this and
instead upgrade objects in the old-fashioned way if that is enabled
    - for convenience, the AM becomeReady(SubObjects) calls should wait for
these requested pendingoperations to be ready so it doesn't have to be done
separately when initially setting up the object, similarly to a)
 - but subclass definitely can't be settable after the first object constructed
in either case
 - Client semantics should be "new Client operations are delayed accordingly to
the new settings, with no direct effect in Accounts and Connections given in
earlier operations" (not direct as in: except through the fact that it can be
the same object due to sharing)
   - can set features whenever convenient, subclass only before first object is
constructed - this means we should document it should be set before the Client
is registered on the bus
   - the Client Acc/Conn factory is unused if an AM is linked to it, except for
the additional features to wait for, because all objects are available from the
AM anyway so there is no conflict between the subclasses specified in the two
factories

This general approach still might seem at odds with your concerns, so I'll try
and defend my position next:

(In reply to comment #4)
> (In reply to comment #3)
> >  b) If application code wants more Account features than just Core (and
> > possibly Capabilities), it needs to call becomeReady() on all of the Accounts,
> > one by one, including any new ones from the newAccount() signal, and wait for
> > the operation to finish
> 
> There's a tension between this, and wanting new Accounts to pop up as soon as
> possible because it's signalling an event that, really, already happened.
> Perhaps we want two signals, newAccount and newlyReadyAccount...

Well, I added the separate signal to my plan, but I'm still not convinced the
not-quite-ready-for-the-use-you-asked-for-yet Account is useful, because tp-qt4
Account features are all semi-fast (neither downloading the Avatar over D-Bus
nor reading .manager files / querying the CM for ProtocolInfo does take that
long ie. make any network round trips). Again, tp-qt4 Features are designed to
allow cutting down state download and signal connections by not specifying
them, not really waiting for any slow-slow (network or user action dependent)
events to happen by specifying them. We tend to have change notification
signals for those.

In other words, DO WE HAVE AN USE CASE!!ONE (an actual tp-qt4 one, with no
hypothetical added slow features)? My concern is that we'd end up applications
upgrading accounts again and again with the same features like they're doing
currently if we have *any* API which doesn't guarantee the requested features
do be ready. Then again, maybe if we finally grew that ABOVEMEREMORTALS API
annotation we could use that to tack those parts to an out-of-reach advanced
section and just hint people towards them if we sense they need it for their
corner-case.

> 
> >  d) One can't specify the subclass of Account to construct in any case - even
> > by subclassing AccountManager itself, actually. Subclassability was one of the
> > original design goals for tp-qt4, but tbh I'm not sure if ever really cared
> > about it - do we?
> 
> It's worth keeping in mind, but not necessarily as a/the primary goal.

The big-object-instance-specific-factories approach described above allows us
to initially leave out the subclass specification but add it later on.

> 
> >  - maybe it's actually: In AM, use whatever features you recognize, otherwise
> > pass them to contained objects, and ditto for them
> 
> I'm not sure I like this meme: it makes it impossible to warn API users if they
> supplied a Feature that makes no sense, and will have no effect. Silent failure
> to act considered unhelpful :-)
> 

It could whine about features still unrecognized in the penultimate object, but
yeah, I don't like it either.

> >  - if the application hasn't requested and made ready a Connection using the
> > Account in question yet, one still needs to make Connections ready in the
> > application when doing channel handling (because it'll be newly created, or
> > made ready on a bus-wide singleton with some default set of features (core), if
> > there has been time to do so). This will probably happen often in handlers in
> > non-monolithic applications, where the application (eg. a tube client) is only
> > started and begins introspecting objects by service activation on the handler
> > method.
> 
> Can we approach this from another angle? In a Client, don't call the
> user-supplied HandleChannels, ObserveChannels or AddDispatchOperation callback
> (vmethod? slot? whatever this is in Qt-land) until certain features are ready.

Yeah, this was the original intention, and present in the refinement in this
post.

> 
> This solves the problem for every feature except Contact features in every app
> that is only a Client, which I think will actually cover a lot of practical
> use-cases, including all Tubes apps (which, long-term, we hope will be the
> widest class of application).
> 
> It's also safe for a telepathy-qt4 Client to have a flag for "don't invoke me
> until the Connection is CONNECTED" (even though telepathy-qt4 doesn't normally
> do this), because the Client knows whether it's useful on disconnected
> Connections (most aren't).

Yeah, instead of having a FeatureConnected this we could allow specifying this
just for the Client, with no effect on accessing the Connections through the
AM.

> 
> It doesn't solve anything for apps that start from the AccountManager and work
> downwards - account management UIs, status choosers, contact lists, and that
> sort of "furniture" - but it's a start.
> 

This posts' refinement does, by using the exact same AccountObjectFactory API
in both AM and Client, with the documented interaction between the two as
described in case both are used.

> > Another approach 2), formalizing on the hand-waving I did with Andre last week
> > and somewhat inscribed in the bug description is "object factories"
> 
> This seems like something we eventually want for subclassability, even if we
> don't want it for feature preparation.
> 

Yeah, so let's go all the way and specify both with it. This also gives us a
natural location to add any other automatic manipulations on the objects beyond
requesting Features on them if we come up with any (say, enable Contact
features on all Connection's ContactManagers constructed with it if we add
something of the sort to ContactManager).

> >  - we can make the factory instance / its settings either settable on a
> > specific AccountManager, bus-connection globals or library globals
> 
> No library globals, please; that breaks the case of two unrelated plugins in a
> non-Telepathy-aware plugin host (e.g. Maemo 5 status menu, gnome-panel, GEdit),
> each of which wants to use Telepathy and subclass stuff. Attaching a factory to
> an AccountManager instance could be a good way, though?

Yeah, in case there's no Telepathy common ground between the plugins (like
there would be eg. in plugins for a chat window app which already uses Tp for
the main functionality) they will each construct their own AM instances, which
solves the problem if we go for what I propose.

> 
> > If we make it somehow possible to specify the AccountManager
> > instance to co-operate with on ClientRegistrar / the Adaptors (AbstractClient*
> > is just a thin base class with no actual functionality!!!), we don't need a
> > (bus-wide) singleton AM or factories to enable constructing Accounts /
> > Connections with the desired factory settings.
> 
> I do like this. With hindsight, we should have probably made TpBaseClient's
> constructor take a TpAccountManager, not a TpDBusDaemon, so that it can behave
> similarly; I'll sort that out.

++

> 
> > This kind of link-up would also
> > allow the Account convenience channel request methods to be improved to allow
> > waiting for the requested channel to arrive, bypassing the usual handling
> > mechanism.
> 
> This is a Qt equivalent of Bug #13422
> (tp_account_channel_request_create_and_handle_async()), achieved by creating a
> Handler behind the scenes and making it preferred, and it's probably worth
> using a similar API:
> 
> http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-account-channel-request.html

I really must check out how this API fits into my proposed scheme next, will do
today.

> 
> What you already have is equivalent to Bug #29456,
> tp_account_channel_request_create_async().
> 

Yeah, thankfully there's no interaction between object construction and that
variant.

> > Caveats:
> >  - if some part of the application wants to access Accounts before all of their
> > features some other part of the application requested are ready, we must add
> > another "giveMeAccountsEvenIfTheyAren'tReady" feature - or more logically,
> > instead add AM::FeatureWaitForAccountsToBeReady which would be specified by
> > default, leading to the [1] behavior described above, but can be left out, such
> > that AM::becomeReady() will then return for that request before the accounts
> > are ready - currently Core is always waited for, maybe this is a suitable
> > middle-ground
> 
> Or, modules that only want to care about connected accounts can listen for
> accountConnected instead of accountAdded?

The above scheme does add the Feature AND at least for now the separate
signals.

> 
> >  - if a feature that only becomes ready when a connection is Connected is set
> > to be guaranteedly ready on a Connection, this makes not-yet-connected
> > Connections invisible, which makes for example handling authentication channels
> > impossible as Simon pointed out in the previous comment.
> 
> Specifying features per (TpBase|Tp::Abstract)Client solves this: if you handle
> authentication channels in your TpBaseClient, and you demand
> TP_CONNECTION_FEATURE_CONNECTED, then you're silly.

++

> 
> > Actually, there are no
> > such features on Tp::Connection - all features can be ready in any connection
> > state, but are re-introspected when the state changes, and signaling the state
> > change always waits for all previously requested features to be ready. (So we
> > actually have had a guarantee somewhat resembling the ones I'm suggesting here
> > for a long time - whenever statusChanged is emitted, any features previously
> > requested are ready for the new status)
> 
> Indeed, telepathy-glib features and telepathy-qt4 features don't correspond
> 1:1.
> 
> I'm a bit wary of the telepathy-qt4 design at the moment, because I've been
> investigating ContactList API for telepathy-glib. The contact list can be
> downloaded asynchronously after CONNECTED, can take time to download (in XMPP,
> multiple-minute lag is apparently entirely possible), and not signalling
> StatusChanged(CONNECTED) until the contact list has been retrieved, because a
> module over there -> asked for it before connecting, doesn't seem entirely
> desirable...
> 
> The sketch I have at the moment is:
> 
> TP_CONNECTION_FEATURE_CONTACT_LIST_INFO
>     Status/capability flags (CanChangeContactList, ContactListState etc.)
>     which are always available after you have CONNECTED
>     Implies TP_CONNECTION_FEATURE_CONNECTED
> 
> TP_CONNECTION_FEATURE_CONTACT_LIST
>     The actual contact list, with TpContact objects (with ID, handle
>     and subscription states only, at the moment), and change notification
>     Implies TP_CONNECTION_FEATURE_CONNECTED and ...CONTACT_LIST_INFO, and
>     also implies contact-list-state == TP_CONTACT_LIST_STATE_SUCCESS

I think currently tp-qt4 would go Ready when it has the Info and emit
allKnownContactsChanged when the contacts are actually fetched. The clients
need to listen for new contacts added anyway if eg. another client adds a
contact.

> 
> I haven't done anything about contact features yet. I was wondering whether to
> have a list of features to which you can only add (asynchronously) and never
> remove; after the async addition has finished, each TpContact on the contact
> list has all the features, and subsequently-added contacts aren't announced
> until they too have all of those features.
> 
> (This only works because all the contact features are "fast", though...)

This is equivalent to what I propose for the tp-qt4 Account, Connection and
Channel features too, given that tp-qt4 Account and Connection features are
also "fast" (ie. don't wait for network / user events to happen). Otherwise,
I'd like to postpone the contact features discussion for now. Having a factory
for connection objects allows us to add API auto-enabling contact features on
the relevant Connection's ContactManagers so I think we're future proof for
that.

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