Bug 27791

In "SSO accounts can have multiple services[...]":

> +          gchar *slist = NULL;

Seems to be unused?

> +          AgManager * agm = ag_account_get_manager (account);

AgManager *agm = ...

> +          GList *srv = NULL;

I'd prefer this called iter or something; we shouldn't have two variables in the same scope called srv and service :-)

> +              AgService *service = srv->data;
> +              const gchar *name = ag_service_get_name (service);

This could just be: const gchar *name = ag_service_get_name (srv->data);

In "Generate headers and code for the new SSO properties interface":

> +       _gen/svc-Account_Manager_Interface_Sso.h \

I'd prefer Account_Manager_Interface_SSO

> +<node name="/Account_Sso"

This node name should be consistent with the filename - pick one. I prefer Account.Interface.SSO myself.

Ideally, both should be consistent with the D-Bus interface name.

> +    <property name="Service" type="s" access="read"

Didn't you just say that accounts can have many services?

> +<node name="/Account_Manager_Telepathy_Sso"

Not consistent with the filename or the interface name

> +      <tp:docstring xmlns="http://www.w32.org/1999/xhtml">

w32? :-)

> +      <arg direction="in" name="ID" type="s">
> +        <tp:docstring><p>SSO Account ID</p></tp:docstring>

Aren't SSO account IDs integers in the other interface? Make your mind up!

(I see you fixed this later.)

In "Implement the new SSO related utility methods":

> +static void sso_util_iface_init (McSvcAccountManagerTelepathySsoClass *iface,

Where did the word "util" come from?

> +                               const gchar *in_Service,

Just because the generated code names its arguments like this doesn't mean you have to - the generated code is trying hard to avoid colliding with reserved words, etc., but you could just call it "service" or "service_name" or something.

> McSvcAccountManagerTelepathySso *self,
...
> McdAccountManager *manager = MCD_ACCOUNT_MANAGER (self);

I'd prefer the argument of an unhelpful interface type to be called iface, svc, sso or something, and the McdAccountManager to be called self.

> +                gchar *slist =

Please avoid uncommon abbreviations. I don't think things that aren't a GSList should be called slist.

I think you're doing this wrong: g_key_file_get_string + g_strsplit isn't suitable for retrieving a string list (for a start, it behaves incorrectly in the presence of escaped semicolons, if there ever are any). Why don't you use g_key_file_get_string_list?

> +                    GStrv svcs = g_strsplit (slist, ";", 0);

Please avoid uncommon abbreviations. "services" would be fine.

> +              g_key_file_get_string (cache, name, "libacct-uid", NULL);

Use g_key_file_get_integer if you care about the value (sso_util_get_account), or g_key_file_get_value if you just care about presence or absence (sso_util_get_service_accounts).

> +        GError *error = g_error_new (TP_TYPE_ERROR,
> +                                     TP_ERROR_DOES_NOT_EXIST,

It'd be good to document this possible error in the XML.

> I think you're doing this wrong: g_key_file_get_string + g_strsplit isn't
> suitable for retrieving a string list (for a start, it behaves incorrectly in
> the presence of escaped semicolons, if there ever are any). Why don't you use
> g_key_file_get_string_list?

The plugin does't know there is even a key file - it uses the shim interface
which only lets it set a string - SSO service names can't have ; in them,
so that was used as a separator

Additionally the Account interface was droped from requirements, I should 
have removed it completely before committing, so problems with it haven't
been addressed, it's gone now.

The other issues should all have been addressed now.

(In reply to comment #2)
> > I think you're doing this wrong: g_key_file_get_string + g_strsplit isn't
> > suitable for retrieving a string list (for a start, it behaves incorrectly in
> > the presence of escaped semicolons, if there ever are any). Why don't you use
> > g_key_file_get_string_list?
> 
> The plugin does't know there is even a key file - it uses the shim interface
> which only lets it set a string - SSO service names can't have ; in them,
> so that was used as a separator

It sounds to me like the shim interface is at fault here. :)

> > +        GError *error = g_error_new (TP_TYPE_ERROR,
> > +                                     TP_ERROR_DOES_NOT_EXIST,
> 
> It'd be good to document this possible error in the XML.

This doesn't seem to have happened.

(In reply to comment #3)
> > > +        GError *error = g_error_new (TP_TYPE_ERROR,
> > > +                                     TP_ERROR_DOES_NOT_EXIST,
> > 
> > It'd be good to document this possible error in the XML.
> 
> This doesn't seem to have happened.

Fix looks good, ship it!

This went in at commit dfdc4d82c654dad67c89b8a37951f5a079b37a8b, forgot to update bug then.