[Telepathy] Review of MissionControl spec

[Xavier Claessens]

On lun, 2008-01-21 at 21:43 +0000, Simon McVittie wrote: 
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> OK, I've actually started on this now...
> 
> One thing I notice here is that we're conflating two (or more) things which
> are, in principle, completely separate.
> 
> * Account management. Telepathy components need to be able to keep track
>   of what accounts are available to us. This is the part that needs to
>   care about connections that aren't actually connected, connection
>   managers that aren't actually running, and so on.
> 
> * Dispatching channels. So we've got a channel up and running. What now?
>   Do we launch an Empathy chat window, or what?
> 
> (I note in passing here that the term "Chandler" must die. A chandler
> is a maker of candles, or a dealer in specialist (usually ship-related)
> equipment - nothing whatsoever to do with IM. If it survives in our
> API, we should capitalize it as CHandler, but I'd much prefer
> ChannelHandler. Let's keep the puns for project naming rather than
> public API please.)
> 
> 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.

I think filters and CH are almost the same conceptually, that's why I
merged both in MC spec. In NMC the difference is I can only register one
CH for a given type of channel, so we have filters to catch the channel
before the CH gets fired. Filters can only be registered at runtime so
without having to install a .chandler file. Filters can also tell CM to
not start the CH.

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

I agree, some things could be implemented as helper functions in tp-glib
or libempathy using CM API.


> The specification really needs to explain the distinctions (if any) between
> default, actual, global and current presence.

I agree that could be simplified and/or better explanation is spec.
- Default presence is the one MC set on offline accounts when it should
be put online.
- Actual presence is the presence our contacts sees
- Requested presence is the presence the user wants to have, but could
be != Actual presence because the requested presence could not be
supported by some protocols.
- Setting global presence request that presence to be set on all
accounts from all groups.


> 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.
> 
> Not coincidentally, I've been pondering the value of standard
> D-Bus Properties (possibly with extensions) for Telepathy in general - I think
> it could be extremely useful for reducing the amount of boilerplate in the
> spec, and using GetAll() would save us round-trips too.

Sure that really should be properties, but we first need a good dbus
properties support. With current system I think it's easier for clients
to have Get/Set/changed. I'm all for improved properties!

> Account
> =======
> 
> GetConnection() -> s, o
> Documented as returning (s, o) "if it's online". What happens if it's
> offline needs documenting too.
> The bus name and object path are theoretically redundant - you can derive
> either from the other, if we constrain the connection to be one that obeys the
> Telepathy spec - but it would save clients a round-trip to the bus
> daemon if we mentioned the *unique* bus name (e.g. :1.35) here.

Ok so it should return the object path or "" if offline?

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

It's easier for clients to connect one time to a signal that gets
emitted for all accounts. It could be done using 
StatusChanged from Connection iface but that means to connect many times
and disconnect/reconnect each time the accounts reconnects. of course if
dbus-glib is improved it's even better.

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

+1 for string handle. I still think we need something like
account.DispatchChannel(TpChannel). Like that the client can call
channel = connection.RequestChannel(supress_handler=true);
do_stuff(channel);
account.DispatchChannel(channel);

We could set supress_handler=false like that MC gets NewChannel but MC
will think it's an incoming channel even if it should be outgoing.

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

In fact Empathy updates each value separately directly when you write
it, so there is not a "Cancel" and "Ok" buttons but only "Close" as
everything already got applied. I did that because GNOME applications
generally prefer instant-apply and NMC API makes that easy. However with
"new" behaviour of NMC is to reconnect the account each time a param is
changed so I really should change that in Empathy UI.

Not sure what's the best...

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

If we go for SetParameters it's we can remove UnsetParameters I think.

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

+1 ! With recent gabble changes I had to update empathy UI to set
require-encryption, now if a user uses old gabble I have no idea how all
that will work... So empathy HEAD now "depends" on gabble >= 0.7.2 even
if empathy package do not have any hard dependency on CMs... that's a
bit a mess...


> Profiles are dead, long live presets. Much rejoicing.

The only real difference is presets are not required to create an
account.

> 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? 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"?

+1 for empty presets name. And +1 for unique names per (CM, protocol),
because haze/sofiasip could need different presets for the same SIP
service so both would like to have "ekiga.presets"... Same for the
"gtalk" profile with haze/gabble.

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

NMC has that too, it's useful but could be done using 
GetConnectionStatus on each account. I don't know if MC spec should
include "helper" methods or if we should let that to tp-glib or
libempathy.

> RegisterChandler
> No. Wrong interface. Try again.

You mean that should be moved to another interface or that shouldn't
exist at all?

> ChannelHandler
> ==============
> 
> The patch "New Chandler system" completely rewrites the API. It's not
> clear to me which one is better or why, and the patch naming doesn't
> give me much help.
> 
> I think we need to get a clear picture of what's going on with Tubes and
> File Transfer before we go much further with this.

That's the hard part, surely Tubes and FT is what makes it difficult and
why NMC's system can't be used.

Thanks for your work !

Xavier Claessens.