[Bug 28915] MC does not save "old-ssl" flag

[bugzilla-daemon at freedesktop.org]

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

--- Comment #9 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-09-02 10:14:25 PDT ---
"Implement the new McdStorage interface" duplicates a lot of existing code, and
then "Tidy up most of the mcd-account value getting/setting" deletes the
original versions. I'd have preferred it if there was one commit per virtual
method (or group of closely-related virtual methods) which *moved* the original
implementation into the McdStorage implementation; as it stands at the moment,
it's rather difficult to review.

> void mcd_storage_store_connections (McdStorage *storage)

Looks like this should be internal (G_GNUC_INTERNAL, _ prefix, -priv.h).

> +  g_object_get (master, "account-manager", &account_manager, NULL);

account_manager is leaked. Coding style: new line before "account-manager" and
before NULL.

McdStorage calls g_assert a lot. All checks on its arguments should be
g_return[_val]_if_fail instead.

It appears that the set_string and set_value methods return whether the value
changed, rather than whether setting succeeded. This is unconventional, so it
deserves a (skeletal) doc-comment.

Could you rename list_accounts to dup_accounts, and likewise for list_settings,
to indicate that they return a new strv?

get_string should be dup_string, because it g_strdups.

get_value should be dup_value. It deserves a comment saying that it returns a
slice-allocated GValue.

The indentation in the default case of _storage_get_value is strange; please
fix it (Telepathy style preferred, historical MC style tolerated).

In several places in _storage_get_value, a non-NULL GValue is returned with
@error set, which isn't right (admittedly, this is not a regression, since
keyfile_get_value had this bug too).

> +void mcd_plugin_account_manager_ready (McdPluginAccountManager *self);
> +
> +void mcd_plugin_account_manager_connect_signal (const gchar *signal,

These should be underscore-prefixed and/or G_GNUC_INTERNAL; ideally, so should
everything else in that header, which isn't installed.

> +_storage_set_string (McdStorage *storage,

I'd prefer tp_strdiff instead of a cmp-style function.

You should be passing an escaped form of @val (the result of
g_key_file_get_value) to update_storage; please test this with strings
containing escape characters.

> +_storage_set_value (McdStorage *storage,

Again, I'd prefer tp_strdiff.

Why are you NIHing g_key_file_set_boolean et al? I can see that the code
structure is vaguely attractive, but...

The code to set GStrv values will fall apart if the strings contain escaped
semicolons: by calling g_key_file_set_string_list followed by
g_key_file_get_string, you're losing the distinction between ; and \; (or
possibly between \; and \\;, just pile on extra escapes until you find a
failing case). Instead, you should be passing the raw value (result of
g_key_file_get_value) to update_storage.

> +  /* special case, might be synthesised by the backend: */
> +  if (value == NULL && g_strcmp0 ("sso-services", key));

I'd prefer get_sso_services to be a separate method, if it needs this special
behaviour.

> +    g_debug ("FETCHED AUTO PRESENCE: %u %s %s", presence_type, presence, message);

Delete?

> +        g_warning("\nsetting "MC_ACCOUNTS_KEY_AUTO_PRESENCE_MESSAGE" to %s", new_message);

Delete!

> -        mcd_account_changed_property (account, "Enabled", &value);
> +        mcd_account_changed_property (account, MC_ACCOUNTS_KEY_ENABLED, &value);

I'd prefer to reserve MC_ACCOUNTS_KEY_ENABLED for things written into the
account's (pseudo-)keyfile, and stick to "Enabled" when targeting D-Bus (like
here), even though MC_ACCOUNTS_KEY_ENABLED happens to also be "Enabled".

> +mcd_account_get_storage (McdAccount *account)

Can this be internal within src (underscore prefix, G_GNUC_INTERNAL, -priv
header)?

Non-blockers
============

McdStorage could use G_DEFINE_INTERFACE, with a GLib 2.24 dependency.

The function currently called get_value, which I suggested you rename to
dup_value, might make more sense if it returned void and either took a
zero-filled GValue as argument, or took an initialized GValue of type @type as
argument. This would make mcd_account_get_string_val() trivial.

> +        if (message && message[0] != 0)

tp_str_empty()?

> +        /* don't keep a reference to the storage: we can safely assume
>           * its lifetime is longer than the McdAccount's */

Now that the reference isn't circular, I'd prefer this to be a real ref,
released in dispose.

> +      case PROP_DBUS_DAEMON:
> +        /* as above, the dbus daemon is held by the account manager *
> +         * which must live longer than the account:                 */

Likewise.

> Remove direct access to AM internals from account-manager-query

One day, please close Bug #24914 by deleting this entire interface. As I
understand it, telepathy-qt4 supports filtering accounts at the client side,
which is the way forward really.

> Remove direct access to account manager from mcd-account-compat,c

This interface is a dumping ground for misc from Maemo 5, which we're now
designing replacements for. telepathy-qt4 doesn't offer access to any of this
interface's functionality. Bug #24899 asks for part of it to be deleted, but to
be honest all of it should be deleted.

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