geda-developers team mailing list archive

[Ivan Stankovic <pokemon@xxxxxxxxxxxxxx>]

On Mon, Dec 17, 2012 at 04:31:43PM +0000, Peter TB Brett wrote:
> #define EDA_TYPE_OBJECTSET (eda_object_set_get_type ())
                                        ^
You probably want to remove that underscore.

Also, EdaObjectset looks a bit weird, compared to naming conventions
in other libraries, but I don't have a better name.

> struct _EdaObjectsetClass
> {
>   GObjectClass parent_class;
> 
>   /* virtual public methods */
>   gboolean (*get_iter)(EdaObjectset *set, EdaObjectIter *iter, gint index);

I assume the index here is the initial iterator position.

>   gboolean (*find)(EdaObjectset *set, EdaObjectIter *iter);

Missing OBJECT * here.

>   OBJECT *(*get)(EdaObjectset *set, EdaObjectIter *iter);
>   void (*append)(EdaObjectset *set, OBJECT *object);
>   void (*insert)(EdaObjectset *set, EdaObjectIter *iter, OBJECT *object);
>   void (*remove)(EdaObjectset *set, EdaObjectIter *iter, OBJECT *object);

Looking good (but I wonder about subclasses and how they might implement these).

> /* Convenience functions */
> EdaObjectset *eda_objectset_new (void) G_GNUC_WARN_UNUSED_RESULT;
> void eda_objectset_foreach (EdaObjectset *set, GCallback func, gpointer user_data);
> 
> /* Getting iterators */
> gboolean eda_objectset_get_iter (EdaObjectset *set, EdaObjectIter *iter, gint index);
> gboolean eda_objectset_iter_next (EdaObjectset *set, EdaObjectIter *iter);
> gboolean eda_objectset_find (EdaObjectset *set, EdaObjectIter *iter, OBJECT *object);
> 
> /* Obtaining values */
> OBJECT *eda_object_set_get (EdaObjectset *set, EdaObjectIter *iter);
                    ^
Extra undescore.

> /* Modifying the set */
> void eda_objectset_append (EdaObjectset *set, OBJECT *object);
> void eda_objectset_insert (EdaObjectset *set, EdaObjectIter *iter, OBJECT *object);
> void eda_objectset_remove (EdaObjectset *set, EdaObjectIter *iter);

Looking good, but I expect the eda_objectset_remove method will be cumbersome
to use.  You would first need to obtain the iterator that points to the object
and then call eda_objectset_remove. For example, eda_objectset_append does not
require an iterator.

APIs are best judged by examples so let's say that I want to remove
all objects from a page that satisfy some criteria:

    EdaObjectIter iter;
    EdaObjectset *set = eda_page_get_objects(page);

    if(eda_objectset_get_iter(set, &iter, 0)) {
        while(1) {
            EdaObject *obj = eda_objectset_get(set, &iter);

            if(should_remove(obj))
                eda_objectset_remove(set, &iter);

            if(!eda_objectset_iter_next(set, &iter))
                break;
        }
    }


While this is not too verbose, I have a feeling we could do better. More
troublesome is that it's not obvious how eda_objectset_remove and 
eda_objectset_iter_next interact. Does eda_objectset_remove automatically
advances the iterator? Does the iterator get invalidated?

-- 
Ivan Stankovic, pokemon@xxxxxxxxxxxxxx

"Protect your digital freedom and privacy, eliminate DRM, 
learn more at http://www.defectivebydesign.org/what_is_drm";