[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


Sjoerd Simons <sjoerd at luon.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sjoerd at luon.net




--- Comment #3 from Sjoerd Simons <sjoerd at luon.net>  2009-09-23 04:13:08 PST ---
(In reply to comment #1)
> API review:

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

If you don't know why we'd need it, please ask next time as this was added to
empathy for a reason :)..

Basically the issue (as discussed at various points) is that we can't know from
the AM what the user last set as a presence. This is particularily annoying
when creating a new account, as we don't know what its initial status should
be. This function allows one to at least have a proper default. Ofcourse it
only works as long as the presence setter and the account UI are the same
process, which hopefully will stop being so during the G2.29 series.

I guess we should clone this bug and reassign it to the AM, to get some sort of
representation of this, at which point we can properly add this API

> > 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 pondered a bit more about the become_ready and i must say (assuming i'm
allowed to bikeshed a bit) i prefer prepare. As it the operation you call means
prepare these features on this object, like g_output_stream_write means write
these bytes to the output stream. become ready these features just doens't work 

But again, i do like my bikesheds :)

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

This doesn't make sense to me, is there any reason you ever want to know this ?
Either you want a feature to be available or not, i don't see how the knowledge
that something is preparing itself ever helps the user.

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

tbh, the fact that the async operation has finished is slightly redundant here.
A feature should be in that set as soon as it's known it can't work (e.g. there
is no avatar interface). For some features you might not know until you've
tried for the first time ofcourse, but that's an implementation detail the user
doesn't have to care about.

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

smcv just clarified that this is basically used in Qt land to assert on. I'm
not sure how useful this actually is.

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

In empathy it should stay the way it is, no reason for code churn, and nobody
will ever thing it means ``only connected'', i don't even known what that would
mean :)

> > TpConnection *tp_account_get_connection (TpAccount *account);
> 
> I think this should be allowed to return unready connections.
> 
> The fact that the "connection-ready" state in TpConnection implies CONNECTED is
> unfortunate from a future-tp-glib point of view, and in future (when we have
> the "interactive SASL authentication" D-Bus API) accounts' unconnected
> connections will probably be rather more interesting.

Hrm, didn't know that the ready state was that heavy in tp-glib. It is quite
convenient inside empathy to know that if you have a connection for an account
that connection is ready (saves us from doing call_when_ready all over the
place), but we can easily wrap this.

> > const gchar *tp_account_get_connection_manager (TpAccount *account);
> 
> telepathy-qt4 also has FeatureProtocolInfo; we should clone this bug as
> "implement TpAccount FeatureProtocolInfo, which gets its protocol info from a
> hidden TpConnectionManager" or something, as future work.

What extra information does FeatureProtocolInfo have that's not in
TpConnectionManager ?

> > TpConnectionStatus tp_account_get_connection_status (TpAccount *account);
> 
> Perhaps this should be TpConnectionStatus get_connection_status (TpA *, TpCSR
> *maybe_reason) like in TpConnection?
> 
> > 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).
> 
> 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.

Either way would be fine by me, it's splitup in empathy for hystorical raisins,
sometimes it's convenient to not need to have temporary variables, but in
practice it doesn't matter that much. Addition of _current_ is a good point
indeed

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

So does empathy, refresh_properties is basically private api for the Account
managers benefit, so that it can poke still existing accounts after a AM crash
to ensure they're still in sync. If we allow the TpAccount to be created
manually this is less benefitial indeed, althought it's slightly annoying that
all TpAccount objects need to watch the AM, verify they still exist etc etc. 



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