Call set_attribute, set_parameter to delete them

It really doesn't make a great deal of sense to use the same callback
to delete individual keys, and to delete accounts.

McdAccountManagerDefault already dealt with that case, but
the two test plugins didn't.

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

5 files changed, 63 insertions, 78 deletions

diff --git a/mission-control-plugins/account-storage.c b/mission-control-plugins/account-storage.c
index 5ad31b6c..53b00be0 100644
--- a/mission-control-plugins/account-storage.c
+++ b/mission-control-plugins/account-storage.c
@@ -504,7 +504,8 @@ mcp_account_storage_get (const McpAccountStorage *storage,
  * @am: an #McpAccountManager instance
  * @account: the unique name of the account
  * @attribute: the name of an attribute, e.g. "DisplayName"
- * @value: a value to associate with @attribute
+ * @value: (allow-none): a value to associate with @attribute,
+ *  or %NULL to delete
  * @flags: flags influencing how the attribute is to be stored
  *
  * Store an attribute.
@@ -547,7 +548,8 @@ mcp_account_storage_set_attribute (McpAccountStorage *storage,
  * @account: the unique name of the account
  * @parameter: the name of a parameter, e.g. "account" (note that there
  *  is no "param-" prefix here)
- * @value: a value to associate with @parameter
+ * @value: (allow-none): a value to associate with @parameter,
+ *  or %NULL to delete
  * @flags: flags influencing how the parameter is to be stored
  *
  * Store a parameter.
diff --git a/src/mcd-account-manager-default.c b/src/mcd-account-manager-default.c
index e37ea89b..07f22319 100644
--- a/src/mcd-account-manager-default.c
+++ b/src/mcd-account-manager-default.c
@@ -374,28 +374,7 @@ _delete (const McpAccountStorage *self,
     }
   else
     {
-      if (g_str_has_prefix (key, "param-"))
-        {
-          if (g_hash_table_remove (sa->parameters, key + 6))
-            amd->save = TRUE;
-
-          if (g_hash_table_remove (sa->untyped_parameters, key + 6))
-            amd->save = TRUE;
-        }
-      else
-        {
-          if (g_hash_table_remove (sa->attributes, key))
-            amd->save = TRUE;
-        }
-
-      /* if that was the last attribute or parameter, the account is gone
-       * too */
-      if (g_hash_table_size (sa->attributes) == 0 &&
-          g_hash_table_size (sa->untyped_parameters) == 0 &&
-          g_hash_table_size (sa->parameters) == 0)
-        {
-          sa->pending_deletion = TRUE;
-        }
+      g_assert_not_reached ();
     }
 
   return TRUE;
diff --git a/src/mcd-storage.c b/src/mcd-storage.c
index 37d6700f..dbc04c30 100644
--- a/src/mcd-storage.c
+++ b/src/mcd-storage.c
@@ -1379,38 +1379,29 @@ update_storage (McdStorage *self,
     GVariant *variant)
 {
   GList *store;
-  gboolean done = FALSE;
   gboolean parameter = g_str_has_prefix (key, "param-");
   McpAccountManager *ma = MCP_ACCOUNT_MANAGER (self);
 
-  /* we're deleting, which is unconditional, no need to check if anyone *
-   * claims this setting for themselves                                 */
-  if (variant == NULL)
-    done = TRUE;
-
   for (store = stores; store != NULL; store = g_list_next (store))
     {
       McpAccountStorage *plugin = store->data;
       const gchar *pn = mcp_account_storage_name (plugin);
 
-      if (done)         /* in particular, if variant == NULL */
-        {
-          DEBUG ("MCP:%s -> delete %s.%s", pn, account, key);
-          mcp_account_storage_delete (plugin, ma, account, key);
-        }
-      else if (!parameter &&
+      if (!parameter &&
           mcp_account_storage_set_attribute (plugin, ma, account, key, variant,
             MCP_ATTRIBUTE_FLAG_NONE))
         {
-          done = TRUE;
           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))
         {
-          done = TRUE;
           DEBUG ("MCP:%s -> store parameter %s.%s", pn, account, key);
+          /* set it to NULL in all lower-priority stores */
+          variant = NULL;
         }
     }
 }
diff --git a/tests/twisted/dbus-account-plugin.c b/tests/twisted/dbus-account-plugin.c
index 016417a1..3ea5df9d 100644
--- a/tests/twisted/dbus-account-plugin.c
+++ b/tests/twisted/dbus-account-plugin.c
@@ -940,28 +940,9 @@ test_dbus_account_plugin_delete (const McpAccountStorage *storage,
           "DeferringDelete", g_variant_new_parsed ("(%o,)", account->path),
           NULL);
     }
-  else if (g_str_has_prefix (key, "param-"))
-    {
-      g_hash_table_remove (account->parameters, key + 6);
-      g_hash_table_remove (account->untyped_parameters, key + 6);
-      g_hash_table_remove (account->parameter_flags, key + 6);
-      g_hash_table_add (account->uncommitted_parameters, g_strdup (key + 6));
-
-      g_dbus_connection_emit_signal (self->bus, NULL,
-          TEST_DBUS_ACCOUNT_PLUGIN_PATH, TEST_DBUS_ACCOUNT_PLUGIN_IFACE,
-          "DeferringDeleteParameter",
-          g_variant_new_parsed ("(%o, %s)", account->path, key + 6), NULL);
-    }
   else
     {
-      g_hash_table_remove (account->attributes, key);
-      g_hash_table_remove (account->attribute_flags, key);
-      g_hash_table_add (account->uncommitted_attributes, g_strdup (key));
-
-      g_dbus_connection_emit_signal (self->bus, NULL,
-          TEST_DBUS_ACCOUNT_PLUGIN_PATH, TEST_DBUS_ACCOUNT_PLUGIN_IFACE,
-          "DeferringDeleteAttribute",
-          g_variant_new_parsed ("(%o, %s)", account->path, key), NULL);
+      g_assert_not_reached ();
     }
 
   return TRUE;
@@ -1099,8 +1080,6 @@ test_dbus_account_plugin_set_attribute (McpAccountStorage *storage,
 
   g_return_val_if_fail (account_name != NULL, FALSE);
   g_return_val_if_fail (attribute != NULL, FALSE);
-  /* for deletions, MC would call delete() instead */
-  g_return_val_if_fail (value != NULL, FALSE);
 
   DEBUG ("%s of %s", attribute, account_name);
 
@@ -1108,6 +1087,20 @@ test_dbus_account_plugin_set_attribute (McpAccountStorage *storage,
       (account->flags & UNCOMMITTED_DELETION))
     return FALSE;
 
+  if (value == NULL)
+    {
+      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));
+
+      g_dbus_connection_emit_signal (self->bus, NULL,
+          TEST_DBUS_ACCOUNT_PLUGIN_PATH, TEST_DBUS_ACCOUNT_PLUGIN_IFACE,
+          "DeferringDeleteAttribute",
+          g_variant_new_parsed ("(%o, %s)", account->path, attribute), NULL);
+
+      return TRUE;
+    }
+
   g_hash_table_insert (account->attributes, g_strdup (attribute),
       g_variant_ref (value));
   g_hash_table_insert (account->attribute_flags, g_strdup (attribute),
@@ -1136,8 +1129,6 @@ test_dbus_account_plugin_set_parameter (McpAccountStorage *storage,
 
   g_return_val_if_fail (account_name != NULL, FALSE);
   g_return_val_if_fail (parameter != NULL, FALSE);
-  /* for deletions, MC would call delete() instead */
-  g_return_val_if_fail (value != NULL, FALSE);
 
   DEBUG ("%s of %s", parameter, account_name);
 
@@ -1145,6 +1136,21 @@ test_dbus_account_plugin_set_parameter (McpAccountStorage *storage,
       (account->flags & UNCOMMITTED_DELETION))
     return FALSE;
 
+  if (value == NULL)
+    {
+      g_hash_table_remove (account->parameters, parameter);
+      g_hash_table_remove (account->untyped_parameters, parameter);
+      g_hash_table_remove (account->parameter_flags, parameter);
+      g_hash_table_add (account->uncommitted_parameters, g_strdup (parameter));
+
+      g_dbus_connection_emit_signal (self->bus, NULL,
+          TEST_DBUS_ACCOUNT_PLUGIN_PATH, TEST_DBUS_ACCOUNT_PLUGIN_IFACE,
+          "DeferringDeleteParameter",
+          g_variant_new_parsed ("(%o, %s)", account->path, parameter), NULL);
+
+      return TRUE;
+    }
+
   g_hash_table_remove (account->untyped_parameters, parameter);
   g_hash_table_insert (account->parameters, g_strdup (parameter),
       g_variant_ref (value));
diff --git a/tests/twisted/mcp-account-diversion.c b/tests/twisted/mcp-account-diversion.c
index 44fd4e3a..9639fafb 100644
--- a/tests/twisted/mcp-account-diversion.c
+++ b/tests/twisted/mcp-account-diversion.c
@@ -125,11 +125,29 @@ _set (McpAccountStorage *self,
   if (g_str_has_prefix (account, DONT_DIVERT))
       return FALSE;
 
-  adp->save = TRUE;
+  if (val == NULL)
+    {
+      gsize n;
+      GStrv keys;
+
+      if (g_key_file_remove_key (adp->keyfile, account, key, NULL))
+        adp->save = TRUE;
+
+      keys = g_key_file_get_keys (adp->keyfile, account, &n, NULL);
 
-  val_str = mcp_account_manager_escape_variant_for_keyfile (am, val);
-  g_key_file_set_value (adp->keyfile, account, key, val_str);
-  g_free (val_str);
+      if (keys == NULL || n == 0)
+        g_key_file_remove_group (adp->keyfile, account, NULL);
+
+      g_strfreev (keys);
+    }
+  else
+    {
+      adp->save = TRUE;
+
+      val_str = mcp_account_manager_escape_variant_for_keyfile (am, val);
+      g_key_file_set_value (adp->keyfile, account, key, val_str);
+      g_free (val_str);
+    }
 
   return TRUE;
 }
@@ -220,18 +238,7 @@ _delete (const McpAccountStorage *self,
     }
   else
     {
-      gsize n;
-      GStrv keys;
-
-      if (g_key_file_remove_key (adp->keyfile, account, key, NULL))
-        adp->save = TRUE;
-
-      keys = g_key_file_get_keys (adp->keyfile, account, &n, NULL);
-
-      if (keys == NULL || n == 0)
-        g_key_file_remove_group (adp->keyfile, account, NULL);
-
-      g_strfreev (keys);
+      g_assert_not_reached ();
     }
 
   return TRUE;