[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 #29 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-09-26 11:25:46 PST ---
Implementation bug noticed in passing:
_tp_account_manager_account_invalidated_cb assumes that an account will never
be invalidated with a domain other than TP_DBUS_ERRORS. That's not (meant to
be) true.

One thing I'm not very happy about (which I hadn't previously spotted because I
wasn't reviewing the implementation!) is that the AccountManager seems to treat
the list of accounts and the list of valid accounts as synonymous.

I recognise that invalid accounts are not very useful (account management UIs
that might be able to rescue them are the only use-case that springs to mind),
but I think pretending they don't exist at all is harmful (after all, we
introduced the concept of validity so that MC wouldn't silently delete invalid
accounts, so it doesn't seem right to have our reference client API behave as
if they had been deleted).

We don't need to introduce API for invalid accounts yet, but we do need to
avoid ruling out that API later. (I think a property that's just a list of
object paths would be fine, for instance.)

My suggestions would be:

* _tp_account_manager_ensure_all_accounts: when recovering from AM
crash/recovery, do not consider newly-invalid accounts to have been deleted -
that's clearly untrue! (I note that you've made
_tp_account_manager_account_invalidated_cb do the right thing, so this bug only
exists during crash-recovery...)

* tp_account_manager_get_accounts: document that it only returns valid
accounts, and ideally call it get_valid_accounts

I'm suspicious about the fact that account-created is only emitted if the
account is valid, and only when it is ready. At the very least, it should be
documented that this is not guaranteed to be emitted for invalid accounts (or
that it is guaranteed not to be emitted for invalid accounts, if you want to
stick to that interpretation), and that it is only emitted when the core
feature of the account has been prepared.

(Actually, I'm also suspicious about the fact that the C API is deviating from
the D-Bus API for no particularly compelling reason...)

------------

Lessons that I think we should learn from this process:

* Reviewing the API without also reviewing the implementation doesn't really
work, unless there is very clear documentation (I kept missing API quirks that
are only apparent from the implementation)

* Next time we pull in code from Empathy or some similar source, it should land
gradually, rather than as a major code drop that then needs a series of
incremental changes


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