Created attachment 248001 [details] [review]
add GPropertyAction

Add a new type of GAction that represents the value of a property on an
object.  As an example, this might be used on the "visible-child-name"
property of a GtkStack.

Review of attachment 248001 [details] [review]:

::: gio/gpropertyaction.c
@@ +403,3 @@
+   *
+   * The name of the action.  This is mostly meaningful for identifying
+   * the action once it has been added to a #GPropertyActionGroup.

Whats a GPropertyActionGroup ?

@@ +533,3 @@
+*
+ * Returns: a new #GPropertyAction
+ *

Should this say that the property action takes a strong reference on the object ?

Review of attachment 248001 [details] [review]:

::: gio/gpropertyaction.c
@@ +403,3 @@
+   *
+   * The name of the action.  This is mostly meaningful for identifying
+   * the action once it has been added to a #GPropertyActionGroup.

It is a sed error :)

@@ +533,3 @@
+*
+ * Returns: a new #GPropertyAction
+ *

Yes.  Probably should.

::: gio/gpropertyaction.h
@@ +37,3 @@
+                                                             G_TYPE_PROPERTY_ACTION))
+
+GLIB_AVAILABLE_IN_ALL

obvious mistake here, as well...

Review of attachment 248001 [details] [review]:

::: gio/gpropertyaction.c
@@ +157,3 @@
+g_property_action_get_enabled (GAction *action)
+{
+  return TRUE;

Actions could be disabled when the property is not writable.

@@ +208,3 @@
+      gboolean value;
+
+      g_return_if_fail (paction->pspec->value_type == G_TYPE_BOOLEAN && parameter == NULL);

That first condition is always true. Did you put it there so that error messages are more understandable?

@@ +273,3 @@
+  gchar *detailed;
+
+  pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (paction->object), property_name);

You are printing a critical when this returns NULL, but no other function checks it for NULL. Are you okay with it blowing up? You could check whether the property exists in g_property_action_new().

::: gio/gpropertyaction.h
@@ +1,2 @@
+/*
+ * Copyright © 2010 Codethink Limited

You wrote that three years ago? :P

Review of attachment 248001 [details] [review]:

::: gio/gpropertyaction.c
@@ +157,3 @@
+g_property_action_get_enabled (GAction *action)
+{
+  return TRUE;

I want to wait for someone to request this feature.  I don't think that a real usecase exists for it.

@@ +208,3 @@
+      gboolean value;
+
+      g_return_if_fail (paction->pspec->value_type == G_TYPE_BOOLEAN && parameter == NULL);

Yes.

@@ +273,3 @@
+  gchar *detailed;
+
+  pspec = g_object_class_find_property (G_OBJECT_GET_CLASS (paction->object), property_name);

I just want to get a critical onto the screen at the point that we first notice the problem.  Things may well crash later, but at least they will know why (and be able to easily get a backtrace from the source of the problem).

I don't really feel like adding insane levels of extra checking everywhere to detect and re-detect programmer error...

Created attachment 248942 [details] [review]
add GPropertyAction

Add a new type of GAction that represents the value of a property on an
object.  As an example, this might be used on the "visible-child-name"
property of a GtkStack.

Attachment 248942 [details] pushed as f77e121 - add GPropertyAction