From 5b38bc5ad5181bb4900c1da898b2e4fcdcec1757 Mon Sep 17 00:00:00 2001
From: Ryan Lortie <desrt@desrt.ca>
Date: Sat, 21 Aug 2010 17:35:32 -0400
Subject: [PATCH] Simplify/fix state logic in GAction, test it.

---
 gio/gaction.c       | 55 ++++++++++++++++++++++++++++++++++-------------------
 gio/tests/actions.c | 35 ++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 20 deletions(-)

diff --git a/gio/gaction.c b/gio/gaction.c
index a01b2dd..a7ed5f4 100644
--- a/gio/gaction.c
+++ b/gio/gaction.c
@@ -67,9 +67,8 @@ struct _GActionPrivate
 {
   gchar        *name;
   GVariantType *parameter_type;
-  gboolean      enabled;
-
-  GVariantType *state_type;
+  guint         enabled : 1;
+  guint         state_set : 1;
   GVariant     *state;
 };
 
@@ -137,14 +136,29 @@ g_action_set_property (GObject *object, guint prop_id,
       g_action_set_enabled (action, g_value_get_boolean (value));
       break;
 
-    case PROP_STATE_TYPE:
-      g_assert (action->priv->state_type == NULL);
-      action->priv->state_type = g_value_dup_boxed (value);
-      break;
-
     case PROP_STATE:
-      if (g_value_get_variant (value))
+      /* PROP_STATE is marked as G_PARAM_CONSTRUCT so we always get a
+       * call during object construction, even if it is NULL.  We treat
+       * that first call differently, for a number of reasons.
+       *
+       * First, we don't want the value to be rejected by the
+       * possibly-overridden .set_state() function.  Second, we don't
+       * want to be tripped by the assertions in g_action_set_state()
+       * that would enforce the catch22 that we only provide a value of
+       * the same type as the existing value (when there is not yet an
+       * existing value).
+       */
+      if (action->priv->state_set)
         g_action_set_state (action, g_value_get_variant (value));
+
+      else /* this is the special case */
+        {
+          /* only do it the first time. */
+          action->priv->state_set = TRUE;
+
+          /* blindly set it. */
+          action->priv->state = g_value_dup_variant (value);
+        }
       break;
 
     default:
@@ -193,8 +207,6 @@ g_action_finalize (GObject *object)
   g_free (action->priv->name);
   if (action->priv->parameter_type)
     g_variant_type_free (action->priv->parameter_type);
-  if (action->priv->state_type)
-    g_variant_type_free (action->priv->state_type);
   if (action->priv->state)
     g_variant_unref (action->priv->state);
 
@@ -310,8 +322,7 @@ g_action_class_init (GActionClass *class)
                                                        P_("State Type"),
                                                        P_("The type of the state kept by the action"),
                                                        G_TYPE_VARIANT_TYPE,
-                                                       G_PARAM_READWRITE |
-                                                       G_PARAM_CONSTRUCT_ONLY |
+                                                       G_PARAM_READABLE |
                                                        G_PARAM_STATIC_STRINGS));
 
   /**
@@ -354,9 +365,13 @@ void
 g_action_set_state (GAction  *action,
                     GVariant *value)
 {
+  const GVariantType *state_type;
+
   g_return_if_fail (G_IS_ACTION (action));
-  g_return_if_fail ((action->priv->state_type == NULL && value == NULL) ||
-                    g_variant_is_of_type (value, action->priv->state_type));
+  g_return_if_fail (value != NULL);
+  state_type = g_action_get_state_type (action);
+  g_return_if_fail (state_type != NULL);
+  g_return_if_fail (g_variant_is_of_type (value, state_type));
 
   g_variant_ref_sink (value);
 
@@ -377,10 +392,7 @@ g_action_set_state (GAction  *action,
  * action is stateful then the type of the return value is the type
  * given by g_action_get_state_type().
  *
- * The return value (if non-%NULL) should be freed with
- * g_variant_unref() when it is no longer required.
- *
- * Returns: (allow-none): the current state of the action
+ * Returns: (allow-none) (transfer none): the current state of the action
  *
  * Since: 2.26
  **/
@@ -461,7 +473,10 @@ g_action_get_state_type (GAction *action)
 {
   g_return_val_if_fail (G_IS_ACTION (action), NULL);
 
-  return action->priv->state_type;
+  if (action->priv->state != NULL)
+    return g_variant_get_type (action->priv->state);
+  else
+    return NULL;
 }
 
 /**
diff --git a/gio/tests/actions.c b/gio/tests/actions.c
index 9d24da0..d6baf63 100644
--- a/gio/tests/actions.c
+++ b/gio/tests/actions.c
@@ -96,6 +96,40 @@ test_simple_group (void)
   g_assert (!a.did_run);
 }
 
+static void
+test_stateful (void)
+{
+  GAction *action;
+
+  action = g_action_new_stateful ("foo", NULL, g_variant_new_string ("hihi"));
+  g_assert (g_variant_type_equal (g_action_get_state_type (action),
+                                  G_VARIANT_TYPE_STRING));
+  g_assert_cmpstr (g_variant_get_string (g_action_get_state (action), NULL),
+                   ==, "hihi");
+
+  if (g_test_trap_fork (0, G_TEST_TRAP_SILENCE_STDERR))
+    {
+      g_action_set_state (action, g_variant_new_int32 (123));
+      exit (0);
+    }
+  g_test_trap_assert_failed ();
+
+  g_action_set_state (action, g_variant_new_string ("hello"));
+  g_assert_cmpstr (g_variant_get_string (g_action_get_state (action), NULL),
+                   ==, "hello");
+
+  g_object_unref (action);
+
+  action = g_action_new ("foo", NULL);
+  if (g_test_trap_fork (0, G_TEST_TRAP_SILENCE_STDERR))
+    {
+      g_action_set_state (action, g_variant_new_int32 (123));
+      exit (0);
+    }
+  g_test_trap_assert_failed ();
+  g_object_unref (action);
+}
+
 int
 main (int argc, char **argv)
 {
@@ -104,6 +138,7 @@ main (int argc, char **argv)
 
   g_test_add_func ("/actions/basic", test_basic);
   g_test_add_func ("/actions/simplegroup", test_simple_group);
+  g_test_add_func ("/actions/stateful", test_stateful);
 
   return g_test_run ();
 }
-- 
2.7.4