Re: addproperty: a `XXX' property already exists in the graphics object

[John W. Eaton]

The patch attached below seems to fix the problem for me.  I have not
yet checked it because I don't completely understand the way dynamic
properties are intended to behave, or even how they are behaving
without the patch...

But it certainly does seem more reasonable to me that dynamic
properties would be specific to a given object, not a class of
objects.  So for example, adding a property to a single axes object
would not make it a property of all other axes objects, which from
reading the code seems like what the previous code was doing.

For example, I think

  hg1 = hggroup ();
  addproperty ("textlist", hg1, "data", 1);
  hg2 = hggroup ();

should only add the property "textlist" to the hg1 object.  The hg2
object should not have this new property.

However, if my interpretation is not correct, and instead the new
property should also appear in the hg2 object, then I think the
previous implementation was wrong anyway because it had the effect of
not only making the properties shared, but also their values.

>From reading the code (without the patch), I expected the following
behavior

  octave> hg1 = hggroup ();
  octave> addproperty ("textlist", hg1, "data", pi);
  octave> hg2 = hggroup ();
  octave> get (hg1, "textlist")
  ans =  3.1416
  octave> get (hg2, "textlist")
  ans =  3.1416
  octave> set (hg1, "textlist", e);
  octave> get (hg1, "textlist")
  ans =  2.7183
  octave> get (hg2, "textlist")
  ans =  2.7183

but instead, I see

  octave> hg1 = hggroup ();
  octave> addproperty ("textlist", hg1, "data", pi);
  octave> hg2 = hggroup ();
  octave> get (hg1, "textlist")
  ans =  3.1416
  octave> get (hg2, "textlist")
  error: get: unknown hggroup property textlist
  octave> set (hg1, "textlist", e);
  octave> get (hg1, "textlist")
  ans =  2.7183
  octave> get (hg2, "textlist")
  error: get: unknown hggroup property textlist

which is the same as what I get *with* the patch.  Hmm.  Now I'm a bit
confused, but as I said, with the patch, I no longer see any errors
about properties already existing when addproperty attempts to add
them.

Maybe someone who understands this code better can explain both what
is supposed to happen and what is happening without the patch?

If instead we are supposed to have shared property names but not
shared values, then I think we need something a bit different.

Comments?

jwe

diff --git a/src/genprops.awk b/src/genprops.awk
--- a/src/genprops.awk
+++ b/src/genprops.awk
@@ -264,12 +264,7 @@
   if (class_name && ! base)
     emit_common_declarations();
 
-  printf ("public:\n\n\n  static std::set<std::string> core_property_names 
(void);\n\n  static bool has_core_property (const caseless_str& pname);\n\n  
std::set<std::string> all_property_names (");
-  if (base)
-    printf ("const std::string& cname");
-  else
-    printf ("void");
-  printf (") const;\n\n");
+  printf ("public:\n\n\n  static std::set<std::string> core_property_names 
(void);\n\n  static bool has_core_property (const caseless_str& pname);\n\n  
std::set<std::string> all_property_names (void) const;\n\n");
 
   if (! base)
     printf ("  bool has_property (const caseless_str& pname) const;\n\n");
@@ -439,7 +434,7 @@
     ## set method
 
     if (base)
-      printf ("void\nbase_properties::set (const caseless_str& pname, const 
std::string& cname, const octave_value& val)\n{\n");
+      printf ("void\nbase_properties::set (const caseless_str& pname, const 
octave_value& val)\n{\n");
     else
       printf ("void\n%s::properties::set (const caseless_str& pname_arg, const 
octave_value& val)\n{\n",
               class_name);
@@ -460,9 +455,9 @@
     }
 
     if (base)
-      printf ("  else\n    set_dynamic (pname, cname, val);\n}\n\n");
+      printf ("  else\n    set_dynamic (pname, val);\n}\n\n");
     else
-      printf ("  else\n    base_properties::set (pname, \"%s\", val);\n}\n\n", 
class_name);
+      printf ("  else\n    base_properties::set (pname, val);\n}\n\n");
 
     ## get "all" method
 
@@ -610,16 +605,11 @@
        printf ("base_properties");
     else
       printf ("%s::properties", class_name);
-    printf ("::all_property_names (");
+    printf ("::all_property_names (void) const\n{\n  static 
std::set<std::string> all_pnames = core_property_names ();\n\n");
     if (base)
-      printf ("const std::string& cname");
+      printf ("  std::set<std::string> retval = all_pnames;\n  
std::set<std::string> dyn_props = dynamic_property_names ();\n  retval.insert 
(dyn_props.begin (), dyn_props.end ());\n  for (std::map<caseless_str, 
property, cmp_caseless_str>::const_iterator p = all_props.begin ();\n       p 
!= all_props.end (); p++)\n    retval.insert (p->first);\n\n  return 
retval;\n}\n\n");
     else
-      printf ("void");
-    printf (") const\n{\n  static std::set<std::string> all_pnames = 
core_property_names ();\n\n");
-    if (base)
-      printf ("  std::set<std::string> retval = all_pnames;\n  
std::set<std::string> dyn_props = dynamic_property_names (cname);\n  
retval.insert (dyn_props.begin (), dyn_props.end ());\n  for 
(std::map<caseless_str, property, cmp_caseless_str>::const_iterator p = 
all_props.begin ();\n       p != all_props.end (); p++)\n    retval.insert 
(p->first);\n\n  return retval;\n}\n\n");
-    else
-      printf ("  std::set<std::string> retval = all_pnames;\n  
std::set<std::string> base_props = base_properties::all_property_names 
(\"%s\");\n  retval.insert (base_props.begin (), base_props.end ());\n\n  
return retval;\n}\n\n", class_name);
+      printf ("  std::set<std::string> retval = all_pnames;\n  
std::set<std::string> base_props = base_properties::all_property_names ();\n  
retval.insert (base_props.begin (), base_props.end ());\n\n  return 
retval;\n}\n\n");
 
     if (! base)
       printf ("bool\n%s::properties::has_property (const caseless_str& pname) 
const\n{\n  std::set<std::string> pnames = all_property_names ();\n\n  return 
pnames.find (pname) != pnames.end ();\n}\n\n", class_name);
diff --git a/src/graphics.cc b/src/graphics.cc
--- a/src/graphics.cc
+++ b/src/graphics.cc
@@ -1804,26 +1804,22 @@
   return m;
 }
 
-std::map<std::string, std::set<std::string> > 
base_properties::all_dynamic_properties;
-
 std::set<std::string>
-base_properties::dynamic_property_names (const std::string& cname)
-{
-  return all_dynamic_properties[cname];
+base_properties::dynamic_property_names (void) const
+{
+  return dynamic_properties;
 }
 
 bool
-base_properties::has_dynamic_property (const std::string& pname,
-                                      const std::string& cname)
-{
-  const std::set<std::string>& dynprops = dynamic_property_names (cname);
+base_properties::has_dynamic_property (const std::string& pname)
+{
+  const std::set<std::string>& dynprops = dynamic_property_names ();
 
   return dynprops.find (pname) != dynprops.end ();
 }
 
 void
 base_properties::set_dynamic (const caseless_str& pname,
-                             const std::string& cname,
                              const octave_value& val)
 {
   std::map<caseless_str, property, cmp_caseless_str>::iterator it = 
all_props.find (pname);
@@ -1835,7 +1831,7 @@
 
   if (! error_state)
     {
-      all_dynamic_properties[cname].insert (pname);
+      dynamic_properties.insert (pname);
 
       mark_modified ();
     }
diff --git a/src/graphics.h.in b/src/graphics.h.in
--- a/src/graphics.h.in
+++ b/src/graphics.h.in
@@ -1642,13 +1642,7 @@
       all_props[name] = p;
     }
 
-  virtual void set (const caseless_str&, const octave_value&)
-  {
-    panic_impossible ();
-  }
-
-  void set (const caseless_str& pname, const std::string& cname,
-           const octave_value& val);
+  virtual void set (const caseless_str&, const octave_value&);
 
   virtual octave_value get (const caseless_str& pname) const;
 
@@ -1744,16 +1738,14 @@
     return ! handlevisibility.is ("off");
   }
 
-  static std::map<std::string, std::set<std::string> > all_dynamic_properties;
+  std::set<std::string> dynamic_property_names (void) const;
+
+  bool has_dynamic_property (const std::string& pname);
+
+protected:
+  std::set<std::string> dynamic_properties;
  
-  static std::set<std::string> dynamic_property_names (const std::string& 
cname);
-
-  static bool has_dynamic_property (const std::string& pname,
-                                   const std::string& cname);
-
-protected:
-  void set_dynamic (const caseless_str& pname, const std::string& cname,
-                   const octave_value& val);
+  void set_dynamic (const caseless_str& pname, const octave_value& val);
 
   octave_value get_dynamic (const caseless_str& pname) const;