Bug 24061

API review:

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

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

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

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.

(TpQt4 equivalent of ensure_account is accountForPath)

> GList *tp_account_manager_get_accounts (TpAccountManager *manager);

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

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

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

get_most_available_presence() please.

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

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

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

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

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

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

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

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

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

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

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

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

I would not necessarily object to a method tp_account_get_object_path_tail() or tp_account_get_identifier() or something, which returned the part of the object path after the common prefix (this is what MC internally, and inappropriately, calls the account's "unique name").

I've responded to half of your comments, up to the comment:

> This should have the same semantics as tpqt4's ReadyObject,
> which has these accessors:

The rest of the comments I've not yet seen to, but are mostly "remove or rename this function".

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

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

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

This is now Bug #24104.

We should always return the TpConnection, even when it's not ready.

http://git.collabora.co.uk/?p=user/danni/telepathy-glib.git;a=commitdiff;h=45eb1bc71b818cda7a9f05fc3728919b24f75808

Where we ask for a list of features as a 'Quark *features', should that be 'const Quark *features' ?

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.

(In reply to comment #4)
> Yes, AccountManager.DefaultPresence or some such would be a good addition. I'll
> do that.

I've added a #TpAccount:default-presence property. I guess you'd prefer it on TpAccountManager instead then?

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

I renamed it from prepare, to become_ready, and back to prepare. You've hit your quota for changing your mind, boys.

> > 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.
[snip]
> 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?

Hmm, okay, so I'll make these internal?

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

Hmm, does that mean that http://git.collabora.co.uk/?p=user/jonny/telepathy-glib.git;a=commitdiff;h=fce7efce420e is in error?

(In reply to comment #6)
> We should always return the TpConnection, even when it's not ready.
> 
> http://git.collabora.co.uk/?p=user/danni/telepathy-glib.git;a=commitdiff;h=45eb1bc71b818cda7a9f05fc3728919b24f75808

I cherry-picked your commit, thanks!

(In reply to comment #7)
> Where we ask for a list of features as a 'Quark *features', should that be
> 'const Quark *features' ?

Fixed, thanks!

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

Perhaps you misunderstood: I don't think there should be *any* API for this in telepathy-glib, until there is a corresponding D-Bus API that it can poke (which would be on AccountManager). At the moment its status would be "this is a trap, it only works if your account UI and presence UI are in the same process and share a TpAccountManager", which doesn't seem all that useful in the long term.

(Without looking at your implementation I don't even know what a default-presence property on TpAccount would do...)

(In reply to comment #11)
> > I removed it, and added a new property on TpAccount called default-presence.
>
> (Without looking at your implementation I don't even know what a
> default-presence property on TpAccount would do...)

Having looked at the implementation I'm pretty sure it's not useful...

> "the default presence that should be set on the account when it becomes enabled"

... since this is just RequestedPresence in disguise, but without syncing up over D-Bus. (MC will allow RequestedPresence to be set on disabled accounts, and will honour RequestedPresence when an account is Enabled.)

The feature that I think Sjoerd wants is Bug #24104, which needs extra D-Bus API to do correctly, and isn't adequately covered by this default-presence property anyway.

Until Bug #24104 is fixed, Empathy can emulate tp_account_manager_get_requested_global_presence by having a global variable (or something equivalent, like a member of a singleton) in which it stores its idea of the global presence, and using that global variable as the presence to request on newly created accounts. That doesn't work unless the account and presence UIs are in the same process, but neither does the API you had initially, so it's not a regression.

(In reply to comment #12)
> The feature that I think Sjoerd wants is Bug #24104, which needs extra D-Bus
> API to do correctly, and isn't adequately covered by this default-presence
> property anyway.

I see. I've removed it.

> Until Bug #24104 is fixed, Empathy can emulate
> tp_account_manager_get_requested_global_presence by having a global variable
> (or something equivalent, like a member of a singleton) in which it stores its
> idea of the global presence, and using that global variable as the presence to
> request on newly created accounts. That doesn't work unless the account and
> presence UIs are in the same process, but neither does the API you had
> initially, so it's not a regression.

Right, okay.

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.

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

Done.

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

Sounds reasonable, done.

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

Done.

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

Yeah, done.

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

I guess not, removed.

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

Done.

Xavier made some comments online regarding my branch too:

06:24 < Zdra> jonnylamb: _tp_account_connection_ready_cb() should not call g_object_notify (G_OBJECT (account), 
              "connection"); because you don't return only ready TpConnection anymore
06:35 < Zdra> jonnylamb: note that TpAccount retuning non-ready TpConnection has implications in empathy
06:35 < Zdra> EmpathyAccount was returning NULL if the connection was not ready
06:35 < Zdra> and all the empathy code always assume objects are ready
06:36 < Zdra> jonnylamb: for example there are places when it gets the connection from an account in the 
              EmpathyAccountChooser
06:36 < Zdra> jonnylamb: then make some channel request with that connection without checking if it was ready
06:37 < Zdra> basically some if(connection!=NULL) should be replaced by if(connection!=NULL&&tp_connection_is_ready())
06:49 < Zdra> jonnylamb, What's the rational for calling _tp_account_become_ready in the middle of _tp_account_update 
              instead of the end?

(In reply to comment #16)
> Xavier made some comments online regarding my branch too:
> 
> 06:24 < Zdra> jonnylamb: _tp_account_connection_ready_cb() should not call
> g_object_notify (G_OBJECT (account), 
>               "connection"); because you don't return only ready TpConnection
> anymore
> 06:35 < Zdra> jonnylamb: note that TpAccount retuning non-ready TpConnection
> has implications in empathy

I think Empathy should have a wrapper function that only returns ready TpConnections, then (this can be added while porting).

TpConnection *
empathy_account_get_ready_connection (...)
{
  TpConnection *c = tp_account_get_connection (...);

  if (! tp_connection_is_ready (c))
    c = NULL;

  return c;
}

If we need it badly enough, we can add tp_account_get_ready_connection later, or even add that extra guarantee. However, making tp_account_get_connection *not* guarantee to produce a ready connection is not a safe change to make later.

Aargh, TpAM also has signals I hadn't looked at...

+  /**
+   * TpAccountManager::account-changed:
+   * @manager: a #TpAccountManager
+   * @account: a #TpAccount
+   *
+   * Emitted when an account @manager is changed.

What?! What does that even mean?

It seems to never be emitted, anyway. Drop it?

+  /**
+   * TpAccountManager::account-connection-changed:

Is this guaranteed by FeatureCore to be emitted? If so, are you sure it should be an API guarantee that it is?

(If you're not sure, the shortest path to fewer API guarantees is to #if 0 it, and consider adding it later)

+  /**
+   * TpAccountManager::new-connection

Likewise, but more expensive to emit (it's creating a TpConnection with no guarantee that anyone cares about it - the sugar-presence-service anti-pattern).

+  /**
+   * TpAccountManager::global-presence-changed:

This should have been renamed when tp_account_manager_get_global_presence was.

Further to TpAccountManager::new-connection: it appears that TpAccount always creates a Connection whenever it has a non-empty object path.

I think that's a bad idea, because of the "sugar-presence-service anti-pattern", by which I mean:

* client binding for container (Sugar's PresenceService, originally)
* objects in the container (Activity and Buddy, originally) that "most" clients of the container don't care about "most" instances of
* when a new object appears, D-Bus traffic is needed to set up a proxy for it
* the proxy is emitted in a signal
* nobody actually cares, so the signal is ignored
* the proxy is unreffed and destroyed, and the D-Bus traffic was a waste of time
* for bonus points (as seen in Sugar), many applications have a reason to watch the container, so you get a thundering herd of applications all doing unnecessary work simultaneously

It sounds silly, but this was a practical performance problem on OLPC/Sugar - worse, we couldn't even fix it, because it was an API guarantee.

For an easy example of something that watches an Account but doesn't care about its Connection, consider an account setup UI. A more subtle example is a handler for channels, which only cares about the Connection for each Channel that it's handling.

So, I don't think _tp_account_set_connection should do what it currently does (set up a TpConnection and watch for it to be ready or invalidated). The Account D-Bus API was specifically designed so that this wouldn't be necessary for basic use of an account: when the connection drops, you get something like AccountPropertiesChanged({Connection: "/", ConnectionStatus: DISCONNECTED, ConnectionStatusReason: OUT_OF_CHEESE}). (This is also why the Account proxies the current presence through itself.)

Currently, they might arrive in different signals because MC's internals are weird (I should probably file a bug about that). The safest thing to do would be to just remember the reason, and use it when connection status DISCONNECTED or Connection "/" arrives (connection statuses CONNECTING and CONNECTED are always for reason REQUESTED in practice).

For those clients that *do* care about the TpConnection, tp_account_get_connection could create one lazily (if the object path is not "/"), or the guarantee to have a TpConnection (or a ready TpConnection, if you prefer) could be a Feature.

In the short term, tp_account_ensure_connection seems sufficient.

Similarly, we should consider removing TpAccount's connection property, temporarily or permanently.

Something I forgot to mention about the Connection stuff is that if we're making the TpConnection not an API guarantee, it'll need a property + getter pair for the Connection object path (which wouldn't be a bad idea anyway).

I'd be inclined to make the C API for that either return NULL or a real object path (as opposed to the raw D-Bus API, which is either "/" or a real object path).

(In reply to comment #9)
> Hmm, okay, so I'll make these internal?

get_{requested,actual,missing}_features still seem to be extern on both Account and AM. Indeed, I don't see an account-internal.h, which would be a prerequisite for making things internal - presumably you forgot to implement (or perhaps push) this?

*_is_ready (should be _is_prepared now, surely?) are also reasonable candidates for this treatment, if they're not immediately needed.

Having expressed concern about the "kick MC 5 if it crashes" functionality landing in TpAccountManager, I now realise that a reasonable solution would be API something like:

    am = tp_account_manager_new (...);
    tp_account_manager_enable_restart (am);

If we later decide that it's desirable to have in every application, we can make this functionality always active, and make the opt-in call a no-op; if we decide it was a bad idea, we can deprecate it (and if it turns out to have been really bad, again make the call a no-op).

I personally think that when Empathy gets split up into components, the presence-setting applet should kick MC if it crashes, but the other bits shouldn't.

Sjoerd, do you think this is a reasonable compromise between making the restart behaviour explicit, and copy/pasting between all the implementations?

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

Done.

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

Done.

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

Done.

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

Ah yes, I frequently got confused between parameters and properties.

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

Done.

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

Ah okay, done.

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

Done.

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

Done.

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

Done.

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

Done.

(In reply to comment #16)
> 06:24 < Zdra> jonnylamb: _tp_account_connection_ready_cb() should not call
> g_object_notify (G_OBJECT (account), 
>               "connection"); because you don't return only ready TpConnection
> anymore

Removed.

> 06:49 < Zdra> jonnylamb, What's the rational for calling
> _tp_account_become_ready in the middle of _tp_account_update 
>               instead of the end?

None; moved.

(In reply to comment #18)
> +  /**
> +   * TpAccountManager::account-connection-changed:

> +  /**
> +   * TpAccountManager::new-connection

Both of these can be dropped, it's basically empathy legacy as empathy internally still is connection based instead of account based in a lot of places.. More refactoring is needed to change that around :)

(In reply to comment #18)
> +  /**
> +   * TpAccountManager::account-changed:
> +   * @manager: a #TpAccountManager
> +   * @account: a #TpAccount
> +   *
> +   * Emitted when an account @manager is changed.
> 
> What?! What does that even mean?
> 
> It seems to never be emitted, anyway. Drop it?

Dropped.

> +  /**
> +   * TpAccountManager::account-connection-changed:
> 
> Is this guaranteed by FeatureCore to be emitted? If so, are you sure it should
> be an API guarantee that it is?

Removed.

> +  /**
> +   * TpAccountManager::new-connection
> 
> Likewise, but more expensive to emit (it's creating a TpConnection with no
> guarantee that anyone cares about it - the sugar-presence-service
> anti-pattern).

Removed.

> +  /**
> +   * TpAccountManager::global-presence-changed:
> 
> This should have been renamed when tp_account_manager_get_global_presence was.

Renamed.

(In reply to comment #19)
> So, I don't think _tp_account_set_connection should do what it currently does
> (set up a TpConnection and watch for it to be ready or invalidated). The
> Account D-Bus API was specifically designed so that this wouldn't be necessary
> for basic use of an account: when the connection drops, you get something like
> AccountPropertiesChanged({Connection: "/", ConnectionStatus: DISCONNECTED,
> ConnectionStatusReason: OUT_OF_CHEESE}). (This is also why the Account proxies
> the current presence through itself.)

Okay, I made _set_connection stop listening to invalidated and listen for a "/" connection object path.

> For those clients that *do* care about the TpConnection,
> tp_account_get_connection could create one lazily (if the object path is not
> "/"), or the guarantee to have a TpConnection (or a ready TpConnection, if you
> prefer) could be a Feature.

I have made _get_connection lazily create a TpConnection when called.

> In the short term, tp_account_ensure_connection seems sufficient.

Not if we don't have the connection object path, which is why I kept _get_connection.

> Similarly, we should consider removing TpAccount's connection property,
> temporarily or permanently.

Why remove it? If it is defined as having the same guarantees as the connection getter which is staying around, there is no point removing it, or do you want the option to have a :connection property later on with different semantics from _get_connection?

(In reply to comment #20)
> Something I forgot to mention about the Connection stuff is that if we're
> making the TpConnection not an API guarantee, it'll need a property + getter
> pair for the Connection object path (which wouldn't be a bad idea anyway).
> 
> I'd be inclined to make the C API for that either return NULL or a real object
> path (as opposed to the raw D-Bus API, which is either "/" or a real object
> path).

At the time of writing, I've made _get_connection return a TpConnection. Sounds good. As a result, this isn't particularly important right now, but would be very trivial to add. Shall I add it now or just wait?

> get_{requested,actual,missing}_features still seem to be extern on both Account
> and AM. Indeed, I don't see an account-internal.h, which would be a
> prerequisite for making things internal - presumably you forgot to implement
> (or perhaps push) this?

I was just waiting a while to make sure people weren't going to change their mind on this one. I've internalized them now.

> *_is_ready (should be _is_prepared now, surely?) are also reasonable candidates
> for this treatment, if they're not immediately needed.

I think _is_prepared (now renamed from _is_ready) is useful to have around. Although one could easily call _prepare_async, it's sometimes really annoying to have to make all your code asynchronous and you simply might not want to do anything if a certain feature is not prepared. As Sjoerd pointed out, comment #17 gives a use-case for _is_prepared.

(In reply to comment #21)
> Having expressed concern about the "kick MC 5 if it crashes" functionality
> landing in TpAccountManager, I now realise that a reasonable solution would be
> API something like:
> 
>     am = tp_account_manager_new (...);
>     tp_account_manager_enable_restart (am);
> 
> If we later decide that it's desirable to have in every application, we can
> make this functionality always active, and make the opt-in call a no-op; if we
> decide it was a bad idea, we can deprecate it (and if it turns out to have been
> really bad, again make the call a no-op).
> 
> I personally think that when Empathy gets split up into components, the
> presence-setting applet should kick MC if it crashes, but the other bits
> shouldn't.

I implemented this quickly, but Sjoerd said that we were going to stick with the current way and transition to activating the AM somehow in the future.

So, I've currently replied to and/or fixed all comments regarding this branch.

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

(In reply to comment #26)
> > Similarly, we should consider removing TpAccount's connection property,
> > temporarily or permanently.
> 
> Why remove it? If it is defined as having the same guarantees as the connection
> getter which is staying around, there is no point removing it, or do you want
> the option to have a :connection property later on with different semantics
> from _get_connection?

Partly just that it seems weird: the idea of a property whose value is lazily allocated seems stranger to me than that of a getter whose result is lazily allocated.

I'm a bit concerned about bindings' behaviour: are there bindings that eagerly retrieve all properties in order to populate Python-or-whatever wrappers' attributes? If there are, then they'll suffer from the TpConnection being created unnecessarily.

(In reply to comment #28)
> (In reply to comment #21)
> > Having expressed concern about the "kick MC 5 if it crashes" functionality
> > landing in TpAccountManager, I now realise that a reasonable solution would be
> > API something like:
> 
> I implemented this quickly, but Sjoerd said that we were going to stick with
> the current way and transition to activating the AM somehow in the future.

Sjoerd, what do you think about my proposal in Comment #21 as a compromise for this?

> So, I've currently replied to and/or fixed all comments regarding this branch.

OK, I think you've addressed everything except:

* MC restart from Comment #21 (still under discussion)
* everything from Comment #29 (new)
* removal of the TpAccount:connection property (on which I'm prepared to be overruled if you and Sjoerd both think having a lazily-allocated property value isn't weird)

(In reply to comment #31)
> * MC restart from Comment #21 (still under discussion)

Removing this behaviour or making it opt-in later would not *technically* be an API break, I suppose, so we could release with it if it's the only blocker.

> * everything from Comment #29 (new)

I do think these need addressing and are merge blockers.

> * removal of the TpAccount:connection property (on which I'm prepared to be
> overruled if you and Sjoerd both think having a lazily-allocated property value
> isn't weird)

Like I said, this is more "this is weird and may be unnecessary" rather than "this is harmful".

(In reply to comment #32)
> (In reply to comment #31)
> > * MC restart from Comment #21 (still under discussion)
> 
> Removing this behaviour or making it opt-in later would not *technically* be an
> API break, I suppose, so we could release with it if it's the only blocker.

I'm happy to have a tp_account_manager_enable_restart for now, if we change our minds making it a dummy and decrecating it won't hurt anyway.

TBH i'm more concerned about the channel dispatcher then the Am, it just happens that both are the same currently

> > > * everything from Comment #29 (new)
>
> I do think these need addressing and are merge blockers.

To summarize:
  * instead of a created and deleted signal have:
     * A account-removed signal matching the one on D-Bus
     * an account-validity-changed signal matching the one on D-Bus as well
  * fix the implementation issue that smcv noticed
  * rename get_accounts to _get_valid_accounts
  * related to that, remove accounts from the when they are no longer valid or filter them out in the  
    _get_valid_accounts call (latter is probably more future-proof)

> > * removal of the TpAccount:connection property (on which I'm prepared to be
> > overruled if you and Sjoerd both think having a lazily-allocated property value
> > isn't weird)
>
> Like I said, this is more "this is weird and may be unnecessary" rather than
> "this is harmful".

From my perspective, it being lazily allocated is an implementation details of the TpAccount
implementation, not something the API user would care about. 

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

Fixed.

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

Fixed.

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

Done.

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

account-created has been renamed to account-validity-changed (with arguments TpAccount, gboolean).

(In reply to comment #31)
> * MC restart from Comment #21 (still under discussion)

I added tp_account_manager_enable_restart.

> * everything from Comment #29 (new)

This should all be fixed (see comment #34)

> * removal of the TpAccount:connection property (on which I'm prepared to be
> overruled if you and Sjoerd both think having a lazily-allocated property value
> isn't weird)

Sjoerd thinks this isn't worth it, as he says in comment #33.

(In reply to comment #33)
> I'm happy to have a tp_account_manager_enable_restart for now, if we change our
> minds making it a dummy and decrecating it won't hurt anyway.

Done.

>   * instead of a created and deleted signal have:
>      * A account-removed signal matching the one on D-Bus

Done.

>      * an account-validity-changed signal matching the one on D-Bus as well

Done.

>   * fix the implementation issue that smcv noticed

Done.

>   * rename get_accounts to _get_valid_accounts

Done.

>   * related to that, remove accounts from the when they are no longer valid or
> filter them out in the  
>     _get_valid_accounts call (latter is probably more future-proof)

I did the former.

> From my perspective, it being lazily allocated is an implementation details of
> the TpAccount
> implementation, not something the API user would care about. 

Left this property in for now then.

For completeness, me and smcv pondered about account-{enabled}, account-{disabled} the other night, after talking to jonny they should stay in for the following reasons:
  * It's not complicated to implement
  * They're very simple semantically and don't add any policy
  * They're very convenient, both empathy and moblin uses it. For a lot of use-cases you only 
     care about enabled accounts and ignore the others this is a nice way to have change 
     notification for them.

In summary it doesn't cost us anything and is a win for API users :)

kthxmerged