[Bug 35896] Use XDG directories

[bugzilla-daemon at freedesktop.org]

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

--- Comment #14 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2012-08-27 11:39:30 UTC ---
I've only reviewed the first couple of commits so far.

----

> + (GDestroyNotify) g_free, (GDestroyNotify) g_hash_table_unref);

Nitpicking: g_free is already a GDestroyNotify and doesn't need this cast.

> +/* simply replaces '/' with '_' */
> +static gchar *
> +escape_path (const gchar *str)

This function can be reduced to

    return g_strdelimit (g_strdup (str), "/", '_');

To guarantee collision-free filenames I think I'd prefer either '-' (or
anything in [+%,=~^@] if you prefer), or keeping '/' (leading to use of
subdirectories), rather than '_', yielding filenames like this:

    ~/.config/mission-control/accounts/gabble-jabber-smcv_40example_2ecom

(_ is special because we're using tp_escape_as_identifier, so it's the escape
character.)

> + /* TODO: use this instead of MC_ACCOUNT_DIR
> + self->directory = g_build_filename (
> + g_get_user_config_dir (), "mission-control", "accounts", NULL);
> + */

Presumably fixed later

> + /* TODO: this will only work in the test environment for now. */
> + self->directory = g_build_filename (g_getenv ("MC_ACCOUNT_DIR"), NULL);

Presumably fixed later

> + props = g_hash_table_new_full (g_str_hash, g_str_equal,
> + (GDestroyNotify) g_free, (GDestroyNotify) tp_g_value_slice_free);

Why not string => GVariant? (I suppose you have to do the same sort of
serialization dance in commit_one either way, though.)

> The big problem with this right now is that the mcp API depends
> on all account properties being strings. Once the API is fixed
> we can update this plugin easily to support it, and actually save
> types to disk!

I'd prefer to add get_variant(), set_variant() vfuncs (but I see you did that
later so never mind).

> +/* We happen to know that the string MC gave us is "sufficiently escaped" to
> + * put it in the keyfile as-is. */
> +static gboolean
> +_set (const McpAccountStorage *storage,

This comment is irrelevant copypasta.

> + mcp_account_manager_set_value (am, account,
> + key, val);

It's not your fault, but this API is madness...

> +static gboolean
> +_delete (const McpAccountStorage *storage,
> + const McpAccountManager *am,
> + const gchar *account,
> + const gchar *key)

key is ignored, the entire account is deleted. Is this how it's meant to work?

> + /* owned (gchar *) of account => (gpointer) boolean whether account
> + * has changed and so needs to be written to disk. in reality the
> + * value is always GUINT_TO_POINTER(TRUE) or the account is simply
> + * absent. */
> + GHashTable *accounts_modified;

It's (becoming) conventional to represent a set as a hash table (key => key).
GLib 2.32 has some magic to make those a bit more space-efficient, even. See
the GHashTable docs.

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