[Workman-devel] [PATCH 4/4] Add attributes property to WorkmanObject
Josh Poimboeuf
jpoimboe at redhat.com
Fri Mar 1 20:18:15 UTC 2013
On 03/01/2013 08:22 AM, Daniel P. Berrange wrote:
> On Fri, Mar 01, 2013 at 08:19:49AM -0600, Josh Poimboeuf wrote:
>> On 03/01/2013 08:11 AM, Daniel P. Berrange wrote:
>>> On Fri, Mar 01, 2013 at 08:08:47AM -0600, Josh Poimboeuf wrote:
>>>> On 03/01/2013 03:52 AM, Daniel P. Berrange wrote:
>>>>> On Fri, Mar 01, 2013 at 09:51:20AM +0000, Daniel P. Berrange wrote:
>>>>>> On Thu, Feb 28, 2013 at 03:51:49PM -0600, Josh Poimboeuf wrote:
>>>>>>> Good finds. I made all your suggested changes:
>>>>>>>
>>>>>>> ---8<---
>>>>>>>
>>>>>>> Subject: [PATCH 2/2] Add attributes property to WorkmanObject
>>>>>>>
>>>>>>> - Add the "attributes" property to WorkmanObject.
>>>>>>> - Remove the attribute-related overridable function pointers from the
>>>>>>> class struct, since the subclasses won't be overriding them.
>>>>>>> - Change the WorkmanState enum to use bit flags to make it easier to
>>>>>>> deal with WORKMAN_STATE_ALL.
>>>>>>> ---
>>>>>>> workman/workman-object.c | 128 ++++++++++++++++++++++++++++++++++++++---------
>>>>>>> workman/workman-object.h | 16 ------
>>>>>>> workman/workman-state.h | 7 +--
>>>>>>> 3 files changed, 107 insertions(+), 44 deletions(-)
>>>>>>>
>>>>>>> diff --git a/workman/workman-object.c b/workman/workman-object.c
>>>>>>> index 45324bf..27606de 100644
>>>>>>> --- a/workman/workman-object.c
>>>>>>> +++ b/workman/workman-object.c
>>>>>>> @@ -32,6 +32,7 @@ G_DEFINE_ABSTRACT_TYPE(WorkmanObject, workman_object, G_TYPE_OBJECT);
>>>>>>> struct _WorkmanObjectPrivate {
>>>>>>> gchar *name;
>>>>>>> WorkmanState state;
>>>>>>> + GList *attributes;
>>>>>>> };
>>>>>>>
>>>>>>>
>>>>>>> @@ -39,8 +40,50 @@ enum {
>>>>>>> PROP_0,
>>>>>>> PROP_NAME,
>>>>>>> PROP_STATE,
>>>>>>> + PROP_ATTRIBUTES,
>>>>>>> };
>>>>>>>
>>>>>>> +static void
>>>>>>> +workman_object_free_attributes(WorkmanObject *self)
>>>>>>> +{
>>>>>>> + WorkmanObjectPrivate *priv = self->priv;
>>>>>>> +
>>>>>>> + if (priv->attributes) {
>>>>>>> + g_list_foreach(priv->attributes, (GFunc)g_object_unref, NULL);
>>>>>>> + g_list_free(priv->attributes);
>>>>>>> + }
>>>>>>> +}
>>>>>>> +
>>>>>>> +
>>>>>>> +static GList *
>>>>>>> +workman_object_copy_attributes(GList *attributes,
>>>>>>> + WorkmanState state)
>>>>>>> +{
>>>>>>> + GList *new_attrs, *a;
>>>>>>> +
>>>>>>> + new_attrs = NULL;
>>>>>>> +
>>>>>>> + for (a = attributes; a; a = a->next) {
>>>>>>> + WorkmanAttribute *attr = a->data;
>>>>>>> + if (workman_attribute_get_state(attr) & state) {
>>>>>>> + g_object_ref(attr);
>>>>>>> + new_attrs = g_list_append(new_attrs, attr);
>>>>>>> + }
>>>>>>> + }
>>>>>>> +
>>>>>>> + return new_attrs;
>>>>>>> +}
>>>>>>> +
>>>>>>> +
>>>>>>> +static void
>>>>>>> +workman_object_set_attributes(WorkmanObject *self,
>>>>>>> + GList *attributes)
>>>>>>> +{
>>>>>>> + workman_object_free_attributes(self);
>>>>>>> + self->priv->attributes = workman_object_copy_attributes(attributes,
>>>>>>> + WORKMAN_STATE_ALL);
>>>>>>> +}
>>>>>>> +
>>>>>>>
>>>>>>> static void
>>>>>>> workman_object_get_property(GObject *object,
>>>>>>> @@ -56,7 +99,13 @@ workman_object_get_property(GObject *object,
>>>>>>> g_value_set_string(value, self->priv->name);
>>>>>>> break;
>>>>>>> case PROP_STATE:
>>>>>>> - g_value_set_enum(value, self->priv->state);
>>>>>>> + g_value_set_flags(value, self->priv->state);
>>>>>>> + break;
>>>>>>> + case PROP_ATTRIBUTES:
>>>>>>> + attributes = workman_object_get_attributes(self,
>>>>>>> + WORKMAN_STATE_ALL,
>>>>>>> + NULL);
>>>>>>> + g_value_set_pointer(value, attributes);
>>>>>>> break;
>>>>>>> default:
>>>>>>> G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>>>>>>> @@ -78,7 +127,10 @@ workman_object_set_property(GObject *object,
>>>>>>> self->priv->name = g_value_dup_string(value);
>>>>>>> break;
>>>>>>> case PROP_STATE:
>>>>>>> - self->priv->state = g_value_get_enum(value);
>>>>>>> + self->priv->state = g_value_get_flags(value);
>>>>>>> + break;
>>>>>>> + case PROP_ATTRIBUTES:
>>>>>>> + workman_object_set_attributes(self, g_value_get_pointer(value));
>>>>>>> break;
>>>>>>> default:
>>>>>>> G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
>>>>>>> @@ -87,10 +139,21 @@ workman_object_set_property(GObject *object,
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> -void workman_object_finalize(GObject *object)
>>>>>>> +static void
>>>>>>> +workman_object_dispose(GObject *object)
>>>>>>> +{
>>>>>>> + WorkmanObject *self = WORKMAN_OBJECT(object);
>>>>>>> +
>>>>>>> + workman_object_free_attributes(self);
>>>>>>> +
>>>>>>> + G_OBJECT_CLASS(workman_object_parent_class)->dispose(object);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void
>>>>>>> +workman_object_finalize(GObject *object)
>>>>>>> {
>>>>>>> - WorkmanObject *attr = WORKMAN_OBJECT(object);
>>>>>>> - WorkmanObjectPrivate *priv = attr->priv;
>>>>>>> + WorkmanObject *obj = WORKMAN_OBJECT(object);
>>>>>>> + WorkmanObjectPrivate *priv = obj->priv;
>>>>>>>
>>>>>>> g_free(priv->name);
>>>>>>>
>>>>>>> @@ -104,6 +167,7 @@ workman_object_class_init(WorkmanObjectClass *klass)
>>>>>>> GObjectClass *g_klass = G_OBJECT_CLASS(klass);
>>>>>>> GParamSpec *pspec;
>>>>>>>
>>>>>>> + g_klass->dispose = workman_object_dispose;
>>>>>>> g_klass->finalize = workman_object_finalize;
>>>>>>> g_klass->set_property = workman_object_set_property;
>>>>>>> g_klass->get_property = workman_object_get_property;
>>>>>>> @@ -117,16 +181,24 @@ workman_object_class_init(WorkmanObjectClass *klass)
>>>>>>> G_PARAM_STATIC_STRINGS);
>>>>>>> g_object_class_install_property(g_klass, PROP_NAME, pspec);
>>>>>>>
>>>>>>> - pspec = g_param_spec_enum("state",
>>>>>>> - "State",
>>>>>>> - "The object's WorkmanState",
>>>>>>> - WORKMAN_TYPE_STATE,
>>>>>>> - 0,
>>>>>>> - G_PARAM_READWRITE |
>>>>>>> - G_PARAM_CONSTRUCT_ONLY |
>>>>>>> - G_PARAM_STATIC_STRINGS);
>>>>>>> + pspec = g_param_spec_flags("state",
>>>>>>> + "State",
>>>>>>> + "The object's WorkmanState",
>>>>>>> + WORKMAN_TYPE_STATE,
>>>>>>> + 0,
>>>>>>> + G_PARAM_READWRITE |
>>>>>>> + G_PARAM_CONSTRUCT_ONLY |
>>>>>>> + G_PARAM_STATIC_STRINGS);
>>>>>>> g_object_class_install_property(g_klass, PROP_STATE, pspec);
>>>>>>>
>>>>>>> + pspec = g_param_spec_pointer("attributes",
>>>>>>> + "Attributes",
>>>>>>> + "The object's list of WorkmanAttributes",
>>>>>>> + G_PARAM_READWRITE |
>>>>>>> + G_PARAM_STATIC_STRINGS);
>>>>>>
>>>>>> Hmm, one thing that just occurred to me - use of the 'pointer' parameter
>>>>>> type means that non-C language bindings only see an opaque pointer. They
>>>>>> have no way of knowing it is a list, nor are they able to traverse it.
>>>>>>
>>>>>> Previously I would have suggested GValueArray, but that is now deprecated.
>>>>>> We're supposed to use the GArray type in combination with a GValue, and
>>>>>> g_param_spec_boxed AFAICT, but the docs are clear as mud on how you
>>>>>> actually use this.
>>>>>
>>>>> Oh, I should mention the actual APIs in your patch are fine to use GList
>>>>> etc as you have done - only the property needs special handling.
>>>>
>>>> I think I found a way to turn a GList into a boxed object:
>>>>
>>>> https://mail.gnome.org/archives/commits-list/2013-January/msg03731.html
>>>>
>>>> It's similar to adding an enum type -- add a get_type() function and
>>>> call g_boxed_type_register_static(). That sound ok?
>>>
>>> The problem with adding new boxed types is that, AFAIK, it still won't
>>> be accessible to any introspection based language bindings. Adding
>>> boxed types was ok when people were manually writing language bindings,
>>> but these days it is all auto-generated from metadata & I don't think
>>> there's any way for language bindings to understand our new boxed type.
>>> Hence I think we need to stick with the standard ones. Or just don't
>>> create a property for this - require apps to just use the APIs we
>>> provide instead, which is a valid option.
>>
>> I had assumed that the introspection would figure out the bindings from the
>> following comment:
>>
>> + /**
>> + * GIMarshallingTestsPropertiesObject:some-boxed-glist:
>> + *
>> + * Type: GLib.List(gint)
>> + * Transfer: none
>> + */
>> + g_object_class_install_property (object_class, SOME_BOXED_GLIST_PROPERTY,
>> + g_param_spec_boxed ("some-boxed-glist", "some-boxed-glist", "some-boxed-glist",
>> + gi_marshalling_tests_boxed_glist_get_type(),
>> + G_PARAM_READABLE | G_PARAM_WRITABLE | G_PARAM_CONSTRUCT));
>> +
>>
>> See "Type: GLib.List(gint)" in the comments. That looks awfully similar to the
>> introspection gtk-doc annotations described in:
>>
>> https://live.gnome.org/GObjectIntrospection/Annotations
>>
>> Though that link doesn't specifically describe the annotation of properties...
>
> Hmm, i guess the only real answer is to actually try it with the
> python bindings and see if it works or not.
I got it to work using the annotations I mentioned above, combined with
some get_type() boilerplate. I verified that the generated gir file
showed the contained type:
<property name="attributes" writable="1" transfer-ownership="full">
<type name="GLib.List">
<type name="Attribute"/>
</type>
</property>
However, I'm wondering if this is the best approach...
You mentioned the possibility of just not creating the property. I kind
of like that idea because it gets rid of a lot of boilerplate code, for
this class and for all the others. Do you see any problem with just
not having *any* properties and only supporting the API interfaces?
The only real downside I can think of would be that we'd have to use
"protected" functions to set the name, state, attributes, etc when
initializing a subclass object. And GObject protected functions are
kind of a joke, as they seem to be based on the honor system (i.e. a
"/* protected */" comment). But I don't think that's a big deal.
More information about the Workman-devel
mailing list