[Telepathy] Review of MissionControl spec

[Alberto Mardegan]

Hi,

ext Simon McVittie wrote:
> OK, I've actually started on this now...

Thanks! Much appreciated! :-)

[...]
> Related to dispatching channels is the concept of filtering. I'm
> somewhat fuzzy on what filtering, in fact, does: perhaps a Nokian could
> explain it to me, since I gather it's used fairly extensively in the
> current NMC.

Some examples of filters' tasks:
- Brighten up the display if it was off
- Check if we can accept the channel (memory status, blocked contact,...)
- Logging (you could have a channel handler for displaying the chat 
messages, and a different one for logging them)
- When you receive a call, set the system status to "Call", which will 
prevent the user for starting playback of a song, or maybe lower its 
volume a bit.

> Anyway, in principle there'd be nothing to stop these being entirely
> independent services, and I'm not convinced that we want to force there
> to be one process that does everything NMC does today.

In Nokia devices, filters are run inside the MC process; in the current 
API proposal, though, we want to allow them to reside on different 
processes.

> I think it would also be instructive to look at the proposed API from the
> point of view of "can we do this without MC?" rather than "might someone
> find this useful?". A lot of the API seems to provide short-cuts for
> things you can do right now, using existing Telepathy API. Perhaps some
> of it would be better implemented in libraries rather than as a D-Bus
> service; having just spent around 3 months on client API in
> telepathy-glib, I'm open to the possibility of making a client library
> that doesn't suck, rather than bouncing everything through a daemon
> because libtelepathy is too hard.

Makes a lot of sense.

[...]
> There's an awful lot of spec of the form GetBadger, SetBadger, BadgerChanged.
> I imagine George and Xavier got very bored while writing this. If we
> could code up a better Properties interface, we'd instantly obsolete
> about 60% of the text.

I don't know how properties work, but if they are simpler to implement, 
why not. :-)

> GetConnectionStatus() -> u / signal ConnectionStatusChanged(u, u)
> Do we really need this? Connections already signal it. If we had a
> robust way to go from an Account to a Connection, and from a Connection
> to an Account, clients could just do a wildcard signal match on
> StatusChanged (ignoring for the moment dbus-glib's inability to do such
> things - this is the wonderful world of open source, we could just add
> that code to dbus-glib and stop working around its absence.)

While I agree on the fact that it's not strictly needed, I don't know if 
we want to remove it; monitoring the connection status is something that 
MC has to do anyway, so I think it makes sense to expose it.

> RequestChannel(s: ctype, s: htype, u: handle)
> This method seems to me to be entirely useless - either we make it take
> a string contact ID (the same thing you pass to RequestHandle()) as Mika N
> wants, or we kill it. If you have a handle, then presumably you know which
> Connection you got it from, and that that connection is still alive (and if
> you don't, then you should re-read the Telepathy spec, paying particular
> attention to the part where we explain that handles are only valid for a given
> connection).

We definitely want a string version for the handle. About the 
use{ful,less}ness of this RequestChannel() method, it helps to inform MC 
that the NewChannel signal it will receive has to be processed by the 
channel handlers; so, we have three cases of NewChannel handling:

1) Incoming channel from the CM (suppress_handler = 0): MC will launch 
the channel handlers for the incoming channel.

2) Channel that was requested through MC RequestChannel() 
(suppress_handler = 1): MC will launch the channel handlers for the 
outgoing channel.

3) Channel requested directly to the CM by a client (suppress_handler = 
1): MC will ignore the channel completely.

I cannot think of any case where a client would want to bypass the 
filters, but I would leave the possibility open anyways. Well, on a 
second thought, this feature might also be used by a malicious program 
to make calls without the user noticing it, so maybe we might want to 
have MC process all NewChannel signals...

> UpdateParameters() -> a{sv}
> I'm going to contradict everyone and say I think we should have this
> *and* SetParameters(). A typical accounts widget a la Empathy or
> osso-accounts-ui sets all parameters simultaneously, so the appropriate
> thing to do is probably in fact an atomic overwrite (Set). If you're
> silly enough to press OK in two accounts dialogs simultaneously, the
> second one wins. However, if we have some sort of migration tool or
> something (perhaps a tool that goes through all your accounts removing
> deprecated parameters, or setting require-encryption) it might want to
> set only the parameters it cares about, to reduce races.
> 
> 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.

> On the subject of parameters: how do we cope if we have a value
> specified for a parameter the CM no longer supports? How do we cope if
> we have no value for something the CM now needs? Probably, disable the
> account and notify the user. The former case is tricky: most of the
> time, the user will be irritated, because the correct resolution is
> "obvious" and they'll resent having to say "OK". However, if the parameter
> being removed (from Gabble) is "old-ssl", silently removing it would
> make their connection no longer secure. Loss.

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. This can tipically happen if:
- Some required parameter was specified with a default value in the 
presets file, and the presets file is later modified or deleted;
- The list of required parameters in the .manager file grows.

I guess that both cases can be avoided, if some care is used.

> The solution here might be "well, don't do that" - deprecate obsolete
> parameters but never remove them. I suspect this would cause Will (of
> telepathy-haze fame) many headaches, though.

Agreed.

[...]
> CreateAccount / CreateAccountFromPresets
> Before we resort to libdbus naming conventions right away
> (dbus_connection_read_write_dispatch_and_also_make_me_a_sandwich)
> could we perhaps have CreateAccount(s: CM, s: protocol, s: preset, a{sv})
> where preset can be empty?

Yes, it definitely makes sense; in fact, that was the API that I was 
going to use internally.

> We could even consider making preset names only
> unique per (CM, protocol) pair - maybe install them like
> "/usr/share/telepathy/presets/gabble/jabber/gtalk.preset"?

Mmmm... While it makes some sense, I would prefer to have them all in 
the same directory: you don't lose any real benefit, you save some 
directory in the filesystem and the code to implement the reading is 
simpler.

> GetActualStatus
> I understand vaguely what this is - it's the status that Empathy
> displays. I'm not sure exactly what it's *for*, though (Maemo seems to
> get by fine without it).

We had a different API in Maemo, but the concept is there, indeed: it is 
basically used to signal whether we are connecting (blinking icon). 
Seems that also Pidgin has a similar logic, i.e. it shows a connecting 
icon when one account is in connecting state.

> RegisterChandler
> No. Wrong interface. Try again.

As I wrote in another e-mail, I would remove this API completely.

Looking forward for the next episode, :-)
   Alberto


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