Bug 32153 - Account Storage commit() virtual method not optional (as claimed in docs)
Summary: Account Storage commit() virtual method not optional (as claimed in docs)
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: mission-control (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/wj...
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2010-12-06 15:58 UTC by Travis Reitter
Modified: 2011-01-19 10:00 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Travis Reitter 2010-12-06 15:58:21 UTC
The documentation for mcp_account_storage_commit() claims that the commit() virtual function doesn't need to be implemented if commit_one() is.

However, the code for mcp_account_storage_commit() doesn't use commit_one() at all and returns an "error" response if commit() is not implemented.
Comment 1 Jonny Lamb 2010-12-07 00:18:01 UTC
m5-account-plugins != mission-control
Comment 3 Will Thompson 2011-01-05 09:48:15 UTC
here's a branch that fixes this, as well as fixing a number of other storage buglets and improving the test suite a bit.
Comment 4 Jonny Lamb 2011-01-06 01:21:05 UTC
-    old_string = mcd_storage_dup_string (storage, name, key);
     new_string = g_value_get_string (value);
 
-    if (!tp_strdiff (old_string, new_string))
-    {
-        g_free (old_string);
-        return SET_RESULT_UNCHANGED;
+    if (*new_string == '\0') {
+        new_string = NULL;
     }

What if the value GValue hasn't had a string set to it yet? g_value_get_string will return NULL and *NULL isn't so cool.
Comment 5 Will Thompson 2011-01-06 03:33:26 UTC
(In reply to comment #4)
> -    old_string = mcd_storage_dup_string (storage, name, key);
>      new_string = g_value_get_string (value);
> 
> -    if (!tp_strdiff (old_string, new_string))
> -    {
> -        g_free (old_string);
> -        return SET_RESULT_UNCHANGED;
> +    if (*new_string == '\0') {
> +        new_string = NULL;
>      }
> 
> What if the value GValue hasn't had a string set to it yet? g_value_get_string
> will return NULL and *NULL isn't so cool.

As discussed IRL this is actually safe because dbus-glib never gives you GValues containing NULL strings. But I've amended the patch to use tp_str_empty; and merged it. Cheers! This fix will be in 5.7.2
Comment 6 Vivek Dasmohapatra 2011-01-19 10:00:35 UTC
Released in 5.7.2


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.