[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 #4 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-09-23 04:41:08 PST ---
I think the general message is "if in doubt, omit it from public API for now"
:-)

(In reply to comment #3)
> (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 :)..

Note the phrasing as a question :-)

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

Since this function has the limitation you note, I'd like to leave this as some
sort of global in Empathy rather than have it in the TpAM API, and when we have
a proper way to do it over D-Bus (which I'd prefer phrased as "the default
presence for new accounts" rather than "the global presence"), we can add
*that* to the TpAM API.

> 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

Yes, AccountManager.DefaultPresence or some such would be a good addition. I'll
do that.

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

If that's the colour you want your bike shed, then I think we should rename the
entire "ready" concept to "prepared", which does make a certain amount of
sense. This would also make it less confusing when we deprecate
TpConnection::connection-ready.

Jonny: what do you think of this idea?

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

Perhaps that should be internal (or proxy-subclass.h), yeah. The distinction
between the three sets is important, though, even if not all of them are public
API - if an API works with readiness (or "preparedness") it should document
exactly what it does/needs.

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

That's a good point, yes.

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

Could we leave it out, or make it library-internal (_tp_foo namespace and
account{,-manager}-internal.h), until we decide whether it's useful?

> > > TpConnection *tp_account_get_connection (TpAccount *account);
> > 
> > I think this should be allowed to return unready connections.
> 
> 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.

Yes, please wrap it inside Empathy. If we later decide that this is useful,
*then* we can add it to tp-glib's API.

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

Nothing - it's a short-cut to get a ProtocolInfo object (which describes one of
a CM's protocols, including parameters). The idea is that you can either get a
ProtocolInfo by making a Tp::ConnectionManager and asking it about the
appropriate protocol, or (as syntactic sugar) by asking the Tp::Account about
its protocol.

Not a merge blocker at all, just a suggestion for future work.

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

Can we put this in the internal namespace too, then? If it's only for the AM's
benefit then it shouldn't be in the tp-glib ABI (and if in doubt, it shouldn't
be in the ABI *yet*).


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