[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [Workman-devel] [PATCH 4/4] Add attributes property to WorkmanObject



On Thu, Feb 28, 2013 at 10:40:37AM +0000, Daniel P. Berrange wrote:
> On Wed, Feb 27, 2013 at 05:55:37PM -0600, Josh Poimboeuf wrote:
> > - 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 | 110 ++++++++++++++++++++++++++++++++++++++++-------
> >  workman/workman-object.h |  16 -------
> >  workman/workman-state.h  |   7 +--
> >  3 files changed, 99 insertions(+), 34 deletions(-)
> > 
> > diff --git a/workman/workman-object.c b/workman/workman-object.c
> > index 11bf434..d22e0a8 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,
> > @@ -58,6 +101,12 @@ workman_object_get_property(GObject *object,
> >          case PROP_STATE:
> >              g_value_set_uint(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);
> >              break;
> > @@ -80,6 +129,9 @@ workman_object_set_property(GObject *object,
> >          case PROP_STATE:
> >              self->priv->state = g_value_get_uint(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);
> >              break;
> > @@ -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;
> > @@ -128,6 +192,14 @@ workman_object_class_init(WorkmanObjectClass *klass)
> >                                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);
> > +
> > +    g_object_class_install_property(g_klass, PROP_ATTRIBUTES, pspec);
> > +
> >      g_type_class_add_private(klass, sizeof(WorkmanObjectPrivate));
> >  }
> >  
> > @@ -151,12 +223,11 @@ const gchar *workman_object_get_name(WorkmanObject *obj)
> >   *
> >   * Returns: (transfer full) (element-type WorkmanAttribute): the attributes for this object
> >   */
> > -GList *workman_object_get_attributes(WorkmanObject *obj,
> > +GList *workman_object_get_attributes(WorkmanObject *self,
> >                                       WorkmanState state,
> >                                       GError **error)
> >  {
> > -    WorkmanObjectClass *klass = WORKMAN_OBJECT_GET_CLASS(obj);
> > -    return klass->get_attributes(obj, state, error);
> > +    return workman_object_copy_attributes(self->priv->attributes, state);
> >  }
> >  
> >  
> > @@ -165,31 +236,40 @@ GList *workman_object_get_attributes(WorkmanObject *obj,
> >   *
> >   * Returns: (transfer full): the attribute or NULL
> >   */
> > -WorkmanAttribute *workman_object_get_attribute(WorkmanObject *obj,
> > +WorkmanAttribute *workman_object_get_attribute(WorkmanObject *self,
> >                                                 WorkmanState state,
> >                                                 const gchar *name,
> >                                                 GError **error)
> >  {
> > -    WorkmanObjectClass *klass = WORKMAN_OBJECT_GET_CLASS(obj);
> > -    return klass->get_attribute(obj, state, name, error);
> > +    GList *a;
> > +    WorkmanAttribute *attr;
> > +
> > +    for (a = self->priv->attributes; a; a = a->next) {
> > +        attr = a->data;
> > +        if (workman_attribute_get_state(attr) == state &&
> > +            !strcmp(workman_attribute_get_name(attr), name)) {
> 
> You can use   g_str_equal() instead of !strcmp
> 
> > +            g_object_ref(a);
> > +            return attr;
> 
> Instead of these two lines (with typo in first) just use
> 
>    return g_object_ref(attr);
> 
> > +        }
> > +    }
> > +
> > +    return NULL;
> >  }
> >  
> >  
> > -gboolean workman_object_refresh_attributes(WorkmanObject *obj,
> > +gboolean workman_object_refresh_attributes(WorkmanObject *self,
> >                                             WorkmanState state,
> >                                             GError **error)
> >  {
> > -    WorkmanObjectClass *klass = WORKMAN_OBJECT_GET_CLASS(obj);
> > -    return klass->refresh_attributes(obj, state, error);
> > +    /* TODO: call plugin->get_attributes and update obj->priv->attributes */
> >  }
> >  
> >  
> > -gboolean workman_object_save_attributes(WorkmanObject *obj,
> > +gboolean workman_object_save_attributes(WorkmanObject *self,
> >                                          WorkmanState state,
> >                                          GError **error)
> >  {
> > -    WorkmanObjectClass *klass = WORKMAN_OBJECT_GET_CLASS(obj);
> > -    return klass->save_attributes(obj, state, error);
> > +    /* TODO: call plugin->update_attribute for each changed attribute */
> >  }
> >  
> >  
> > diff --git a/workman/workman-object.h b/workman/workman-object.h
> > index 7754783..88eb27c 100644
> > --- a/workman/workman-object.h
> > +++ b/workman/workman-object.h
> > @@ -59,22 +59,6 @@ struct _WorkmanObjectClass
> >      GObjectClass parent_class;
> >  
> >  
> > -    /* class members */
> > -    GList *(*get_attributes)(WorkmanObject *obj,
> > -                             WorkmanState state,
> > -                             GError **error);
> > -    WorkmanAttribute *(*get_attribute)(WorkmanObject *obj,
> > -                                       WorkmanState state,
> > -                                       const gchar *name,
> > -                                       GError **error);
> > -    gboolean (*refresh_attributes)(WorkmanObject *obj,
> > -                                   WorkmanState state,
> > -                                   GError **error);
> > -    gboolean (*save_attributes)(WorkmanObject *obj,
> > -                                WorkmanState state,
> > -                                GError **error);
> > -
> > -
> >      /* Remove from padding when adding new virtual functions */
> >      gpointer padding[20];
> >  };
> > diff --git a/workman/workman-state.h b/workman/workman-state.h
> > index d3a86f2..2e8072d 100644
> > --- a/workman/workman-state.h
> > +++ b/workman/workman-state.h
> > @@ -31,9 +31,10 @@ G_BEGIN_DECLS
> >  
> >  
> >  typedef enum {
> > -    WORKMAN_STATE_ALL,
> > -    WORKMAN_STATE_ACTIVE,
> > -    WORKMAN_STATE_PERSISTENT,
> > +    WORKMAN_STATE_ACTIVE        = 1 << 0,
> > +    WORKMAN_STATE_PERSISTENT    = 1 << 1,
> > +    WORKMAN_STATE_ALL           = WORKMAN_STATE_ACTIVE |
> > +                                  WORKMAN_STATE_PERSISTENT,
> 
> If you do this, then wrt my point about using g_param_spec_enum
> in the previous patch - this will imply a change to use
> g_param_spec_flags instead.


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);
+
+    g_object_class_install_property(g_klass, PROP_ATTRIBUTES, pspec);
+
     g_type_class_add_private(klass, sizeof(WorkmanObjectPrivate));
 }
 
@@ -150,12 +222,11 @@ const gchar *workman_object_get_name(WorkmanObject *obj)
  *
  * Returns: (transfer full) (element-type WorkmanAttribute): the attributes for this object
  */
-GList *workman_object_get_attributes(WorkmanObject *obj,
+GList *workman_object_get_attributes(WorkmanObject *self,
                                      WorkmanState state,
                                      GError **error)
 {
-    WorkmanObjectClass *klass = WORKMAN_OBJECT_GET_CLASS(obj);
-    return klass->get_attributes(obj, state, error);
+    return workman_object_copy_attributes(self->priv->attributes, state);
 }
 
 
@@ -164,31 +235,38 @@ GList *workman_object_get_attributes(WorkmanObject *obj,
  *
  * Returns: (transfer full): the attribute or NULL
  */
-WorkmanAttribute *workman_object_get_attribute(WorkmanObject *obj,
+WorkmanAttribute *workman_object_get_attribute(WorkmanObject *self,
                                                WorkmanState state,
                                                const gchar *name,
                                                GError **error)
 {
-    WorkmanObjectClass *klass = WORKMAN_OBJECT_GET_CLASS(obj);
-    return klass->get_attribute(obj, state, name, error);
+    GList *a;
+    WorkmanAttribute *attr;
+
+    for (a = self->priv->attributes; a; a = a->next) {
+        attr = a->data;
+        if (workman_attribute_get_state(attr) == state &&
+            g_str_equal(workman_attribute_get_name(attr), name))
+            return g_object_ref(attr);
+    }
+
+    return NULL;
 }
 
 
-gboolean workman_object_refresh_attributes(WorkmanObject *obj,
+gboolean workman_object_refresh_attributes(WorkmanObject *self,
                                            WorkmanState state,
                                            GError **error)
 {
-    WorkmanObjectClass *klass = WORKMAN_OBJECT_GET_CLASS(obj);
-    return klass->refresh_attributes(obj, state, error);
+    /* TODO: call plugin->get_attributes and update obj->priv->attributes */
 }
 
 
-gboolean workman_object_save_attributes(WorkmanObject *obj,
+gboolean workman_object_save_attributes(WorkmanObject *self,
                                         WorkmanState state,
                                         GError **error)
 {
-    WorkmanObjectClass *klass = WORKMAN_OBJECT_GET_CLASS(obj);
-    return klass->save_attributes(obj, state, error);
+    /* TODO: call plugin->update_attribute for each changed attribute */
 }
 
 
diff --git a/workman/workman-object.h b/workman/workman-object.h
index 7754783..88eb27c 100644
--- a/workman/workman-object.h
+++ b/workman/workman-object.h
@@ -59,22 +59,6 @@ struct _WorkmanObjectClass
     GObjectClass parent_class;
 
 
-    /* class members */
-    GList *(*get_attributes)(WorkmanObject *obj,
-                             WorkmanState state,
-                             GError **error);
-    WorkmanAttribute *(*get_attribute)(WorkmanObject *obj,
-                                       WorkmanState state,
-                                       const gchar *name,
-                                       GError **error);
-    gboolean (*refresh_attributes)(WorkmanObject *obj,
-                                   WorkmanState state,
-                                   GError **error);
-    gboolean (*save_attributes)(WorkmanObject *obj,
-                                WorkmanState state,
-                                GError **error);
-
-
     /* Remove from padding when adding new virtual functions */
     gpointer padding[20];
 };
diff --git a/workman/workman-state.h b/workman/workman-state.h
index d3a86f2..2e8072d 100644
--- a/workman/workman-state.h
+++ b/workman/workman-state.h
@@ -31,9 +31,10 @@ G_BEGIN_DECLS
 
 
 typedef enum {
-    WORKMAN_STATE_ALL,
-    WORKMAN_STATE_ACTIVE,
-    WORKMAN_STATE_PERSISTENT,
+    WORKMAN_STATE_ACTIVE        = 1 << 0,
+    WORKMAN_STATE_PERSISTENT    = 1 << 1,
+    WORKMAN_STATE_ALL           = WORKMAN_STATE_ACTIVE |
+                                  WORKMAN_STATE_PERSISTENT,
 } WorkmanState;
 
 
-- 
1.7.11.7


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]