[telepathy-mission-control/master] fd.o #21544: change signature of mcd_setprop so it can raise an error
Simon McVittie
simon.mcvittie at collabora.co.uk
Tue May 5 08:09:37 PDT 2009
The new implementation of set_avatar() looks suspicious: it changes the
interpretation of _mcd_account_set_avatar's boolean return value from
"changed" to "no error". However, _mcd_account_set_avatar either returns
FALSE with a GError, or TRUE, in all cases, so this is actually OK.
---
src/mcd-account-compat.c | 11 +++--
src/mcd-account-conditions.c | 5 +-
src/mcd-account.c | 118 +++++++++++++++++++++++------------------
src/mcd-dbusprop.c | 3 +-
src/mcd-dbusprop.h | 4 +-
test/twisted/test-account.py | 88 +++++++++++++++-----------------
6 files changed, 120 insertions(+), 109 deletions(-)
diff --git a/src/mcd-account-compat.c b/src/mcd-account-compat.c
index 03d2fe3..5581703 100644
--- a/src/mcd-account-compat.c
+++ b/src/mcd-account-compat.c
@@ -52,9 +52,9 @@ typedef struct
gchar *requestor_client_id;
} McdAccountCompatReq;
-static void
+static gboolean
set_profile (TpSvcDBusProperties *self, const gchar *name,
- const GValue *value)
+ const GValue *value, GError **error)
{
McdAccount *account = MCD_ACCOUNT (self);
const gchar *string, *unique_name;
@@ -74,6 +74,8 @@ set_profile (TpSvcDBusProperties *self, const gchar *name,
_mcd_account_write_conf (account);
g_signal_emit (account, _mcd_account_signal_profile_set, 0);
+
+ return TRUE;
}
static void
@@ -103,9 +105,9 @@ get_avatar_file (TpSvcDBusProperties *self, const gchar *name, GValue *value)
g_value_take_string (value, string);
}
-static void
+static gboolean
set_secondary_vcard_fields (TpSvcDBusProperties *self, const gchar *name,
- const GValue *value)
+ const GValue *value, GError **error)
{
McdAccount *account = MCD_ACCOUNT (self);
const gchar *unique_name, **fields, **field;
@@ -128,6 +130,7 @@ set_secondary_vcard_fields (TpSvcDBusProperties *self, const gchar *name,
name, NULL);
}
_mcd_account_write_conf (account);
+ return TRUE;
}
static void
diff --git a/src/mcd-account-conditions.c b/src/mcd-account-conditions.c
index 5a3905b..d18d99d 100644
--- a/src/mcd-account-conditions.c
+++ b/src/mcd-account-conditions.c
@@ -54,9 +54,9 @@ store_condition (gpointer key, gpointer value, gpointer userdata)
g_key_file_set_string (keyfile, unique_name, condition_key, condition);
}
-static void
+static gboolean
set_condition (TpSvcDBusProperties *self, const gchar *name,
- const GValue *value)
+ const GValue *value, GError **error)
{
McdAccount *account = MCD_ACCOUNT (self);
const gchar *unique_name;
@@ -80,6 +80,7 @@ set_condition (TpSvcDBusProperties *self, const gchar *name,
g_hash_table_foreach (conditions, store_condition, account);
_mcd_account_write_conf (account);
+ return TRUE;
}
static void
diff --git a/src/mcd-account.c b/src/mcd-account.c
index 45bd321..2d55a9d 100644
--- a/src/mcd-account.c
+++ b/src/mcd-account.c
@@ -685,15 +685,16 @@ mcd_account_get_string_val (McdAccount *account, const gchar *key,
g_value_take_string (value, string);
}
-static void
+static gboolean
set_display_name (TpSvcDBusProperties *self, const gchar *name,
- const GValue *value)
+ const GValue *value, GError **error)
{
McdAccount *account = MCD_ACCOUNT (self);
McdAccountPrivate *priv = account->priv;
DEBUG ("called for %s", priv->unique_name);
- mcd_account_set_string_val (account, name, value, NULL);
+ return (mcd_account_set_string_val (account, name, value, error)
+ != SET_RESULT_ERROR);
}
static void
@@ -704,14 +705,16 @@ get_display_name (TpSvcDBusProperties *self, const gchar *name, GValue *value)
mcd_account_get_string_val (account, name, value);
}
-static void
-set_icon (TpSvcDBusProperties *self, const gchar *name, const GValue *value)
+static gboolean
+set_icon (TpSvcDBusProperties *self, const gchar *name, const GValue *value,
+ GError **error)
{
McdAccount *account = MCD_ACCOUNT (self);
McdAccountPrivate *priv = account->priv;
DEBUG ("called for %s", priv->unique_name);
- mcd_account_set_string_val (account, name, value, NULL);
+ return (mcd_account_set_string_val (account, name, value, error)
+ != SET_RESULT_ERROR);
}
static void
@@ -743,8 +746,9 @@ get_has_been_online (TpSvcDBusProperties *self, const gchar *name,
g_value_set_boolean (value, priv->has_been_online);
}
-static void
-set_enabled (TpSvcDBusProperties *self, const gchar *name, const GValue *value)
+static gboolean
+set_enabled (TpSvcDBusProperties *self, const gchar *name, const GValue *value,
+ GError **error)
{
McdAccount *account = MCD_ACCOUNT (self);
McdAccountPrivate *priv = account->priv;
@@ -755,9 +759,10 @@ set_enabled (TpSvcDBusProperties *self, const gchar *name, const GValue *value)
/* We can't raise an error in this API :-( */
if (!G_VALUE_HOLDS_BOOLEAN (value))
{
- g_warning ("Expected boolean for Enabled, but got %s",
- G_VALUE_TYPE_NAME (value));
- return;
+ g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
+ "Expected boolean for Enabled, but got %s",
+ G_VALUE_TYPE_NAME (value));
+ return FALSE;
}
enabled = g_value_get_boolean (value);
@@ -775,6 +780,8 @@ set_enabled (TpSvcDBusProperties *self, const gchar *name, const GValue *value)
mcd_account_manager_write_conf (priv->account_manager);
mcd_account_changed_property (account, name, value);
}
+
+ return TRUE;
}
static void
@@ -787,19 +794,24 @@ get_enabled (TpSvcDBusProperties *self, const gchar *name, GValue *value)
g_value_set_boolean (value, priv->enabled);
}
-static void
-set_nickname (TpSvcDBusProperties *self, const gchar *name, const GValue *value)
+static gboolean
+set_nickname (TpSvcDBusProperties *self, const gchar *name,
+ const GValue *value, GError **error)
{
McdAccount *account = MCD_ACCOUNT (self);
McdAccountPrivate *priv = account->priv;
+ SetResult ret;
DEBUG ("called for %s", priv->unique_name);
- if (mcd_account_set_string_val (account, name, value, NULL)
- == SET_RESULT_CHANGED)
+ ret = mcd_account_set_string_val (account, name, value, error);
+
+ if (ret == SET_RESULT_CHANGED)
{
g_signal_emit (account, _mcd_account_signals[ALIAS_CHANGED], 0,
g_value_get_string (value));
}
+
+ return (ret != SET_RESULT_ERROR);
}
static void
@@ -810,41 +822,37 @@ get_nickname (TpSvcDBusProperties *self, const gchar *name, GValue *value)
mcd_account_get_string_val (account, name, value);
}
-static void
-set_avatar (TpSvcDBusProperties *self, const gchar *name, const GValue *value)
+static gboolean
+set_avatar (TpSvcDBusProperties *self, const gchar *name, const GValue *value,
+ GError **error)
{
McdAccount *account = MCD_ACCOUNT (self);
McdAccountPrivate *priv = account->priv;
const gchar *mime_type;
const GArray *avatar;
GValueArray *va;
- GError *error = NULL;
- gboolean changed;
DEBUG ("called for %s", priv->unique_name);
- /* mcd_dbusprop can't return an error, so just spam to stderr and assume
- * no change. This is saddening - we should be able to raise error. */
if (!G_VALUE_HOLDS (value, MC_STRUCT_TYPE_AVATAR))
{
- g_warning ("Unexpected type for Avatar: wanted (ay,s), "
- "got %s", G_VALUE_TYPE_NAME (value));
- return;
+ g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
+ "Unexpected type for Avatar: wanted (ay,s), got %s",
+ G_VALUE_TYPE_NAME (value));
+ return FALSE;
}
va = g_value_get_boxed (value);
avatar = g_value_get_boxed (va->values);
mime_type = g_value_get_string (va->values + 1);
- changed = _mcd_account_set_avatar (account, avatar, mime_type, NULL,
- &error);
- if (error)
+
+ if (!_mcd_account_set_avatar (account, avatar, mime_type, NULL, error))
{
- g_warning ("%s: failed: %s", G_STRFUNC, error->message);
- g_error_free (error);
- return;
+ return FALSE;
}
- if (changed)
- mc_svc_account_interface_avatar_emit_avatar_changed (account);
+
+ mc_svc_account_interface_avatar_emit_avatar_changed (account);
+ return TRUE;
}
static void
@@ -883,9 +891,9 @@ get_parameters (TpSvcDBusProperties *self, const gchar *name, GValue *value)
g_value_take_boxed (value, parameters);
}
-static void
+static gboolean
set_automatic_presence (TpSvcDBusProperties *self,
- const gchar *name, const GValue *value)
+ const gchar *name, const GValue *value, GError **error)
{
McdAccount *account = MCD_ACCOUNT (self);
McdAccountPrivate *priv = account->priv;
@@ -896,13 +904,12 @@ set_automatic_presence (TpSvcDBusProperties *self,
DEBUG ("called for %s", priv->unique_name);
- /* mcd_dbusprop can't return an error, so just spam to stderr and assume
- * no change. This is saddening - we should be able to raise error. */
if (!G_VALUE_HOLDS (value, TP_STRUCT_TYPE_SIMPLE_PRESENCE))
{
- g_warning ("Unexpected type for RequestedPresence: wanted (u,s,s), "
- "got %s", G_VALUE_TYPE_NAME (value));
- return;
+ g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
+ "Unexpected type for RequestedPresence: wanted (u,s,s), "
+ "got %s", G_VALUE_TYPE_NAME (value));
+ return FALSE;
}
va = g_value_get_boxed (value);
@@ -952,6 +959,8 @@ set_automatic_presence (TpSvcDBusProperties *self,
mcd_account_manager_write_conf (priv->account_manager);
mcd_account_changed_property (account, name, value);
}
+
+ return TRUE;
}
static void
@@ -978,9 +987,10 @@ get_automatic_presence (TpSvcDBusProperties *self,
g_value_set_static_string (va->values + 2, message);
}
-static void
+static gboolean
set_connect_automatically (TpSvcDBusProperties *self,
- const gchar *name, const GValue *value)
+ const gchar *name, const GValue *value,
+ GError **error)
{
McdAccount *account = MCD_ACCOUNT (self);
McdAccountPrivate *priv = account->priv;
@@ -988,12 +998,12 @@ set_connect_automatically (TpSvcDBusProperties *self,
DEBUG ("called for %s", priv->unique_name);
- /* We can't raise an error in this API :-( */
if (!G_VALUE_HOLDS_BOOLEAN (value))
{
- g_warning ("Expected boolean for ConnectAutomatically, but got %s",
- G_VALUE_TYPE_NAME (value));
- return;
+ g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
+ "Expected boolean for ConnectAutomatically, but got %s",
+ G_VALUE_TYPE_NAME (value));
+ return FALSE;
}
connect_automatically = g_value_get_boolean (value);
@@ -1006,6 +1016,8 @@ set_connect_automatically (TpSvcDBusProperties *self,
mcd_account_manager_write_conf (priv->account_manager);
mcd_account_changed_property (account, name, value);
}
+
+ return TRUE;
}
static void
@@ -1079,9 +1091,10 @@ get_current_presence (TpSvcDBusProperties *self, const gchar *name,
g_value_set_static_string (va->values + 2, message);
}
-static void
+static gboolean
set_requested_presence (TpSvcDBusProperties *self,
- const gchar *name, const GValue *value)
+ const gchar *name, const GValue *value,
+ GError **error)
{
McdAccount *account = MCD_ACCOUNT (self);
McdAccountPrivate *priv = account->priv;
@@ -1091,13 +1104,12 @@ set_requested_presence (TpSvcDBusProperties *self,
DEBUG ("called for %s", priv->unique_name);
- /* mcd_dbusprop can't return an error, so just spam to stderr and assume
- * no change. This is saddening - we should be able to raise error. */
if (!G_VALUE_HOLDS (value, TP_STRUCT_TYPE_SIMPLE_PRESENCE))
{
- g_warning ("Unexpected type for RequestedPresence: wanted (u,s,s), "
- "got %s", G_VALUE_TYPE_NAME (value));
- return;
+ g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
+ "Unexpected type for RequestedPresence: wanted (u,s,s), "
+ "got %s", G_VALUE_TYPE_NAME (value));
+ return FALSE;
}
va = g_value_get_boxed (value);
@@ -1111,6 +1123,8 @@ set_requested_presence (TpSvcDBusProperties *self,
{
mcd_account_changed_property (account, name, value);
}
+
+ return TRUE;
}
static void
diff --git a/src/mcd-dbusprop.c b/src/mcd-dbusprop.c
index d0afb6e..be76512 100644
--- a/src/mcd-dbusprop.c
+++ b/src/mcd-dbusprop.c
@@ -106,8 +106,7 @@ mcd_dbusprop_set_property (TpSvcDBusProperties *self,
}
/* we pass property->name, because we know it's a static value and there
* will be no need to care about its lifetime */
- property->setprop (self, property->name, value);
- return TRUE;
+ return property->setprop (self, property->name, value, error);
}
void
diff --git a/src/mcd-dbusprop.h b/src/mcd-dbusprop.h
index a109454..c953ebe 100644
--- a/src/mcd-dbusprop.h
+++ b/src/mcd-dbusprop.h
@@ -30,8 +30,8 @@
G_BEGIN_DECLS
-typedef void (*mcd_setprop) (TpSvcDBusProperties *self, const gchar *name,
- const GValue *value);
+typedef gboolean (*mcd_setprop) (TpSvcDBusProperties *self, const gchar *name,
+ const GValue *value, GError **error);
typedef void (*mcd_getprop) (TpSvcDBusProperties *self, const gchar *name,
GValue *value);
typedef void (*McdInterfaceInit) (TpSvcDBusProperties *self);
diff --git a/test/twisted/test-account.py b/test/twisted/test-account.py
index 27fc0a8..4991d22 100644
--- a/test/twisted/test-account.py
+++ b/test/twisted/test-account.py
@@ -129,53 +129,47 @@ def test(q, bus, mc):
# but is a no-op, although in future it should change to raising an
# exception
- try:
- account_props.Set(cs.ACCOUNT, 'DisplayName',
- dbus.Struct(('wrongly typed',), signature='s'))
- except dbus.DBusException:
- pass
-
- try:
- account_props.Set(cs.ACCOUNT, 'Icon',
- dbus.Struct(('wrongly typed',), signature='s'))
- except dbus.DBusException:
- pass
-
- try:
- account_props.Set(cs.ACCOUNT, 'Enabled',
- dbus.Struct(('wrongly typed',), signature='s'))
- except dbus.DBusException:
- pass
-
- try:
- account_props.Set(cs.ACCOUNT, 'Nickname',
- dbus.Struct(('wrongly typed',), signature='s'))
- except dbus.DBusException:
- pass
-
- try:
- account_props.Set(cs.ACCOUNT, 'AutomaticPresence',
- dbus.Struct(('wrongly typed',), signature='s'))
- except dbus.DBusException:
- pass
-
- try:
- account_props.Set(cs.ACCOUNT, 'ConnectAutomatically',
- dbus.Struct(('wrongly typed',), signature='s'))
- except dbus.DBusException:
- pass
-
- try:
- account_props.Set(cs.ACCOUNT, 'RequestedPresence',
- dbus.Struct(('wrongly typed',), signature='s'))
- except dbus.DBusException:
- pass
-
- try:
- account_props.Set(cs.ACCOUNT_IFACE_AVATAR, 'Avatar',
- dbus.Struct(('wrongly typed',), signature='s'))
- except dbus.DBusException:
- pass
+ # this variable's D-Bus type must differ from the types of all known
+ # properties
+ badly_typed = dbus.Struct(('wrongly typed',), signature='s')
+
+ for p in ('DisplayName', 'Icon', 'Enabled', 'Nickname',
+ 'AutomaticPresence', 'ConnectAutomatically', 'RequestedPresence'):
+ try:
+ account_props.Set(cs.ACCOUNT, p, badly_typed)
+ except dbus.DBusException, e:
+ assert e.get_dbus_name() == cs.INVALID_ARGUMENT, \
+ (p, e.get_dbus_name())
+ else:
+ raise AssertionError('Setting %s with wrong type should fail' % p)
+
+ for p in ('Avatar',):
+ try:
+ account_props.Set(cs.ACCOUNT_IFACE_AVATAR, p, badly_typed)
+ except dbus.DBusException, e:
+ assert e.get_dbus_name() == cs.INVALID_ARGUMENT, \
+ (p, e.get_dbus_name())
+ else:
+ raise AssertionError('Setting %s with wrong type should fail' % p)
+
+ for p in ('Profile', 'SecondaryVCardFields'):
+ try:
+ account_props.Set(cs.ACCOUNT_IFACE_NOKIA_COMPAT, p, badly_typed)
+ except dbus.DBusException, e:
+ assert e.get_dbus_name() == cs.INVALID_ARGUMENT, \
+ (p, e.get_dbus_name())
+ else:
+ raise AssertionError('Setting %s with wrong type should fail' % p)
+
+ for p in ('Conditions',):
+ try:
+ account_props.Set(cs.ACCOUNT_IFACE_NOKIA_CONDITIONS, p,
+ badly_typed)
+ except dbus.DBusException, e:
+ assert e.get_dbus_name() == cs.INVALID_ARGUMENT, \
+ (p, e.get_dbus_name())
+ else:
+ raise AssertionError('Setting %s with wrong type should fail' % p)
# Make sure MC hasn't crashed yet, and make sure some properties are what
# we expect them to be
--
1.5.6.5
More information about the telepathy-commits
mailing list