[Bug 27791] 3rd party code needs to be able to map SSO (libaccounts) IDs to account object paths

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=27791

--- Comment #1 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-04-22 07:57:43 PDT ---
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.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.