Re: StdProp overhaul: broken plug-ins

[Cyrille Chepelov <chepelov calixo net>]

Hans, it's good to see you're back !

Le mer, aoû 15, 2001, à 06:07:28 +0200, Hans Breuer a écrit:
> +     * plug-ins/python/**/*: I gave up converting this; I can't
> +     compile --with-python (problems with object.h defined both by us
> +     and by Python). I'll gladly help whomever cares to fix it !
> +     (or, fix it myself if someone tells me how to compile it).
> +
>
> Hi Cyrille,
> while making the Python plug-in compile again I stumbled over
> two issues with the new property access, which would be nice
> if you could address them.
>
> 1) it would be nice if one could get a property refernce directly
> from the object, like:
>
> /* Return a reference to objects property with 'name' or NULL */

You already can do something like 
  prop = make_new_prop(PROP_TYPE_FOO, "name", 0);
 
Yes, this is a bit disturbing ; Property, rather than being an object's
property like its name implies, is just a cookie holding information of
various types (a bit like OLE2/COM's VARIANT). So, an object never "has"
Properties. It can be applied a _set_ of properties, and then it can choose to
either process the properties itself, or delegate to the library routines
(giving a set of PropOffsets, for instance). Hmmm. OK, you know that
already.

I'll comment on your usage of object_find_prop() in the patch.

> 2) IMO the property->type should become integral again, to allow
> for a switch statement, instead of string comparsions. See patch.

No. switch statements are the disease which turned lib/properties.c into
unmaintainability. If you need to make fast comparisons, use the type_quark 
field. PropertyType is a string, because it allows an object library to
define its own property type, perhaps of very specific purpose, and have the
StdProp core handle it, and never need to recompile the StdProp core for
that. And with constant initialisers (the old properties code had provisions
for library-custom property types, but that would have meant a variable
PROP_TYPE_FOO value, which is obviously a problem). 

If you really need a switch statement (I'm not going to force you to shuffle
around your code !), you can define a private integral property type, and
store the mapping between strings and your type in a GHash you initialise at
load time. Or better, mapping between quark and private type (should be
pretty fast).

> BTW: 'eml' still doesn't compile.

Yes, and that is intentional. I'm not touching this module even with 3 metre
pole, until at least his author is identified (remember that COPYING file ?)

I've sent a message to the single address I could scrounge from the commit
log (*not even* the ChangeLog !), about month ago, and am still waiting for
an answer. EML is out of objects/Makefile.am. 

>      else if (!strcmp(attr, "diagram"))
>       return PyDiaDiagram_New(self->disp->diagram);
> -    else if (!strcmp(attr, "origo") || !strcmp(attr, "origion"))
> +    /* FIXME: shouldn't it have only one name */
> +    else if (!strcmp(attr, "origo") || !strcmp(attr, "origion") ||
> !strcmp(attr, "origin"))

why do we have these many names, where some obviously look like typos ? Now
the plugin interface version has changed...

> -
> -    prop.name = PyString_AsString(key);
> -    prop.type = PROP_TYPE_INVALID;
> -    if (prop.name) {
> -      /* Get the first property with name */
> -      self->object->ops->get_props(self->object, &prop, 1);
> -      if (prop.type != PROP_TYPE_INVALID)
> -        val = PyDiaProperty_New(&prop); /* makes a copy, too. */
> -      prop_free(&prop);
> -    }
> +    Property *p;
> +    char* name = PyString_AsString(key);
> +    p = object_find_prop (self->object, name);  
> +    if (p && p->type != PROP_TYPE_INVALID)
> +      val = PyDiaProperty_New(p); /* makes a copy */

Isn't this better ?:
  GPtrArray *props = g_ptr_array_new();
  char *name = PyString_AsString(key);
  Property *prop = make_new_property(name,type /* find the type ! */,0)
  g_ptr_array_add(props,prop);
  
  self->object->ops->get_props(self->object,props);

  if (prop->experience & PXP_HAS_GFO) { 
        /* then it's valid */
  }

Mmmm... I think I begin to see why you need object_find_property(). Let's
drop that test to PROP_TYPE_INVALID: if the property is not NULL, it's good.
Even if it's a NoopProperty, you can safely call it, it's a Null Object
designed for that purpose (and the two others should never be called
anyway).

> Property     *object_find_prop (Object *obj, const char* name);

Problem with the prototype you've proposed is that this returns a single
property. And objects expect lists (GPtrArray) of properties....

What about:

GPtrArray *object_prop_singleton_by_name(Object *obj, const char *name);

You get returned either an array of a single property, if a property by that
name is exported by the object, or NULL. 

No, better:

/* Swallows the property into a singleton property list. Can be given NULL. */
/* Don't free yourself the property afterwards; prop_list_free() the list. */
/* You regain responsibility for the property if you g_ptr_array_destroy() the
   list. */
GPtrArray *prop_list_singleton(Property *prop);

/* create a synthetic property (with flags = 0). Free it with
   prop->ops->free(prop); or put it into a singleton list. NULL if object
   has nothing matching. Property's value is initialised by the object. */
Property *object_prop_by_name(Object *obj, const char *name, guint flags);
Property *object_prop_by_name_type(Object *obj, const char *name, 
                                   const char *type, guint flags);

(yes, this is almost your proposal with a few additions). The code above
becomes :
    Property *p;
    char* name = PyString_AsString(key);
    p = object_find_prop (self->object, name,0);  
    if (p) {
        val = PyDiaProperty_New(p); /* makes a copy */
        p->ops->free(p);
    }
    
You'll have noted that object_prop_by_name is almost object_find_prop(). If 
it can't find a matching property name in the object's description, it'll 
return NULL. If it can, it will (as you assumed) ask the object to fill the 
property. To do so, it'll have to hold a static singleton property list, but 
this is none of the caller's business.

Shall I rush an implement that ?


> +#ifdef THE_PROP_TYPE_ID_IS_INTEGRAL
> +    switch (self->property->type) {
>      case PROP_TYPE_CHAR :
> -      return PyInt_FromLong(self->property.d.char_data);
> +      return PyInt_FromLong(((CharProperty*)self->property)->char_data);
>      case PROP_TYPE_BOOL :
>        return PyInt_FromLong(self->property.d.bool_data);
>      case PROP_TYPE_INT :

Quoting Fowler and Beck: 
----
Smells: Switch Statement, Primitive Obsession (etc.)
Refactoring: Replace Type Code with Subclasses.
----

In this case, it's almost trivial. Simply define a set routines matching
   PYTHONTYPE (*PythonFromPropFunc) (const Property *prop);

(actually, something like:
static PYTHONTYPE pyint_from_charprop(const CharProperty *prop);
static PYTHONTYPE pyint_from_intprop(const IntProperty *prop); 
   etc...)

lots of prototypes and braces, but less casts... And this trivial garbage
can easily be hidden in a separate compilation unit (that's the strategy of
all these lib/prop_*.c) 
Then, register a GHash of PythonFromPropFunc indexed on type_quarks. Instead 
of the switch, lookup the type quark you've been given, if you have a routine 
you just return its result, or return whatever sensible default (NULL? 
Py_None ?)

This even gives you the flexibility of understanding a property type
provided by a specific plug-in, and which the core still doesn't understand
(perhaps because it hasn't been deemed generic enough).

Now, if you need two GHashes with different functions (because there are
several similar switch(prop->type) statements)... Huh, that smells data
clumps... no problem, Extract Class (that's how PropertyOps was born, by the
way)

> +#ifdef THE_PROP_TYPE_ID_IS_INTEGRAL
>  #define CASE_STR(s) case PROP_TYPE_##s : tname = #s; break;

yeeech !

> +#else
> +  s = g_strdup_printf("<DiaProperty at 0x%08x, \"%s\", %s>",
> +                      self,
> +                      self->property->name,
> +                      self->property->type);
> +#endif

but that, you'll admit, makes having a string type a useful feature, doesn't
it ? <grin />

I hope my ramblings make sense....

        -- Cyrille

-- 
Grumpf.