GNOME.org
 

Re: StdProp overhaul: broken plug-ins

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
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]