[Bug 24061] TpAccount, TpAccountManager: add convenience API similar to libempathy's

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=24061





--- Comment #14 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-09-24 11:26:36 PST ---
In telepathy-qt4, the core feature is implicit (becomeReady never succeeds
until FeatureCore is ready, regardless of what features you asked for). I think
we should probably do the same here, so it's enough to call
tp_account_prepare_async (., NULL, ., .) to get basic functionality.

A bit more review of signals/properties/docs/etc., although I haven't reviewed
the implementation:

> 991       g_param_spec_uint ("presence",
> 992           "Presence",
> 993           "The account connections presence type",

As with get_current_presence, please explicitly say which presence (this
appears to be the current presence). I think adding "current" to make the
distinction between requested and current presence clear is worth the extra
typing (for how not to do it, see MC 4's Presence and PresenceActual).

I think it's worth calling accessors/properties whose type is
TpConnectionPresenceType "foo-presence-type", and reserving "foo-presence" for
things that (can) get the whole SimplePresence struct. You might disagree,
though.

The API for current and requested presence is currently inconsistent: one has a
single combined method and one has three methods. Please pick one or the other
(I don't really mind which).

> 1881  * tp_account_update_parameters_finish:

This should be able to output reconnect_required somehow. I don't know how you
normally do that in GIO.

> 1285    * TpAccount::removed:

Removal is already represented by invalidating the object with
TP_DBUS_ERROR_OBJECT_REMOVED, as documented in the TpAccount introduction. Do
we need a removed signal as well? (telepathy-glib users are basically going to
have to deal with the idea that objects can be invalidated, regardless - in
telepathy-qt4 we represent account removal as invalidation with a particular
error)

> 1249    * TpAccount::status-changed:

It might be worth adding a string (currently unused, eventually D-Bus error
name) and a hash table (currently unused, eventually "details"), to make the C
API less nasty when we expose ConnectionError(s, a{sv}) on the Account as well
as the Connection.

----

Documentation things:

It's worth documenting which properties the notify signal works on (or perhaps
documenting, once, that the notify signal works on all properties, if that
turns out to be true).

Each property/getter pair should be cross-referenced in at least one direction
(I usually find it looks nicer to document the property properly, and document
the getter as "Returns: the value of the TpAccount::teapot property").

Each property, or each getter, or both should document which feature needs to
be enabled: perhaps something like

    This is not guaranteed to have been retrieved until
tp_account_prepare_async has finished; until then, the value is ""

for core, and

    This is not guaranteed to have been retrieved until
tp_account_prepare_async has finished for the feature
TP_ACCOUNT_FEATURE_TEAPOTS; until then, the value is TP_TEAPOT_NONE

in future for non-core?

(This is one advantage that the "ready" terminology had - you could say "until
FeatureTeapots is ready" and it didn't sound awkward)

> 2295  * Gets the value of the Nickname parameter on @account.

Nitpicking: please call properties properties, not parameters, here and
elsewhere. Only the Parameters property contains parameters.

> 165  * TP_ACCOUNT_FEATURE_CORE:

This should explain briefly what preparing this feature means: in this case,
something like "the basic properties of the Account have been retrieved and are
available for use, and change-notification has been set up".

> 174  * tp_account_get_feature_quark_core:

I don't think this should be documented, assuming people are meant to access it
through the TP_ACCOUNT_FEATURE_CORE macro. It can go in a <SUBSECTION Private>
if that's the case. (See also: the GType macros.)

> 1419  * Set the connection of the account by specifying the connection object path.
> 1420  * This function does not return a new ref and it is not guaranteed that the
> 1421  * returned #TpConnection object is ready.

It would be good if the docstring explained why you'd ever want to call this
method... presumably it's intended to be used when you already know the object
path, from a HandleChannels call or something?

> 1218    * TpAccount:nickname
> 1220    * The account's nickname.

This sounds easy to confuse with DisplayName. "The nickname that should be set
for the user on this account" would be less ambiguous.

> 1082           "DisplayName",
> 1083           "The accounts display name",

Nitpicking: apostrophes are recommended :-) There are various other instances
of what is presumably en_BE from Empathy in the property descriptions.

----

Bits of implementation that I noticed on the way past:

All the getters should ideally have a g_return_val_if_fail (TP_IS_ACCOUNT
(self), .)) guard, although I don't think that's a merge blocker.


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.