Open TobiasFella opened 3 months ago
As a user of libquotient in a an app that isn't primarily a Matrix (chat) client, having the library handle more of this out of the box would indeed be much appreciated :)
Wow, this is comprehensive :) I wasn't particularly happy with this part of the library for a long time, and my ideas overlap with a lot of this. A few considerations below:
Connection
lifecycle - specifically, to produce it fully set up at the moment of a successful login (around the moment currently in Connection::Private::completeSetup()
and destroy it at the moment of a (hard) logout completion. This answers your question with regards to things like isLoggedIn()
and loggedOut()
- I'd like to get rid of them.PendingConnection
(or should we call it LoginFlow
?); we'll probably need an abstract base class for it and implement specific flows in derived classesConnectionSettings
idea, looks like a good way to decouple the settings from actual connection objects. Not sure if we should allow skipping the account registry though.AccountRegistry
changes.I'm not sure if Connection
should still be a base class and provide a protected interface; I see NeoChat using it quite successfully, but we should probably give a bit more thought to what (else) Connection
should provide for derived classes.
Looking once again at it, I wonder if it would make more sense to have login functions in Homeserver
, rather than AccountRegistry
. We could even make Homeserver
a base class so that client authors could add their own fancy login methods.
Not sure if we should allow skipping the account registry though.
Sure, that's not something I want to push for anyway
I wonder if it would make more sense to have login functions in Homeserver, rather than AccountRegistry
That would also make sense from an API perspective. I've put it in AccountRegistry for my proposal since then we automatically have the accountregistry object that is needed during creation of the connection
Problem
Currently, handling connections in libquotient-based clients is complicated.
To log in a client, the flow is roughly:
Connection
objectConnection::resolveServer(mxId)
to get the server's url and query the login flowsConnection::loginWithPassword(mxId, password, ...)
Connection::connected
AccountSettings
)Quotient::AccountRegistry
)To load an existing connection:
AccountSettings
Connection
objectConnection::assumeIdentity(mxId, accessToken)
Connection::connected
Connection::loadState
Quotient::AccountRegistry
)(I'm skipping over SSO login and account registration here. From libQuotient's perspective, account registration doesn't really exist; the relevant API here is the same as for login. For SSO login,
SsoSession
exists)There are some further complications here, e.g., that a client might want to wait until after the initial sync / load from cache before showing the connection (or switching away from a loading screen) to make the UX nicer.
With sliding sync, OIDC login, and my work on making libquotient work offline (#777), this would not get simpler.
Overall, there are too many things that are hard to understand, making the developer expierence not amazing.
There's also the - somewhat failed - attempt to improve this by providing the account loading functionality in
Quotient::AccountRegistry
.Goals
Connection
type is always ready to be used by a client, in the sense that it has the necessary userid, deviceid, access-/refreshtoken, crypto state, etc. that is required for operation. Notably, this does not mean that the homeserver must be reachable / was already reached (see #777)Proposal
Connection
uses its public ConstructorsThe following functions are removed (at least from public API, might be kept as internal API as needed. we should look into removing functions that clients don't need from public API, but that's a different topic):
Connection::isLoggedIn
(needs some further thought. should connections be automatically be destroyed when logging out? At least, it should not be relevant for checking whether a connection is "loaded" yet)Connection::loadState
,Connection::saveState
: Handled automatically, depending on configurationConnection::prepareForSso
,Connection::resolveServer
,Connection::setHomeserver
,Connection::assumeIdentity
: Implementation details that the client shouldn't need to care aboutConnection::sync
: maybe, might be useful for situations where we want to selectively sync,Connection::syncLoop
,Connection::stopSync
: Handled automatically depending on configurationConnection signals are removed / changed as needed:
Connection::resolveError
needs is moved toPendingConnection
Connection::homeserverChanged
is moved (or removed entirely, as needed)Connection::loginFlowsChanged
is removedConnection::connected
is removed (probably. See #777)Connection::loggedOut
is removed if we end up deleting connections automatically on logoutConnection::stateChanged
is removedConnection::loginError
is moved toPendingConnection
A new
Homeserver
class is added (name up for debate). It's intended usage is:AccountRegistry
is kept roughly as-is:AccountRegistry::add
andAccountRegistry::drop
are dropped from public API (maybe?)new functions
PendingConnection AccountRegistry::loginWithPassword(mxId, password)
PendingConnection AccountRegistry::restoreConnection(mxId)
PendingConnection AccountRegistry::loginWithSso(homeserverUrl)
PendingConnection AccountRegistry::loginWithOidc(url)
PendingConnection AccountRegistry::registerAccount()
(maybe we need a different name forAccountRegistry
to make it clear that this is not about adding a name to the registry)A function that lists the mxIDs of all connections that the are stored in state cache, i.e., that can be loaded
Maybe a convenience function to automatically load all connections
PendingConnection
PendingSsoConnection
replacesSsoSession
The functions for getting connections (
AccountRegistry::loginWithPassword
, etc.) take an optionalConnectionSettings
parameter, which configuresSemi-related: