From 3a07794a1901266eee16a5c35f433eba3eda9799 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 14 Nov 2013 15:26:18 +0000 Subject: [PATCH 19/26] mcp_account_storage_set_*: return whether anything changed The plugins are better-placed to do this than McdStorage: they know their own storage format, after all. --- mission-control-plugins/account-storage.c | 23 ++++--- mission-control-plugins/account-storage.h | 16 +++-- src/mcd-account-manager-default.c | 94 +++++++++++++++++++++------ src/mcd-storage.c | 104 +++++++++++++----------------- tests/twisted/dbus-account-plugin.c | 39 +++++++++-- tests/twisted/mcp-account-diversion.c | 33 +++++++--- 6 files changed, 204 insertions(+), 105 deletions(-) diff --git a/mission-control-plugins/account-storage.c b/mission-control-plugins/account-storage.c index 098e336..ca9c7fa 100644 --- a/mission-control-plugins/account-storage.c +++ b/mission-control-plugins/account-storage.c @@ -196,7 +196,7 @@ default_get_restrictions (const McpAccountStorage *storage, return 0; } -static gboolean +static McpAccountStorageSetResult default_set_attribute (McpAccountStorage *storage, McpAccountManager *am, const gchar *account, @@ -204,10 +204,10 @@ default_set_attribute (McpAccountStorage *storage, GVariant *value, McpAttributeFlags flags) { - return FALSE; + return MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED; } -static gboolean +static McpAccountStorageSetResult default_set_parameter (McpAccountStorage *storage, McpAccountManager *am, const gchar *account, @@ -215,7 +215,7 @@ default_set_parameter (McpAccountStorage *storage, GVariant *value, McpParameterFlags flags) { - return FALSE; + return MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED; } static void @@ -521,7 +521,7 @@ mcp_account_storage_get (const McpAccountStorage *storage, * * Since: 5.15.0 */ -gboolean +McpAccountStorageSetResult mcp_account_storage_set_attribute (McpAccountStorage *storage, McpAccountManager *am, const gchar *account, @@ -532,8 +532,10 @@ mcp_account_storage_set_attribute (McpAccountStorage *storage, McpAccountStorageIface *iface = MCP_ACCOUNT_STORAGE_GET_IFACE (storage); SDEBUG (storage, ""); - g_return_val_if_fail (iface != NULL, FALSE); - g_return_val_if_fail (iface->set_attribute != NULL, FALSE); + g_return_val_if_fail (iface != NULL, + MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED); + g_return_val_if_fail (iface->set_attribute != NULL, + MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED); return iface->set_attribute (storage, am, account, attribute, value, flags); } @@ -565,7 +567,7 @@ mcp_account_storage_set_attribute (McpAccountStorage *storage, * * Since: 5.15.0 */ -gboolean +McpAccountStorageSetResult mcp_account_storage_set_parameter (McpAccountStorage *storage, McpAccountManager *am, const gchar *account, @@ -577,7 +579,10 @@ mcp_account_storage_set_parameter (McpAccountStorage *storage, SDEBUG (storage, ""); g_return_val_if_fail (iface != NULL, FALSE); - g_return_val_if_fail (iface->set_parameter != NULL, FALSE); + g_return_val_if_fail (iface != NULL, + MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED); + g_return_val_if_fail (iface->set_parameter != NULL, + MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED); return iface->set_parameter (storage, am, account, parameter, value, flags); } diff --git a/mission-control-plugins/account-storage.h b/mission-control-plugins/account-storage.h index 44f7bd6..3a8766e 100644 --- a/mission-control-plugins/account-storage.h +++ b/mission-control-plugins/account-storage.h @@ -32,6 +32,12 @@ G_BEGIN_DECLS #define MCP_ACCOUNT_STORAGE_PLUGIN_PRIO_NORMAL 100 #define MCP_ACCOUNT_STORAGE_PLUGIN_PRIO_KEYRING 10000 +typedef enum { + MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED = 0, + MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED, + MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED +} McpAccountStorageSetResult; + /* API for plugins to implement */ typedef struct _McpAccountStorage McpAccountStorage; typedef struct _McpAccountStorageIface McpAccountStorageIface; @@ -124,13 +130,13 @@ struct _McpAccountStorageIface McpAccountStorageCreate create; /* Since 5.15.0 */ - gboolean (*set_attribute) (McpAccountStorage *storage, + McpAccountStorageSetResult (*set_attribute) (McpAccountStorage *storage, McpAccountManager *am, const gchar *account, const gchar *attribute, GVariant *val, McpAttributeFlags flags); - gboolean (*set_parameter) (McpAccountStorage *storage, + McpAccountStorageSetResult (*set_parameter) (McpAccountStorage *storage, McpAccountManager *am, const gchar *account, const gchar *parameter, @@ -190,13 +196,15 @@ const gchar *mcp_account_storage_name (const McpAccountStorage *storage); const gchar *mcp_account_storage_description (const McpAccountStorage *storage); const gchar *mcp_account_storage_provider (const McpAccountStorage *storage); -gboolean mcp_account_storage_set_attribute (McpAccountStorage *storage, +McpAccountStorageSetResult mcp_account_storage_set_attribute ( + McpAccountStorage *storage, McpAccountManager *am, const gchar *account, const gchar *attribute, GVariant *value, McpAttributeFlags flags); -gboolean mcp_account_storage_set_parameter (McpAccountStorage *storage, +McpAccountStorageSetResult mcp_account_storage_set_parameter ( + McpAccountStorage *storage, McpAccountManager *am, const gchar *account, const gchar *parameter, diff --git a/src/mcd-account-manager-default.c b/src/mcd-account-manager-default.c index 9ea14d7..e04f408 100644 --- a/src/mcd-account-manager-default.c +++ b/src/mcd-account-manager-default.c @@ -163,7 +163,7 @@ mcd_account_manager_default_class_init (McdAccountManagerDefaultClass *cls) DEBUG ("mcd_account_manager_default_class_init"); } -static gboolean +static McpAccountStorageSetResult set_parameter (McpAccountStorage *self, McpAccountManager *am, const gchar *account, @@ -174,22 +174,61 @@ set_parameter (McpAccountStorage *self, McdAccountManagerDefault *amd = MCD_ACCOUNT_MANAGER_DEFAULT (self); McdDefaultStoredAccount *sa; - sa = ensure_stored_account (amd, account); - amd->save = TRUE; + if (val == NULL) + { + gboolean changed = FALSE; - /* remove it from all sets, then re-add it to the right one if - * non-null */ - g_hash_table_remove (sa->parameters, parameter); - g_hash_table_remove (sa->untyped_parameters, parameter); + sa = lookup_stored_account (amd, account); - if (val != NULL) - g_hash_table_insert (sa->parameters, g_strdup (parameter), - g_variant_ref (val)); + if (sa == NULL) + return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED; - return TRUE; + changed = g_hash_table_remove (sa->parameters, parameter); + /* deliberately not ||= - if we removed it from parameters, we + * still want to remove it from untyped_parameters if it was there */ + changed |= g_hash_table_remove (sa->untyped_parameters, parameter); + + if (!changed) + return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED; + } + else + { + GVariant *old; + + sa = ensure_stored_account (amd, account); + old = g_hash_table_lookup (sa->parameters, parameter); + + if (old == NULL) + { + /* it might still be in untyped_parameters? */ + const gchar *escaped = g_hash_table_lookup (sa->untyped_parameters, + parameter); + gchar *new = mcp_account_manager_escape_variant_for_keyfile ( + am, val); + + if (!tp_strdiff (escaped, new)) + { + g_free (new); + return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED; + } + + g_free (new); + } + else if (g_variant_equal (old, val)) + { + return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED; + } + + g_hash_table_remove (sa->untyped_parameters, parameter); + g_hash_table_insert (sa->parameters, g_strdup (parameter), + g_variant_ref (val)); + } + + amd->save = TRUE; + return MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED; } -static gboolean +static McpAccountStorageSetResult set_attribute (McpAccountStorage *self, McpAccountManager *am, const gchar *account, @@ -198,17 +237,34 @@ set_attribute (McpAccountStorage *self, McpAttributeFlags flags) { McdAccountManagerDefault *amd = MCD_ACCOUNT_MANAGER_DEFAULT (self); - McdDefaultStoredAccount *sa = ensure_stored_account (amd, account); + McdDefaultStoredAccount *sa; - amd->save = TRUE; + if (val == NULL) + { + sa = lookup_stored_account (amd, account); + + if (sa == NULL) + return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED; - if (val != NULL) - g_hash_table_insert (sa->attributes, g_strdup (attribute), - g_variant_ref (val)); + if (!g_hash_table_remove (sa->attributes, attribute)) + return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED; + } else - g_hash_table_remove (sa->attributes, attribute); + { + GVariant *old; - return TRUE; + sa = ensure_stored_account (amd, account); + old = g_hash_table_lookup (sa->attributes, attribute); + + if (old != NULL && g_variant_equal (old, val)) + return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED; + + g_hash_table_insert (sa->attributes, g_strdup (attribute), + g_variant_ref (val)); + } + + amd->save = TRUE; + return MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED; } static gboolean diff --git a/src/mcd-storage.c b/src/mcd-storage.c index 92a2eef..65ed69d 100644 --- a/src/mcd-storage.c +++ b/src/mcd-storage.c @@ -1379,38 +1379,56 @@ mcd_storage_get_integer (McdStorage *self, return g_value_get_int (&tmp); } -static void +static gboolean update_storage (McdStorage *self, const gchar *account, + gboolean parameter, const gchar *key, GVariant *variant) { GList *store; - gboolean parameter = g_str_has_prefix (key, "param-"); McpAccountManager *ma = MCP_ACCOUNT_MANAGER (self); + gboolean updated = FALSE; for (store = stores; store != NULL; store = g_list_next (store)) { McpAccountStorage *plugin = store->data; const gchar *pn = mcp_account_storage_name (plugin); + McpAccountStorageSetResult res; - if (!parameter && - mcp_account_storage_set_attribute (plugin, ma, account, key, variant, - MCP_ATTRIBUTE_FLAG_NONE)) - { - DEBUG ("MCP:%s -> store attribute %s.%s", pn, account, key); - /* set it to NULL in all lower-priority stores */ - variant = NULL; - } - else if (parameter && - mcp_account_storage_set_parameter (plugin, ma, account, key + 6, - variant, MCP_PARAMETER_FLAG_NONE)) + if (parameter) + res = mcp_account_storage_set_parameter (plugin, ma, account, + key, variant, MCP_PARAMETER_FLAG_NONE); + else + res = mcp_account_storage_set_attribute (plugin, ma, account, + key, variant, MCP_ATTRIBUTE_FLAG_NONE); + + switch (res) { - DEBUG ("MCP:%s -> store parameter %s.%s", pn, account, key); - /* set it to NULL in all lower-priority stores */ - variant = NULL; + case MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED: + DEBUG ("MCP:%s -> store %s %s.%s", pn, + parameter ? "parameter" : "attribute", account, key); + updated = TRUE; + /* set it to NULL in all lower-priority stores */ + variant = NULL; + break; + + case MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED: + DEBUG ("MCP:%s -> failed to store %s %s.%s", + pn, parameter ? "parameter" : "attribute", account, key); + break; + + case MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED: + DEBUG ("MCP:%s -> no change to %s %s.%s", + pn, parameter ? "parameter" : "attribute", account, key); + break; + + default: + g_warn_if_reached (); } } + + return updated; } /* @@ -1482,7 +1500,6 @@ mcd_storage_set_attribute (McdStorage *self, const GValue *value) { McdStorageAccount *sa; - GVariant *old_v; GVariant *new_v; gboolean updated = FALSE; @@ -1499,21 +1516,13 @@ mcd_storage_set_attribute (McdStorage *self, else new_v = NULL; - old_v = g_hash_table_lookup (sa->attributes, attribute); - - if (!mcd_nullable_variant_equal (old_v, new_v)) - { - /* First put it in the attributes hash table. (Watch out, this might - * invalidate old_v.) */ - if (new_v == NULL) - g_hash_table_remove (sa->attributes, attribute); - else - g_hash_table_insert (sa->attributes, g_strdup (attribute), - g_variant_ref (new_v)); + if (new_v != NULL) + g_hash_table_insert (sa->attributes, g_strdup (attribute), + g_variant_ref (new_v)); + else + g_hash_table_remove (sa->attributes, attribute); - update_storage (self, account, attribute, new_v); - updated = TRUE; - } + updated = update_storage (self, account, FALSE, attribute, new_v); tp_clear_pointer (&new_v, g_variant_unref); return updated; @@ -1541,10 +1550,7 @@ mcd_storage_set_parameter (McdStorage *self, const gchar *parameter, const GValue *value) { - GVariant *old_v; GVariant *new_v = NULL; - const gchar *old_escaped; - gchar *new_escaped = NULL; McdStorageAccount *sa; gboolean updated = FALSE; @@ -1557,37 +1563,19 @@ mcd_storage_set_parameter (McdStorage *self, if (value != NULL) { - new_escaped = mcd_keyfile_escape_value (value); new_v = g_variant_ref_sink (dbus_g_value_build_g_variant (value)); } - old_v = g_hash_table_lookup (sa->parameters, parameter); - old_escaped = g_hash_table_lookup (sa->escaped_parameters, parameter); + g_hash_table_remove (sa->escaped_parameters, parameter); - if (old_v != NULL) - updated = !mcd_nullable_variant_equal (old_v, new_v); - else if (old_escaped != NULL) - updated = tp_strdiff (old_escaped, new_escaped); + if (new_v != NULL) + g_hash_table_insert (sa->parameters, g_strdup (parameter), + g_variant_ref (new_v)); else - updated = (value != NULL); - - if (updated) - { - gchar key[MAX_KEY_LENGTH]; - - g_hash_table_remove (sa->parameters, parameter); - g_hash_table_remove (sa->escaped_parameters, parameter); + g_hash_table_remove (sa->parameters, parameter); - if (new_v != NULL) - g_hash_table_insert (sa->parameters, g_strdup (parameter), - g_variant_ref (new_v)); - - g_snprintf (key, sizeof (key), "param-%s", parameter); - update_storage (self, account, key, new_v); - return TRUE; - } + updated = update_storage (self, account, TRUE, parameter, new_v); - g_free (new_escaped); tp_clear_pointer (&new_v, g_variant_unref); return updated; } diff --git a/tests/twisted/dbus-account-plugin.c b/tests/twisted/dbus-account-plugin.c index 733fbef..e43d40e 100644 --- a/tests/twisted/dbus-account-plugin.c +++ b/tests/twisted/dbus-account-plugin.c @@ -1082,7 +1082,7 @@ test_dbus_account_plugin_get (const McpAccountStorage *storage, return TRUE; } -static gboolean +static McpAccountStorageSetResult test_dbus_account_plugin_set_attribute (McpAccountStorage *storage, McpAccountManager *am, const gchar *account_name, @@ -1092,6 +1092,8 @@ test_dbus_account_plugin_set_attribute (McpAccountStorage *storage, { TestDBusAccountPlugin *self = TEST_DBUS_ACCOUNT_PLUGIN (storage); Account *account = lookup_account (self, account_name); + GVariant *old; + guint old_flags; g_return_val_if_fail (account_name != NULL, FALSE); g_return_val_if_fail (attribute != NULL, FALSE); @@ -1099,10 +1101,13 @@ test_dbus_account_plugin_set_attribute (McpAccountStorage *storage, DEBUG ("%s of %s", attribute, account_name); if (!self->active || account == NULL) - return FALSE; + return MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED; if (value == NULL) { + if (!g_hash_table_contains (account->attributes, attribute)) + return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED; + g_hash_table_remove (account->attributes, attribute); g_hash_table_remove (account->attribute_flags, attribute); g_hash_table_add (account->uncommitted_attributes, g_strdup (attribute)); @@ -1112,9 +1117,16 @@ test_dbus_account_plugin_set_attribute (McpAccountStorage *storage, "DeferringDeleteAttribute", g_variant_new_parsed ("(%o, %s)", account->path, attribute), NULL); - return TRUE; + return MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED; } + old = g_hash_table_lookup (account->attributes, attribute); + old_flags = GPOINTER_TO_UINT (g_hash_table_lookup ( + account->attribute_flags, attribute)); + + if (old != NULL && g_variant_equal (old, value) && old_flags == flags) + return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED; + g_hash_table_insert (account->attributes, g_strdup (attribute), g_variant_ref (value)); g_hash_table_insert (account->attribute_flags, g_strdup (attribute), @@ -1127,10 +1139,10 @@ test_dbus_account_plugin_set_attribute (McpAccountStorage *storage, g_variant_new_parsed ("(%o, %s, %v)", account->path, attribute, value), NULL); - return TRUE; + return MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED; } -static gboolean +static McpAccountStorageSetResult test_dbus_account_plugin_set_parameter (McpAccountStorage *storage, McpAccountManager *am, const gchar *account_name, @@ -1140,6 +1152,8 @@ test_dbus_account_plugin_set_parameter (McpAccountStorage *storage, { TestDBusAccountPlugin *self = TEST_DBUS_ACCOUNT_PLUGIN (storage); Account *account = lookup_account (self, account_name); + GVariant *old; + guint old_flags; g_return_val_if_fail (account_name != NULL, FALSE); g_return_val_if_fail (parameter != NULL, FALSE); @@ -1147,10 +1161,14 @@ test_dbus_account_plugin_set_parameter (McpAccountStorage *storage, DEBUG ("%s of %s", parameter, account_name); if (!self->active || account == NULL) - return FALSE; + return MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED; if (value == NULL) { + if (!g_hash_table_contains (account->parameters, parameter) && + !g_hash_table_contains (account->untyped_parameters, parameter)) + return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED; + g_hash_table_remove (account->parameters, parameter); g_hash_table_remove (account->untyped_parameters, parameter); g_hash_table_remove (account->parameter_flags, parameter); @@ -1164,6 +1182,13 @@ test_dbus_account_plugin_set_parameter (McpAccountStorage *storage, return TRUE; } + old = g_hash_table_lookup (account->parameters, parameter); + old_flags = GPOINTER_TO_UINT (g_hash_table_lookup ( + account->parameter_flags, parameter)); + + if (old != NULL && g_variant_equal (old, value) && old_flags == flags) + return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED; + g_hash_table_remove (account->untyped_parameters, parameter); g_hash_table_insert (account->parameters, g_strdup (parameter), g_variant_ref (value)); @@ -1177,7 +1202,7 @@ test_dbus_account_plugin_set_parameter (McpAccountStorage *storage, g_variant_new_parsed ("(%o, %s, %v)", account->path, parameter, value), NULL); - return TRUE; + return MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED; } static gboolean diff --git a/tests/twisted/mcp-account-diversion.c b/tests/twisted/mcp-account-diversion.c index e99c614..7bba5d8 100644 --- a/tests/twisted/mcp-account-diversion.c +++ b/tests/twisted/mcp-account-diversion.c @@ -111,7 +111,7 @@ _create_config (void) DEBUG ("created %s", file); } -static gboolean +static McpAccountStorageSetResult _set (McpAccountStorage *self, McpAccountManager *am, const gchar *account, @@ -121,9 +121,10 @@ _set (McpAccountStorage *self, { AccountDiversionPlugin *adp = ACCOUNT_DIVERSION_PLUGIN (self); gchar *val_str; + gboolean changed; if (g_str_has_prefix (account, DONT_DIVERT)) - return FALSE; + return MCP_ACCOUNT_STORAGE_SET_RESULT_FAILED; if (val == NULL) { @@ -131,7 +132,10 @@ _set (McpAccountStorage *self, GStrv keys; if (g_key_file_remove_key (adp->keyfile, account, key, NULL)) - adp->save = TRUE; + { + adp->save = TRUE; + changed = TRUE; + } keys = g_key_file_get_keys (adp->keyfile, account, &n, NULL); @@ -142,17 +146,30 @@ _set (McpAccountStorage *self, } else { - adp->save = TRUE; + gchar *old; val_str = mcp_account_manager_escape_variant_for_keyfile (am, val); - g_key_file_set_value (adp->keyfile, account, key, val_str); + + old = g_key_file_get_value (adp->keyfile, account, key, NULL); + + if (tp_strdiff (old, val_str)) + { + g_key_file_set_value (adp->keyfile, account, key, val_str); + adp->save = TRUE; + changed = TRUE; + } + g_free (val_str); + g_free (old); } - return TRUE; + if (changed) + return MCP_ACCOUNT_STORAGE_SET_RESULT_CHANGED; + else + return MCP_ACCOUNT_STORAGE_SET_RESULT_UNCHANGED; } -static gboolean +static McpAccountStorageSetResult _set_attribute (McpAccountStorage *self, McpAccountManager *am, const gchar *account, @@ -163,7 +180,7 @@ _set_attribute (McpAccountStorage *self, return _set (self, am, account, attribute, val, flags); } -static gboolean +static McpAccountStorageSetResult _set_parameter (McpAccountStorage *self, McpAccountManager *am, const gchar *account, -- 1.8.4.3