Bug 47100

Agreed, please also include something like:
futur.set_requested_presence(Tp.CONNECTION_PRESENCE_TYPE_AVAILABLE, "available", "Oh hi")

That would allow us to get rid of a GValueArray (deprecated) in Empathy.

Actually, may this could replace EmpathyAccountSettings completely.

(In reply to comment #0)
> There should be a less D-Bus-API-dependent way to create accounts with
> specified values for properties. Something like this, perhaps (in pygi-like
> pseudocode):
> 
>     future = Tp.FutureAccount(Tp.AccountManager.dup(),
>         "gabble", "jabber")
> 
>     future.set_display_name('Main Jabber account')
>     future.set_parameter('account', 'me@example.com')

Do we want to make this have some kind of prepare function so it can get the TpProtocol to verify each connection parameter before calling CreateAccount and finding the params are wrong later?

(In reply to comment #3)
> Do we want to make this have some kind of prepare function so it can get the
> TpProtocol to verify each connection parameter before calling CreateAccount and
> finding the params are wrong later?

Something like:

    future.prepare_async(cb)

    def cb(future):
        future.set_parameter('account', 'foo@example.com') # great.
        future.set_parameter('loldongs', 'foo') # this should raise an exception

so something like this in C:

gboolean
tp_f_a_set_parameter (TpFA *,
                      const gchar *key,
                      GVariant *value,
                      GError **error);

(yes I'll add some easier functions for C, but this one is the most important for introspection)

10:01 < cassidy> jonnylamb, I think I'd keep it the API as smcv described it and fail in create_account_async() if a param is wrong
10:01 < cassidy> one can still use the TpProtocol directly if he wants to check param first
10:02 < jonnylamb> yeah but that's more work and more understanding for the client developer for knowing wtf a TpProtocol is.
10:03 < jonnylamb> hmm.
10:05 < cassidy> jonnylamb, or Tp.FutureAccount could take a TpProtocol instead of a (cm, protocol) ?
10:05 < danni> is knowing what a TpProtocol is any worse than knowing what a CM param was?
10:07 < jonnylamb> hm yeah I guess this is going to be used in, say, UIs where you're iterating the account parameters anyway, so have the TpProtocol around.

Here's a patch! I even added tests and stuff like that.

┏━╸╺━┓┏━╸┏━╸╻ ╻   ╻╺┳╸   ┏━┓╻ ╻╺┳╸
┃  ┏━┛┣╸ ┃  ┣━┫   ┃ ┃    ┃ ┃┃ ┃ ┃ 
┗━╸┗━╸┗━╸┗━╸╹ ╹   ╹ ╹    ┗━┛┗━┛ ╹

Any reason to base this on top of next? I'd like to have it in master if possible so I can start using it in Empathy sooner rather than later.

If you do rebase, you could use the new glib-style versions macro to anotate the new API.

You should add it to telepathy-glib/introspection.am


+  gboolean dispose_has_run;
so old school.

tp_future_account_new should be annotated with (transfer full).

http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=c5405121ef1fb6a9ad5b76942cc58e624b348121

GObject properties are missing the "Since: ...".

+TpFutureAccount * tp_future_account_new (TpAccountManager *account_manager,
+    const gchar *manager, const gchar *protocol) G_GNUC_WARN_UNUSED_RESULT;
one arg per line.

priv->account_manager is not unreffed.

http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=e7c7938b89d79dedf3e99a91d145eac437cba689

Shouldn't tp_future_account_set_display_name() fails if the account has already be created?
This applies to all set_*() methods as well.

http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=50e42faecbbb44552a9d9e04c0b809158d44717c

tp_future_account_constructed: the chain_up should be done first.

Shouldn't TpFutureAccount:parameters: be a GVariant?
Ditto 'properties'?

http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=8f4698dc0c8ebd898d4f2bbc97476f5aab5dd5e2

+  if (priv->account_manager == NULL
+      || priv->cm_name == NULL
+      || priv->proto_name == NULL
+      || priv->display_name == NULL)

Shouldn't we check account_manager, cm_name or proto_name in constructed?

tp_future_account_create_account_finish's doc says that only CORE has bit prepared but actually we get the features from the factory (which is the right thing to do).


http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=64a4af6420377b9de8e277676fd12a1aa2875777
+ * tp_future_account_set_autmatic_presence:
typo

I'm wondering if we shouldn't use a GVariant to store the different (presence, status, message) and so reduce the number of properties.

http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=a64f45dc0e9406771e9960ee135300cc26317131

We could group the 2 avatar properties as well.

http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=a64f45dc0e9406771e9960ee135300cc26317131

tmp is leaked

(In reply to comment #7)
> Any reason to base this on top of next? I'd like to have it in master if
> possible so I can start using it in Empathy sooner rather than later.

Actually we won't be able to test this in Empathy while it's based on next as that means we should port it to next first. Could you please rebase your branch on top of master?

During IRC discussion, people weren't very pleased with the name I proposed, since "future" already has a (different) meaning in telepathy-spec.

Possibilities include:
* TpFutureAccount (people think this sounds too much like the opposite of deprecated)
* TpNewAccount (same problem)
* TpAccountRequest (semi-collision with TpAccountChannelRequest)
* TpAccountProperties (too much of an implementation detail?)
* TpPotentialAccount

Based on http://thesaurus.com/browse/potential I'm half tempted to suggest TpEmbryonicAccount, TpLatentAccount, TpPlausibleAccount, TpAbeyantAccount or TpIncipientAccount, but I don't think any of those would be very nice to people whose first language isn't English :-)

More from the thesaurus:

forthcoming
impending
imminent
likely
prospective
ulterior (!)
anticipated
expected
planned
proposed

Since it's mostly what EmpathyAccountSettings does, why not TpAccountSettings? You can create an account from its settings, makes sense... no?

Yeah I was pondering that as well. Ideally I'd like to get rid of EmpathyAccountSettings, which is used as an abstraction on top of TpAccount or when creating a new account to display the settings in the account widget.

(In reply to comment #7)
> Any reason to base this on top of next? I'd like to have it in master if
> possible so I can start using it in Empathy sooner rather than later.

I want next to come quickly and I get the impression working on master including working around deprecations, then having to spend more time merging into next and removing said deprecations is going to take longer.

Anyway, I rebased it onto master for you.

> If you do rebase, you could use the new glib-style versions macro to anotate
> the new API.

OK, done.

> You should add it to telepathy-glib/introspection.am

Done.

> +  gboolean dispose_has_run;
> so old school.

DATS HOW I ROLL YO

…removed.

> tp_future_account_new should be annotated with (transfer full).

Le done. That's French for done.

> http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=c5405121ef1fb6a9ad5b76942cc58e624b348121
> 
> GObject properties are missing the "Since: ...".

Well, so is every other symbol? I thought Since: things were only useful for methods added to objects where the object has existed for a while. I added Since: UNRELEASED for the main TpFutureAccount header. Adding Since to each symbol/property makes it feel like there is one which isn't the same version as the others, which is wrong. What do you think? I can add them if you feel strongly about it.

> +TpFutureAccount * tp_future_account_new (TpAccountManager *account_manager,
> +    const gchar *manager, const gchar *protocol) G_GNUC_WARN_UNUSED_RESULT;
> one arg per line.

huh, you changed that in 2009[0]? I don't remember that. Oh well, done.

0. http://telepathy.freedesktop.org/wiki/Style?action=diff&rev1=22&rev2=23

> priv->account_manager is not unreffed.

Good call, fixed.

> http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=e7c7938b89d79dedf3e99a91d145eac437cba689
> 
> Shouldn't tp_future_account_set_display_name() fails if the account has already
> be created?
> This applies to all set_*() methods as well.

Well I wondered what to do about this. You think I should make _set_ and _create_account be no-ops if the account has been created?

> http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=50e42faecbbb44552a9d9e04c0b809158d44717c
> 
> tp_future_account_constructed: the chain_up should be done first.

Yes, done.

> Shouldn't TpFutureAccount:parameters: be a GVariant?
> Ditto 'properties'?

Fair point. Done.

> http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=8f4698dc0c8ebd898d4f2bbc97476f5aab5dd5e2
> 
> +  if (priv->account_manager == NULL
> +      || priv->cm_name == NULL
> +      || priv->proto_name == NULL
> +      || priv->display_name == NULL)
> 
> Shouldn't we check account_manager, cm_name or proto_name in constructed?

OK.

> tp_future_account_create_account_finish's doc says that only CORE has bit
> prepared but actually we get the features from the factory (which is the right
> thing to do).

I fixed the docs.

> http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=64a4af6420377b9de8e277676fd12a1aa2875777
> + * tp_future_account_set_autmatic_presence:
> typo

Fixed.

> I'm wondering if we shouldn't use a GVariant to store the different (presence,
> status, message) and so reduce the number of properties.
> 
> http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=a64f45dc0e9406771e9960ee135300cc26317131
> 
> We could group the 2 avatar properties as well.

I'm merely copying what TpAccount does, so the two feel more similar. I think keeping them consistent is a good idea.

> http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=a64f45dc0e9406771e9960ee135300cc26317131
> 
> tmp is leaked

Yes, good point. Fixed.

(In reply to comment #11)
> Since it's mostly what EmpathyAccountSettings does, why not TpAccountSettings?
> You can create an account from its settings, makes sense... no?

No I don't think this makes any sense as this is specifically about an account that does not exist yet.

I can't think/pick of a good answer though.

(In reply to comment #13)
> (In reply to comment #7)
> > Any reason to base this on top of next? I'd like to have it in master if
> > possible so I can start using it in Empathy sooner rather than later.
> 
> I want next to come quickly and I get the impression working on master
> including working around deprecations, then having to spend more time merging
> into next and removing said deprecations is going to take longer.

I don't think it's that bad (especially with new files/API) and having it in master makes porting non trivial app (basically Empathy) much easier.

> Anyway, I rebased it onto master for you.

Cool thanks, I'll try using it in Empathy.

> > If you do rebase, you could use the new glib-style versions macro to anotate
> > the new API.
> 
> OK, done.

Strictly speaking you should use _TP_AVAILABLE_IN_UNRELEASED but this will be part of 0.20 so that's fine.



> > GObject properties are missing the "Since: ...".
> 
> Well, so is every other symbol? I thought Since: things were only useful for
> methods added to objects where the object has existed for a while. I added
> Since: UNRELEASED for the main TpFutureAccount header. Adding Since to each
> symbol/property makes it feel like there is one which isn't the same version as
> the others, which is wrong. What do you think? I can add them if you feel
> strongly about it.

I generally add it to all the API if only to be exhaustive and I think GLib does as well.
 
> > +TpFutureAccount * tp_future_account_new (TpAccountManager *account_manager,
> > +    const gchar *manager, const gchar *protocol) G_GNUC_WARN_UNUSED_RESULT;
> > one arg per line.
> 
> huh, you changed that in 2009[0]? I don't remember that. Oh well, done.

Cool, now you just have to review and fix all the code you wrote these last 3 years.


> > http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=e7c7938b89d79dedf3e99a91d145eac437cba689
> > 
> > Shouldn't tp_future_account_set_display_name() fails if the account has already
> > be created?
> > This applies to all set_*() methods as well.
> 
> Well I wondered what to do about this. You think I should make _set_ and
> _create_account be no-ops if the account has been created?

Or at least raise a warning as in tp_future_account_create_account_async().


> > +  if (priv->account_manager == NULL
> > +      || priv->cm_name == NULL
> > +      || priv->proto_name == NULL
> > +      || priv->display_name == NULL)
> > 
> > Shouldn't we check account_manager, cm_name or proto_name in constructed?
> 
> OK.

http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-master-47100&id=09ece4e16b3a8b99d92a9f48610d92949edfa732

Looks a bit weird to me. I think we should either requires users to pass the display name when constructing the object or have a sensible default.
Empathy uses (the first where we have the data):
- $service ($param-account)
- $param-account
- $protocol account
- New account


> > I'm wondering if we shouldn't use a GVariant to store the different (presence,
> > status, message) and so reduce the number of properties.
> > 
> > http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=a64f45dc0e9406771e9960ee135300cc26317131
> > 
> > We could group the 2 avatar properties as well.
> 
> I'm merely copying what TpAccount does, so the two feel more similar. I think
> keeping them consistent is a good idea.

Consistency is good indeed. I opened bug #49616 to consider changing this in next.


> > http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-account-47100&id=a64f45dc0e9406771e9960ee135300cc26317131
> > 
> > tmp is leaked
> 
> Yes, good point. Fixed.

Did you try valgrinding the test to check for leaks?

You forgot to add API to set TP_PROP_ACCOUNT_SERVICE.

Add G_GNUC_WARN_UNUSED_RESULT for (transfer full) functions ? tp_future_account_create_account_finish and the _new().

I ported Empathy http://cgit.collabora.com/git/user/cassidy/empathy/log/?h=future-account

As you can see the only thing we gain by using this API is just the boilerplate to pass the requested presence, making me a bit sad.
As I said in Comment 12, it would be ace to be able to get rid of EmpathyAccountSettings (or at least removing most of its code). I'm not saying that TpFutureAccount is the right answer to that but I think we should at least considering it.

Should TpFutureAccount be renamed to TpAccountSettings and basically turned to EmpathyAccountSettings?

Should we have another object for that? A common interface implemented by TpAccount and TpFutureAccount?

(In reply to comment #15)
> Strictly speaking you should use _TP_AVAILABLE_IN_UNRELEASED but this will be
> part of 0.20 so that's fine.

Oh yes, good point. Oh well, I'll leave it now.

> (In reply to comment #13)
> > Well, so is every other symbol? I thought Since: things were only useful for
> > methods added to objects where the object has existed for a while. I added
> > Since: UNRELEASED for the main TpFutureAccount header. Adding Since to each
> > symbol/property makes it feel like there is one which isn't the same version as
> > the others, which is wrong. What do you think? I can add them if you feel
> > strongly about it.
> 
> I generally add it to all the API if only to be exhaustive and I think GLib
> does as well.

Fair enough, done.

> Cool, now you just have to review and fix all the code you wrote these last 3
> years.

http://jonnylamb.com/lol.gif

> > Well I wondered what to do about this. You think I should make _set_ and
> > _create_account be no-ops if the account has been created?
> 
> Or at least raise a warning as in tp_future_account_create_account_async().

OK, let's add some g_return_if_fails.

> http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-master-47100&id=09ece4e16b3a8b99d92a9f48610d92949edfa732
> 
> Looks a bit weird to me. I think we should either requires users to pass the
> display name when constructing the object or have a sensible default.
> Empathy uses (the first where we have the data):
> - $service ($param-account)
> - $param-account
> - $protocol account
> - New account

Yeah I think you're right. We can't have a default as it should be localised, so we could just require the display name and get empathy to have a good localised fallback? Okay done.

> Consistency is good indeed. I opened bug #49616 to consider changing this in
> next.

Exciting.

> Did you try valgrinding the test to check for leaks?

I hadn't actually. I just did now and found another leak in the simple account.

(In reply to comment #16)
> You forgot to add API to set TP_PROP_ACCOUNT_SERVICE.

I was hoping no-one would notice. Done.

> Add G_GNUC_WARN_UNUSED_RESULT for (transfer full) functions ?
> tp_future_account_create_account_finish and the _new().

It's already there for _new() but yes I forgot for _finish.

(In reply to comment #17)
> I ported Empathy

Do you want a medal?

> As I said in Comment 12, it would be ace to be able to get rid of
> EmpathyAccountSettings (or at least removing most of its code). I'm not saying
> that TpFutureAccount is the right answer to that but I think we should at least
> considering it.
> Should TpFutureAccount be renamed to TpAccountSettings and basically turned to
> EmpathyAccountSettings?

Well, this kind of has a different purpose, doesn't it? If you could file a bug with your exact requirements listed that would be really useful.

btw I'm going to strongly veto the name TpAccountSettings for this object as it is now as it's completely misleading.

(In reply to comment #18)
> Yeah I think you're right. We can't have a default as it should be localised,
> so we could just require the display name and get empathy to have a good
> localised fallback? Okay done.

agreed.
 
> > Consistency is good indeed. I opened bug #49616 to consider changing this in
> > next.
> 
> Exciting.
> 
> > Did you try valgrinding the test to check for leaks?
> I hadn't actually. I just did now and found another leak in the simple account.

Did you forget to push the fix?


> (In reply to comment #17)
> > As I said in Comment 12, it would be ace to be able to get rid of
> > EmpathyAccountSettings (or at least removing most of its code). I'm not saying
> > that TpFutureAccount is the right answer to that but I think we should at least
> > considering it.
> > Should TpFutureAccount be renamed to TpAccountSettings and basically turned to
> > EmpathyAccountSettings?
> 
> Well, this kind of has a different purpose, doesn't it? If you could file a bug
> with your exact requirements listed that would be really useful.

Indeed, that's a wider scope than the original bug. I'll open a new one.

> btw I'm going to strongly veto the name TpAccountSettings for this object as it
> is now as it's completely misleading.

Agreed.

(In reply to comment #19)
> Did you forget to push the fix?

No?

http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-master-47100&id=6f64f59c7

(In reply to comment #20)
> (In reply to comment #19)
> > Did you forget to push the fix?
> 
> No?
> 
> http://cgit.freedesktop.org/~jonny/telepathy-glib/commit/?h=future-master-47100&id=6f64f59c7

I missed it for some reason. Looks good, of course.

(In reply to comment #19) 
> > (In reply to comment #17)
> > > As I said in Comment 12, it would be ace to be able to get rid of
> > > EmpathyAccountSettings (or at least removing most of its code). I'm not saying
> > > that TpFutureAccount is the right answer to that but I think we should at least
> > > considering it.
> > > Should TpFutureAccount be renamed to TpAccountSettings and basically turned to
> > > EmpathyAccountSettings?
> > 
> > Well, this kind of has a different purpose, doesn't it? If you could file a bug
> > with your exact requirements listed that would be really useful.
> 
> Indeed, that's a wider scope than the original bug. I'll open a new one.

The more I think about it, the more I think that's more of an UI thing. So this branch looks good to me, modulo the name of the new object which I don't really care about tbh.

(In reply to comment #22)
> The more I think about it, the more I think that's more of an UI thing. So this
> branch looks good to me, modulo the name of the new object which I don't really
> care about tbh.

Okay I pushed a patch to rename it.

How'd ya feel about the branch then?

Looks good, assuming that's the name we want to use.

Merged, thanks for the review.