Bug 29614

Additionally, TpAccountChannelRequest doesn't know a TpAccountManager, but does know a TpAccount, and would benefit from the ability to feed that "likely account" to the TpBaseClient it uses internally. I've added a patch with internal-only API to do this.

I haven't made this into public API, since it seems pretty niche; if you want precise control over TpAccount re-use, you should probably have a TpAccountManager.

You probably want to check that the bus is the one the AccountManager object given to TpBaseClient is on if both are specified.

Also, in likely-account: in which cases exactly can the account not be the likely account? (The filter is empty, so MC shouldn't dispatch anything to us apart from our request which will indeed have the same account). Somebody guessing our service name and setting it as a preferred handler?

IF there are valid corner-cases, in which this can happen, then shouldn't we actually pass the account's AccountManager, if any, to the TpBaseClient rather than just the bus, to get correctly shared accounts outside the singleton AM instance even if it's not the likely account?

OTOH if there actually aren't, why not just error the request in that case, since it probably indicates MC dispatching an incorrect channel to us?

Otherwise, ++

Making account-manager construct-only like you did seems like the way to go for the simple semantics, I'll probably make it a constructor param for an added constructor to the tp-qt4 AbstractClient base-classes too.

Could you elaborate a bit more in TpBaseClient:account-manager: why/when a user would want (or not) to pass a specific AM. I know why but I think that would be less clear for people not familar with TP as that's pretty subtil.

Do we really need new_with_am variants for TpSimple objects? We could claim that if you don't want to use the "default" AM you are not that simple any more and so should use TpBaseClient directly.

I'd add a small comment explaining why/when  _tp_base_client_set_likely_account() could be used.

(In reply to comment #2)
> You probably want to check that the bus is the one the AccountManager object
> given to TpBaseClient is on if both are specified.

I thought about that, but I couldn't see why it strictly *needed* to be forbidden... better to forbid suspicious things now and relax our checks later, though, so I'll do that.

> Also, in likely-account: in which cases exactly can the account not be the
> likely account? (The filter is empty, so MC shouldn't dispatch anything to us
> apart from our request which will indeed have the same account). Somebody
> guessing our service name and setting it as a preferred handler?

Yes, or even someone who isn't MC calling our HandleChannels function directly. We should try to behave correctly in either case. I can see that having HandleChannels fail might be considered more correct here than trying to cope, though, so I'll change the semantics from "likely account" to "only this account" and make HandleChannels raise InvalidArgument for other accounts.

> IF there are valid corner-cases, in which this can happen, then shouldn't we
> actually pass the account's AccountManager, if any, to the TpBaseClient rather
> than just the bus, to get correctly shared accounts outside the singleton AM
> instance even if it's not the likely account?

The account needn't have an AccountManager at all, in an asymmetric Handler that never actually needs to list accounts because it never initiates channels (a file transfer receiver, or a VNC client accepting a stream tube).

(In reply to comment #3)
> Could you elaborate a bit more in TpBaseClient:account-manager: why/when a user
> would want (or not) to pass a specific AM. I know why but I think that would be
> less clear for people not familar with TP as that's pretty subtil.

Yes, I should do that.

> Do we really need new_with_am variants for TpSimple objects? We could claim
> that if you don't want to use the "default" AM you are not that simple any more
> and so should use TpBaseClient directly.

The simple handler is simple along a different axis :-) It's a way to handle channels without paying the boilerplate cost for a subclass.

If we'd thought of this stuff up-front, I'd have made _new() take an AM and not had a TpDBusDaemon version at all, but we're stuck with the _new() versions now. It might even be worth deprecating them.

> I'd add a small comment explaining why/when 
> _tp_base_client_set_likely_account() could be used.

It's basically just for TpAccountChannelRequest; I'll say so.

*** Bug 29619 has been marked as a duplicate of this bug. ***

(In reply to comment #4)
> better to forbid suspicious things now and relax our checks later,
> though, so I'll do that.

Done.

> I'll change the semantics from "likely account" to "only this
> account" and make HandleChannels raise InvalidArgument for other accounts.

Done.

> (In reply to comment #3)
> > Could you elaborate a bit more in TpBaseClient:account-manager: why/when a user
> > would want (or not) to pass a specific AM. I know why but I think that would be
> > less clear for people not familar with TP as that's pretty subtil.

Done.

> > Do we really need new_with_am variants for TpSimple objects? We could claim
> > that if you don't want to use the "default" AM you are not that simple any more
> > and so should use TpBaseClient directly.

I've documented these a bit better.

> > I'd add a small comment explaining why/when 
> > _tp_base_client_set_likely_account() could be used.
> 
> It's basically just for TpAccountChannelRequest; I'll say so.

Done.

Your patches look good to me. Maybe it would be worth documenting if TP_ACCOUNT_MANAGER_FEATURE_CORE has to be prepared on the AM when passing it to a constructor?

(In reply to comment #7)
> Your patches look good to me.

Merged, will be in 0.11.14.

> Maybe it would be worth documenting if
> TP_ACCOUNT_MANAGER_FEATURE_CORE has to be prepared on the AM when passing it to
> a constructor?

Good idea.
http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/trivia

(In reply to comment #8)
> http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=shortlog;h=refs/heads/trivia

r+ from Guillaume on IRC, also merged for 0.11.14.