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

Created attachment 72708 [details] [review]
proposed patch

This patch comes from a big patch proposed for telepathy branch and can be applied to HEAD. I already fixed most comments from Richard.

Comments are welcome !

Most comments? What is left to do that Richard said then?

datalist vs hashtable is still in debate... Otherwise it's ok I think.

Created attachment 72721 [details] [review]
new proposal

Remove gossip_account_param_get_all and replace it with a nicer gossip_account_param_foreach

Created attachment 72754 [details] [review]
a little polish

 - Useless to pass the flags to gossip_account_param_foreach(), just run the callback for every param, the user can easily check the flag himself.

 - Prevent from emitting CHANGED signal and g_object_notify 2 times if we use gossip_account_set_*().

Created attachment 72787 [details] [review]
better like that

I think it's better with a GHashTable in the end. Both provide exactly the same features and I think it's easier to not worry about GQuark and g_datalist API which is far less used and known and thus more complicate to maintain. I also moved g_object_notify() and emit the CHANGED signal in gossip_account_param_set_full() instead of in each gossip_account_set_*() functions. Like that most gossip_account_set_* are only kept for API compatibility and can be replaced by gossip_account_param_set_[string, g_value, full](account, "param_name", value);

I might be missing something but isn't it a bit weird that the param names don't match the properties? (account/id, use-ssl/old-ssl)?

I agree that the hashtable approach is better.

Because that's the name used by telepathy...

Right, but since we are adding this thing in libgossip and not as some kind of telepathy specific layer somewhere, we shouldn't do that kind of translation, in my opinion. Is there some other way to do it?

Maybe the easiest way is having those names hidden in some

#define GOSSIP_ACCOUNT_PARAM_ID "id"

which can be easily changed in the telepathy's branch by

#ifdef USE_TELEPATHY
#define GOSSIP_ACCOUNT_PARAM_ID "account"
#else
#define GOSSIP_ACCOUNT_PARAM_ID "id"
#endif

What do you think about that ?

Shouldn't the telepathy specific code handle mapping between its property names and the gossip ones? Nothing in the libgossip API exposed should have any telehpathy specific stuff, imo.

ok so what about using normal names and add in gossip_account_param_set_full() only in the telepathy branch:

#ifdef USE_TELEPATHY
param_name = gossip_telepathy_account_param_name_convert (param_name);
#endif

this function can looks like:
if (strcmp (name, "id") == ) {return "account";}

Ok for that ?

Created attachment 72810 [details] [review]
don't use telepathy's parameters names

It doesn't really change anything, but at least it's better in the meantime. Are we really going to have hardcoded telepathy code in libgossip? What I mean is that any telepathy specific stuff needs to be put in the telepathy protocol code, that is in protocols/telepathy/, not in libgossip.

However, if the telepathy names are good, we can also change libgossip to use them instead, but then we should change it completely, not just half-way (i.e. still no special telepathy code in libgossip that translates between property names). And update the ui code of course.

 

gossip_telepathy_account_param_name_convert() goes to telepathy-utils.c so not in libgossip. I think we have not the choice to have some #ifdef USE_TELEPATHY in libgossip, there is one in gossip-protocol.c and some in gossip-account(-manager).c for special parameters. I can post the diff between libgossip HEAD and TELEPATHY if you want to see how it looks like.

Forget to say:
gossip_account_set_*() are kept for API compatibility, if we want to rename them why not simple remove them and use gossip_account_param_set_*() ?

Yes, that's better (at least in my opinion).

Ok so what's missing now before committing this patch ? I think it's ready.

Just my appendix, that's all :)
I will commit it when I next get the chance (maybe this weekend).

Committed thanks ;)