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

[John W. Eaton]

On  4-Jan-2010, Thorsten Meyer wrote:

| 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?

I checked it in.

Thanks,

jwe