[Telepathy] Review of MissionControl spec

[Alberto Mardegan]

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 only report it to the 
user: when they request a channel they use 
MissionControlRequestChannelWithStringHandle() which cares to create the 
connection if it's not there; AFAIK, they don't operate directly on the 
connection. They do operate on channels, though, and I expect that 
before invalidating the channels Telepathy CMs will send a Closed signal 
on them.

[...]
> On the other hand, perhaps we also want connection status monitoring that
> doesn't involve talking to the Connection directly, for the
> benefit of things like the "how online am I?" tray applet in Maemo.

Yes, this is the typical kind of application that will not be talking 
with any CM, but only with MC.

> 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.

[...]
>>> UpdateParameters() -> a{sv}
>>> UnsetParameters(as)
>>> Do we really need this, or is setting a parameter to its CM (or preset)
>>> default equivalent to unsetting it? We just don't know. Something to think
>>> about.
>> I tend to agree that "SetParameters" + "UpdateParameters" is probably 
>> the most useful combination; "UnsetParameters" can be dropped, IMHO.
> 
> In something of a U-turn, on the way home yesterday I was pondering
> replacing all of these with UpdateParameters(a{sv}: set, as: unset),
> which can do everything the separate methods can.

Mmmm... it does, but I think that Set+Update is much easier to use for 
an account editing application: while it will know for sure which are 
the parameters to be set/updated, it might not know which have to be 
unset. To know that information, either it should keep track of the 
previous parameter value, or it should go through the CM protocol and 
see the complete list of parameters.
While there could be a library API for doing that, I think the simplest 
solution is to have two different Set+Update.
(...some minutes of thinking...)
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?

> I'm not sure whether things that are explicitly set to the default
> should be stored as such, or deleted. I slightly lean towards "stored", because
> that way, UIs can easily implement the other behaviour if they want it.

They definitely have to be stored.

> It's unclear whether we want an updated preset or .manager to take
> precedence over what the user has seen and (at least passively) approved in a
> dialog box already.

This logic belongs to the account editor UI, but I'd say that if a 
parameter is set in the account, then it will have the highest 
precedence over presets defaults. So I guess that an account editor UI 
will choose either not to even show default values, or show them in such 
a way that makes clear they are default that will be used if the user 
doesn't overwrite them. And they should not be stored in the account, 
unless the user explicitly sets them.

>> 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.
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.

Ciao,
   Alberto


-- 
http://www.mardy.it <-- Geek in un lingua international!