Re: StdProp overhaul: broken plug-ins

[Hans Breuer <hans breuer org>]

At 22:02 15.08.01 +0200, Cyrille Chepelov wrote:
>Hans, it's good to see you're back !
>
Don't expect too much, IMO Gtk2 has still higher priority.
Even all you factorizing would be simplier there :)

>> [...]
>> 
>> 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). 
Obviously a Property class can be a VARIANT, which allows all the
nice abstract handling for operations which aren't interested in
the type ...

>So, an object never "has"
>Properties. 
It obviously has specific properties like 'width', 'height', etc. which 
are in fact properties of single objects. (They are assumed to vary
between different objects). And the next thing one wants to do with
the properties (from language bindings is to read and change them.

You definetly need to have the concrete type than to don't do

obj.width =  obj.width + obj.name

Ooops. Type error ...

>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.
>
Yeah. And I haven't continued on the Dia Python plug-in because there
are simply missing (or to complicated) things to do with the current
api. You may want to take a look into GObject, which addresses them
all as far as i know.

Say ref counting, change notification, ...

>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). 
>
But any de-marshalling needs them. Or one needs to provide member 
functions to common types, see GObject::g_value_transform
  
>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).
>
Ok you're right with the switch statement, I'll probably need to 
understand Quarks some time ...

>> 
>> 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. 
>
ok, I'll disable it in the win32 build too.
 
>>      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...
>
I'm only maintaining it. Could probably be fixed.
 
>> -
>> -    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....
>
But can an object really have two indepent properties which share
the same name ??

>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);
>
You don't mean the singleton from 'Design Patterns', do you?
As noted above: multiple objects of the same type do share
a the same property names but the property values of differnt
objects are independent. By no means a Singleton (= there is
only one object from this class)

>/* 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);  

what flags do you have in mind ? Setting a patameter for future
unknown additions appears to be rather odd.
 
>    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. 
Why mot simply return the (ref counted) pointer of the object. 
Ok there is currently no ref counting, but the Python plug-in
will crash on deleting whole Dia objects under it's feet
anyway. But the object reference is needed.

>To do so, it'll have to hold a static singleton property list, but 
>this is none of the caller's business.
>
I still don't see the point of 'singleton' ...

>Shall I rush an implement that ?
>
If we get consensus on the above, go right ahead :-)

>
>> +#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...)
>
Can't see how this would help clarifing the code. In the whole
Python plug-in there is exactly one place where the de-marshalling
is needed ...

>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) 
... maybe it should provide the type conversion functions than, too ?

>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 ?)
>
Now I see part of the point ...

>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 !
>
I always try to balance between design theory and pragmatism :-)
 
>> +#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 />
>
Not really. This is near at debug output.
 
	Hans
-------- Hans "at" Breuer "dot" Org -----------
Tell me what you need, and I'll tell you how to 
get along without it.                -- Dilbert