Re: plot templates and options lists for set, plot etc.

[Thorsten Meyer]

Thorsten Meyer wrote:
> Thanks for your feedback (and your offline reminder :-) ).
> 
> Included you will find an updated patch. See comments below.
> 
> John W. Eaton wrote:
>> On 16-Sep-2009, Thorsten Meyer wrote:
>>
>> | Thorsten Meyer wrote:
>> | > Here is a patch implementing struct and cell array arguments for the set 
>> function.
>> | > 
>> | > I have impplemented the matlab behaviour (as I understand it ...). See 
>> the tests
>> | >  added to the patch for the details.
>> | > 
>> | > Could somebody please review the patch (what I do in the sources is 
>> still a lot
>> | > of trial and error):
>> | >  - Have I got the types of the various variables right?
>>
>> | >  - Is it ok to add doxygen style comments to the new methods? (the
>> | >    doxygen docs
>>
>> I don't use Doxygen, so I'm unlikely to remember to add the markup in
>> the comments I write.  If we decide to do this, then I think we should
>> try to do it on a more global scale.
> I removed the Doxygen markup for now.
> 
>> | >  - What about the coding style?
>> | >  - Are the added methods for the graphics_object ok (also where I placed 
>> the
>> | > definitions)?
>>
>> I try to keep the order of the declarations in the .h file the same as
>> the order in the .cc file.  If you've done that, then it is probably
>> OK.
> I fixed the order to be consistent in .h, .cc and the documentation string.
> 
>> | +//! Set properties given in two cell arrays containing names and values.
>> | +void
>> | +graphics_object::set (const Array<std::string> names,
>> | +                      const Cell values, octave_idx_type row)
>>
>> I think this should be 
>>
>>   graphics_object::set (const Array<std::string>& names,
>>                         const Cell& values, octave_idx_type row)
> done.
> 
>> | +  if (! (names.numel () == values.columns ())) 
>>
>> Why use
>>
>>   ! (x == y)
>>
>> instead of
>>
>>   x != y
>>
>> ?
> I changed it.
> 
>> | +void
>> | +graphics_object::set (const Octave_map m)
>>
>> This should be
>>
>>   void
>>   graphics_object::set (const Octave_map& m)
> Done
> 
>> | +//! Set a property to a value or to its (factory) default value.
>> | +void graphics_object::set_value_or_default (caseless_str& name,
>> | +                                            octave_value& val)
>>
>> I think this should be
>>
>>   void
>>   graphics_object::set_value_or_default (const caseless_str& name,
>>                                          const octave_value& val)
>>
>> unless the function needs to modify NAME and VAL.  It doesn't appear
>> need to.
>>
>> | +  if (val.is_string ())
>> | +    {
>> | +      caseless_str tval = val.string_value ();
>> | +
>> | +      if (tval.compare ("default"))
>> | +        val = get_default (name);
>> | +      else if (tval.compare ("factory"))
>> | +        val = get_factory_default (name);
>> | +    }
>> | +
>> | +  if (error_state)
>> | +    return;
>> | +
>> | +  rep->set (name, val);
>> | +}
>>
>> With the non-const VAL, if VAL is a string, then it will also be
>> changed in the caller.  Is that really what you want?  I think it
>> would be better to define a local temporary value inside the
>> function.
> I changed the method arguments as you suggested and blew up the conditional a
> bit to avoid copying val.
> 

here is a kind reminder for the patch above and an updated patch. Can it be
applied as it is?

thanks

Thorsten

# HG changeset patch
# User Thorsten Meyer <address@hidden>
# Date 1262599429 -3600
# Node ID 945826d8f9aa27779958e20c215800d763992340
# Parent  5813ec5077b568b705a536907d219ac0e44b76e5
Fix set function to allow cell and struct arguments.

diff -r 5813ec5077b5 -r 945826d8f9aa src/ChangeLog
--- a/src/ChangeLog     Mon Jan 04 03:00:19 2010 -0500
+++ b/src/ChangeLog     Mon Jan 04 11:03:49 2010 +0100
@@ -0,0 +1,8 @@
+2010-01-04  Thorsten Meyer  <address@hidden>
+
+       * graphics.cc (Fset), graphics.cc (graphics_object::set): Add
+       support for struct and cell arguments.
+
+       * graphics.h.in (graphics_objects::set): Add prototypes for
+       calling with struct respectively cell arguments.
+
diff -r 5813ec5077b5 -r 945826d8f9aa src/graphics.cc
--- a/src/graphics.cc   Mon Jan 04 03:00:19 2010 -0500
+++ b/src/graphics.cc   Mon Jan 04 11:03:49 2010 +0100
@@ -1395,6 +1395,7 @@
     }
 }
 
+// Set properties given as a cs-list of name, value pairs
 void
 graphics_object::set (const octave_value_list& args)
 {
@@ -1412,20 +1413,10 @@
            {
              octave_value val = args(i+1);
 
-             if (val.is_string ())
-               {
-                 caseless_str tval = val.string_value ();
-
-                 if (tval.compare ("default"))
-                   val = get_default (name);
-                 else if (tval.compare ("factory"))
-                   val = get_factory_default (name);
-               }
+              set_value_or_default (name, val);
 
              if (error_state)
                break;
-
-             rep->set (name, val);
            }
          else
            error ("set: expecting argument %d to be a property name", i);
@@ -1435,6 +1426,150 @@
     error ("set: invalid number of arguments");
 }
 
+/*
+%!# test set with name, value pairs
+%!test
+%!  set(gcf, "visible", "off");
+%!  h = plot (1:10, 10:-1:1);
+%!  set (h, "linewidth", 10, "marker", "x");
+%!  assert (get (h, "linewidth"), 10);
+%!  assert (get (h, "marker"), "x");
+*/
+
+// Set properties given in two cell arrays containing names and values.
+void
+graphics_object::set (const Array<std::string>& names,
+                      const Cell& values, octave_idx_type row)
+{
+  if (names.numel () != values.columns ())
+    {
+      error("set: number of names must match number of value columns (%d != 
%d)",
+            names.numel (), values.columns ());
+    }
+
+  octave_idx_type k = names.columns ();
+
+  for (octave_idx_type column = 0; column < k; column++)
+    {
+      caseless_str name = names(column);
+      octave_value val  = values(row, column);
+
+      set_value_or_default (name, val);
+
+      if (error_state)
+        break;
+    }
+}
+
+/*
+%!# test set with cell array arguments
+%!test
+%!  set (gcf, "visible", "off");
+%!  h = plot (1:10, 10:-1:1);
+%!  set (h, {"linewidth", "marker"}, {10, "x"});
+%!  assert (get(h, "linewidth"), 10);
+%!  assert (get(h, "marker"), "x");
+
+%!# test set with multiple handles and cell array arguments
+%!test
+%!  set (gcf, "visible", "off");
+%!  h = plot (1:10, 10:-1:1, 1:10, 1:10);
+%!  set (h, {"linewidth", "marker"}, {10, "x"; 5, "o"});
+%!  assert (get (h, "linewidth"), {10; 5});
+%!  assert (get (h, "marker"), {"x"; "o"});
+%!  set (h, {"linewidth", "marker"}, {10, "x"});
+%!  assert (get (h, "linewidth"), {10; 10});
+%!  assert (get (h, "marker"), {"x"; "x"});
+
+%!error <set: number of graphics handles must match number of value rows>
+%!  set (gcf, "visible", "off");
+%!  h = plot (1:10, 10:-1:1, 1:10, 1:10);
+%!  set (h, {"linewidth", "marker"}, {10, "x"; 5, "o"; 7, "."});
+
+%!error <set: number of names must match number of value columns>
+%!  set (gcf, "visible", "off");
+%!  h = plot (1:10, 10:-1:1, 1:10, 1:10);
+%!  set (h, {"linewidth"}, {10, "x"; 5, "o"});
+*/
+
+// Set properties given in a struct array
+void
+graphics_object::set (const Octave_map& m)
+{
+  for (Octave_map::const_iterator p = m.begin ();
+       p != m.end (); p++)
+    {
+      caseless_str name  = m.key (p);
+
+      octave_value val = octave_value (m.contents (p).elem (m.numel () - 1));
+
+      set_value_or_default (name, val);
+
+      if (error_state)
+        break;
+    }
+}
+
+/*
+%!# test set with struct arguments
+%!test
+%!  set (gcf, "visible", "off");
+%!  h = plot (1:10, 10:-1:1);
+%!  set (h, struct ("linewidth", 10, "marker", "x"));
+%!  assert (get (h, "linewidth"), 10);
+%!  assert (get (h, "marker"), "x");
+%!  h = plot (1:10, 10:-1:1, 1:10, 1:10);
+%!  set (h, struct ("linewidth", {5, 10}));
+%!  assert (get(h, "linewidth"), {10; 10});
+*/
+
+// Set a property to a value or to its (factory) default value.
+void graphics_object::set_value_or_default (const caseless_str& name,
+                                            const octave_value& val)
+{
+  if (val.is_string ())
+    {
+      caseless_str tval = val.string_value ();
+
+      octave_value default_val;
+
+      if (tval.compare ("default"))
+        {
+          default_val = get_default (name);
+
+          if (error_state)
+            return;
+
+          rep->set (name, default_val);
+        }
+      else if (tval.compare ("factory"))
+        {
+          default_val = get_factory_default (name);
+
+          if (error_state)
+            return;
+
+          rep->set (name, default_val);
+        }
+      else
+        rep->set (name, val);
+    }
+  else
+    rep->set (name, val);
+}
+
+/*
+%!# test setting of default values
+%!test
+%!  set (gcf, "visible", "off");
+%!  h = plot (1:10, 10:-1:1);
+%!  set (0, "defaultlinelinewidth", 20);
+%!  set (h, "linewidth", "default");
+%!  assert (get (h, "linewidth"), 20);
+%!  set (h, "linewidth", "factory");
+%!  assert (get (h, "linewidth"), 0.5);
+*/
+
 static double
 make_handle_fraction (void)
 {
@@ -4570,9 +4705,35 @@
 
 DEFUN (set, args, ,
   "-*- texinfo -*-\n\
address@hidden {Built-in Function} {} set (@var{h}, @var{p}, @var{v}, 
@dots{})\n\
-Set the named property value or vector @var{p} to the value @var{v}\n\
-for the graphics handle @var{h}.\n\
address@hidden {Built-in Function} {} set (@var{h}, @var{property}, 
@var{value}, @dots{})\n\
address@hidden {Built-in Function} {} set (@var{h}, @var{properties}, 
@var{values})\n\
address@hidden {Built-in Function} {} set (@var{h}, @var{pv})\n\
+Set named property values for the graphics handle (or vector of graphics\n\
+handles) @var{h}.\n\
+There are three ways how to give the property names and values:\n\
+\n\
address@hidden
address@hidden as a comma separated list of @var{property}, @var{value} pairs\n\
+\n\
+Here, each @var{property} is a string containing the property name, each\n\
address@hidden is a value of the appropriate type for the property.\n\
address@hidden as a cell array of strings @var{properties} containing property 
names\n\
+and a cell array @var{values} containing property values.\n\
+\n\
+In this case, the number of columns of @var{values} must match the number of\n\
+elements in @var{properties}.  The first column of @var{values} contains 
values\n\
+for the first entry in @var{properties} etc..  The number of rows of 
@var{values}\n\
+must be 1 or match the number of elements of @var{h}. In the first case, 
each\n\
+handle in @var{h} will be assigned the same values. In the latter case, the\n\
+first handle in @var{h} will be assigned the values from the first row of\n\
address@hidden and so on.\n\
address@hidden as a structure array @var{pv}\n\
+\n\
+Here, the field names of @var{pv} represent the property names, and the 
field\n\
+values give the property values.  In contrast to the previous case, all\n\
+elements of @var{pv} will be set in all handles in @var{h} independent of\n\
+the dimensions of @var{pv}.\n\
address@hidden itemize\n\
 @end deftypefn")
 {
   gh_manager::autolock guard;
@@ -4583,28 +4744,62 @@
 
   if (nargin > 0)
     {
+      // get vector of graphics handles
       ColumnVector hcv (args(0).vector_value ());
 
       if (! error_state)
         {
          bool request_drawnow = false;
 
+          // loop over graphics objects
           for (octave_idx_type n = 0; n < hcv.length (); n++) 
             {
               graphics_object obj = gh_manager::get_object (hcv(n));
 
               if (obj)
                 {
-                  obj.set (args.splice (0, 1));
-
-                  request_drawnow = true;
+                  if (nargin == 3 && args(1).is_cellstr ()
+                      && args(2).is_cell ())
+                    {
+                      if (args(2).cell_value ().rows () == 1)
+                        {
+                          obj.set (args(1).cellstr_value (),
+                                   args(2).cell_value (), 0);
+                        }
+                      else if (hcv.length () == args(2).cell_value ().rows ())
+                        {
+                          obj.set (args(1).cellstr_value (),
+                                   args(2).cell_value (), n);
+                        }
+                      else
+                        {
+                          error("set: number of graphics handles must match 
number of value rows (%d != %d)",
+                                hcv.length (), args(2).cell_value ().rows ());
+                          break;
+
+                        }
+                    }
+                  else if (nargin == 2 && args(1).is_map ())
+                    {
+                      obj.set (args(1).map_value ());
+                    }
+                  else
+                    {
+                      obj.set (args.splice (0, 1));
+                      request_drawnow = true;
+                    }
                 }
               else
                {
                  error ("set: invalid handle (= %g)", hcv(n));
                  break;
                }
-            }
+
+              if (error_state)
+                break;
+
+              request_drawnow = true;
+           }
 
          if (! error_state && request_drawnow)
            Vdrawnow_requested = true;
diff -r 5813ec5077b5 -r 945826d8f9aa src/graphics.h.in
--- a/src/graphics.h.in Mon Jan 04 03:00:19 2010 -0500
+++ b/src/graphics.h.in Mon Jan 04 11:03:49 2010 +0100
@@ -2059,6 +2059,14 @@
 
   void set (const octave_value_list& args);
 
+  void set (const Array<std::string>& names, const Cell& values,
+            octave_idx_type row);
+
+  void set (const Octave_map& m);
+
+  void set_value_or_default (const caseless_str& name,
+                             const octave_value& val);
+
   void set_defaults (const std::string& mode) { rep->set_defaults (mode); }
 
   octave_value get (bool all = false) const { return rep->get (all); }