[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 #8 from Jonny Lamb <jonny.lamb at collabora.co.uk>  2009-09-24 10:04:09 PST ---
My reply to the first comment:

(In reply to comment #1)
> > #define TP_ACCOUNT_MANAGER_FEATURE_CORE \
> >   g_quark_from_static_string ("tp-account-manager-feature-core")
> 
> I'd have made this expand to a function call so the linker has already done the
> hash table lookup, but, whatever. Same for Account.

Done.

> > TpAccount *tp_account_manager_get_account_for_connection (
> >     TpAccountManager *manager, TpConnection *connection);
> 
> Drop this, it can only work if all accounts are guaranteed to be cached, and if
> we really need it we can add it later.

Dropped.

> > TpAccount *tp_account_manager_get_account (TpAccountManager *manager,
> >     const gchar *path);
> 
> General point: if it's not guaranteed to be ready, the docs should say so.
> (It's not, afaics.)

Done.

> I don't think get_account makes a great deal of sense, we should just have
> ensure_account?
> 
> ensure_account should create an object even if the TpAM doesn't think the
> account exists, to avoid races. The docs should probably say it does.

Done.

> > void tp_account_manager_request_global_presence (TpAccountManager *manager,
> >     TpConnectionPresenceType type, const gchar *status, const gchar *message);
> 
> I'd prefer this called something like request_all_presences() or
> set_all_requested_presences().

Renamed.

> > TpConnectionPresenceType tp_account_manager_get_requested_global_presence (
> >     TpAccountManager *manager, gchar **status, gchar **message);
> 
> Can this be removed? I'm not sure why we need it.

I removed it, and added a new property on TpAccount called default-presence. I
guess I added it to the wrong object given the following discussion since I
implemented it.

> > TpConnectionPresenceType tp_account_manager_get_global_presence (
> >     TpAccountManager *manager, gchar **status, gchar **message);
> 
> get_most_available_presence() please.

Renamed.

> Document what happens if no accounts are enabled and valid (should be OFFLINE,
> "offline", "").

Done.

> Document the invariant that there exists at least one account with this
> presence (this is never synthesized from multiple accounts' presences).

Done.

> > void tp_account_manager_prepare_async (TpAccountManager *account,
> >     GQuark* features, GAsyncReadyCallback callback, gpointer user_data);
> > 
> > gboolean tp_account_manager_prepare_finish (TpAccountManager *account,
> >     GAsyncResult *result, GError **error);
> 
> become_ready_async()? Also in Account.

I renamed this, then noticed you'd changed your mind so renamed it back.

> > gboolean tp_account_manager_set_features (TpAccountManager *account,
> >     const GQuark* features);
> 
> Remove this, and make become_ready_async() callable with a NULL callback to get
> the same result. Also in Account.

Done.

> > const GQuark * tp_account_manager_get_features (TpAccountManager *account);
> 
> This should have the same semantics as tpqt4's ReadyObject, which has these
> accessors:
> 
> * requestedFeatures() - features that the async operation started, and may or
> may not have finished
> * actualFeatures() - features that the async operation finished successfully
> and the feature does really work (e.g. Connection has Avatars)
> * missingFeatures() - features that the async operation finished, but the
> feature can't work (e.g. Connection doesn't have Avatars)

Added.

> and isReady() means that the proxy was not invalidated yet, and the feature is
> in either the actual set or the missing set.

Done.

> > gboolean tp_account_is_just_connected (TpAccount *account);
> 
> No, no, and furthermore, no. (Also, the corresponding function in Empathy
> should be renamed to connected_recently() to avoid the ambiguity of whether
> "just" means "recently" or "only".)

Removed.

> > TpConnection *tp_account_get_connection (TpAccount *account);
> 
> I think this should be allowed to return unready connections.

I cherry-picked Danni's commit. Thanks!

> > TpConnectionStatus tp_account_get_connection_status (TpAccount *account);
> 
> Perhaps this should be TpConnectionStatus get_connection_status (TpA *, TpCSR
> *maybe_reason) like in TpConnection?

Done.

> > TpConnectionPresenceType tp_account_get_presence (TpAccount *account);
> > const gchar *tp_account_get_status (TpAccount *account);
> > const gchar *tp_account_get_status_message (TpAccount *account);
> 
> tp_account_get_current_presence(), please. It's very deliberate that Account
> doesn't have a Presence member (MC 4's "presence" and "presence_actual" were
> horrible).

Done.

> Perhaps this should return a presence type, with status/message optionally
> returned through pointers too? tp_account_get_current_presence_type() could
> return the enum member if you think that's valuable.

Done.

> > void tp_account_refresh_properties (TpAccount *account);
> 
> Should never be needed... if the AM crashes, TpAccount should recover
> automatically when it notices the AM name come back, updating any accounts that
> are still there and invalidating any accounts that turn out to have disappeared
> (telepathy-qt4 implements this).

Removed.

> > const gchar *tp_account_get_unique_name (TpAccount *account);
> 
> Remove this. tp_proxy_get_object_path() suffices, and "unique name" is D-Bus
> jargon already, with a different meaning.

Removed.


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