[Telepathy] Review of MissionControl spec

[Simon McVittie]

On Wed, 23 Jan 2008 at 09:43:57 +0200, Alberto Mardegan wrote:
> ext Simon McVittie wrote:
> > Once the Connection is up and running, I think it's an incredibly bad
> > idea for clients that are actively using it *not* to watch the Connection's
> > status directly - as soon as the connection goes Disconnected, all handles
> > become invalid, all channels become useless, etc., and failing to
> > respond to this *will* cause problems (unexpected handle re-use).
> Yes; but I'd say that most clients (especially those in Maemo) are not 
> that interested in the connection status
> [...]
> They do operate on channels, though, and I expect that 
> before invalidating the channels Telepathy CMs will send a Closed signal 
> on them.

The problem with this approach is partly that CMs might be incorrectly
implemented, and partly that handles are entirely meaningless once the
Connection has gone away. If a UI can ever continue to manipulate
handles after the Connection has gone, then that UI is fundamentally
broken, IMO.

telepathy-glib 0.7.1 will force the issue by having the TpChannel constructor
require a TpConnection; I believe this is the only safe way forward for
UIs that handle channels. Note that the current HandleChannel interface
provides the object-path of the Connection to use.

(http://monkey.collabora.co.uk/stream-engine-smcv-tpglib071/patch-35.html
demonstrates how small a change this requires.)

> > In a way, we want these to be signals on the AccountManager or something,
> > rather than on the Account - if their goal is to aggregate info from
> > multiple connections to work around dbus-glib's inability to do wildcard
> > signal matching, there's no point in having that information
> > un-aggregated!
> 
> It's not necessarily aggragated. We also want an aggregated status, but 
> there might be a more detailed view where you see the statuses of each 
> account.

My proposal is to emit signals from the AccountManager that contain
separate statuses for all the connections (the "aggregation" I mentioned
is in having all the signals come from the same object for easier
signal connecting, rather than in merging the statuses with the
resulting loss of information).

> Actually, since we have a GetParameters() method which will return all 
> the parameters set for the account, it would be enough to have 
> SetParameters(a{sv}) method which sets the given parameters and unsets 
> all the rest. Any use-case where this would be an inconvenient API?

Trivial race condition. Imagine we write a migration tool that will tweak the
user's Gabble settings for optimal encryptedness, and some non-conflicting
migration tool is also running (perhaps it's, I don't know, setting the
'alias' parameter on all our accounts). If the calls interleave like this:

A: GetParameters
B: GetParameters
A or B: SetParameters
B or A: SetParameters

then whichever one called SetParameters first will lose. Special-purpose
tools like that, that tweak an isolated subset of parameters, should always
use an "update"- rather than "set"-style API.

> >> The account would disappear from the AccountManager: when MC starts, it 
> >> will check all the accounts, and if some required parameter is missing, 
> >> the account is considered invalid and ignored.
> > 
> > I don't like this at all! Silent loss of user data considered harmful.
> > Can't we just disable the account/flag it as invalid/etc., and let them sort
> > it out and enable it? Ideally, the CM-specific account UI (that we need
> > anyway) would include some sort of migration helper, which could even
> > exploit its CM-specific knowledge to make the corrections silently and
> > re-enable the account, in many cases.
> 
> I see your point. On the other hand, incomplete accounts is something 
> that we would like to avoid...
> My suggestion is to create a GetInvalidAccounts() method that returns an 
> array of the invalid accounts, where each element is composed by a 
> string ID identifying the account, a dictionary of all the configuration 
> keys (manager, protocol, display name, normalized name, alias, 
> avatar,...) with their value and their parameters dictionary. Then a 
> PurgeInvalidAccount() that takes the string ID and deletes the account 
> completely. The goal here is to have an account repair tool which calls 
> GetInvalidAccounts, and for each of the accounts it manages to "repair" 
> it calls CreateAccount to create a valid accound, then it deletes the 
> invalid one.

Invalid accounts are going to have all the stored properties of a valid
account, which sort of argues for them to be just an Account like any
other...

> But I definitely don't want to have invalid accounts being advertised by 
> the other AccountManager APIs, I'm afraid that would make everything 
> more complex.

Right, the "basic" AccountManager API used by most things should be
defined to only deal with accounts that we'd be reasonably happy to put
online. I'll bear that in mind.

    Simon