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

[Thorsten Meyer]

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.

Thorsten

# HG changeset patch
# User Thorsten Meyer <address@hidden>
# Date 1255376224 -7200
# Node ID 3e1401e7f06961e8ee275c55f24dfd37ab85593a
# Parent  519e164dde1ecd3644500a29a3e01e72d7db86a2
Fix set function to allow cell and struct arguments.

diff -r 519e164dde1e -r 3e1401e7f069 src/ChangeLog
--- a/src/ChangeLog     Thu Oct 08 20:58:14 2009 -0700
+++ b/src/ChangeLog     Mon Oct 12 21:37:04 2009 +0200
@@ -0,0 +1,8 @@
+2009-10-12  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 519e164dde1e -r 3e1401e7f069 src/graphics.cc
--- a/src/graphics.cc   Thu Oct 08 20:58:14 2009 -0700
+++ b/src/graphics.cc   Mon Oct 12 21:37:04 2009 +0200
@@ -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)
 {
@@ -4574,9 +4709,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;
@@ -4587,28 +4748,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 519e164dde1e -r 3e1401e7f069 src/graphics.h.in
--- a/src/graphics.h.in Thu Oct 08 20:58:14 2009 -0700
+++ b/src/graphics.h.in Mon Oct 12 21:37:04 2009 +0200
@@ -2067,6 +2067,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); }