Re: general c++ question

[Shai Ayal]

On 8/28/07, John W. Eaton <address@hidden> wrote:
> On 28-Aug-2007, Shai Ayal wrote:
>
> | In a patch I submitted some time ago with the subject "exposing
> | graphics object properties" I outline what is the call graph for
> | getting properties. I quote from that mail:
> |
> | -------
> | I attach a patch who's purpose is to expose the properties of the
> | graphics objects. The purpose of this patch to allow access to
> | properties as in:
> |
> | if (gh_manager::get_object (h).isa () == "image")
> |   image_properties ip = gh_manager::image_properties_value (h);
> |
> | It turned out it involved quite a lot of new functions and
> | "declaration acrobatics" where function definitions had to be moved
> | because of forward declarations. To illustrates, the above functions
> | call will set in motions the following:
> |
> | gh_manager::image_properties_value
> | gh_manager::do_image_properties_value
> | graphics_object:::image_properties_value
> | virtual base_graphics_object::image_properties_value
> |
> | in all these calls the image_properties class is passed by value. I
> | also had to un-nest the properties classes from inside their
> | respective object classes in order to allow their forward
> | declarations.
> |
> | I do not like my patch, I hope it turned out this complicated because
> | of some oversight by me or my lack of c++ knowledge. Please comment on
> | it.
> |
> | ---- end of quote.
> | as you can see, graphics object & base_graphics_object also need to be
> | aware of the properties in the scheme that I ended up with. So just
> | moving gh_manager to the end of the declarations would not do.
>
> How about the following changes?

I like it. I was doing some very ugly hacks in order to avoid the use
of RTTI -- I was somehow under the impression that RTTI is a "no no"
in octave. using RTTI makes it much nicer.

The one thing I didn't like at first is the set_XXX method:
Until this morning I though it would be best not to allow direct
change of properties not through the normal set(h,name,val) functions.
This ensures that properties are checked for errors, and while it does
carry a performance hit over direct access, the graphics backend does
not need to set many values. However, this morning I remembered that
thery are some  properties which are "read-only" -- informative
properties such as mouse position etc... The easiest way to implement
read-only properties is not to add them into the XXX::set function,
but to add them to the XXX::get function. This dictates that the
backend will have direct access to setting the methods not through the
set function.

So I think this patch is good.

I can now  go on with the backend

Thanks,
Shai

> Here is an example of how the new functions can be used:
>
>   DEFUN (doit, args, , "")
>   {
>     octave_value retval;
>
>     if (args.length () == 1)
>       {
>         graphics_handle h = octave_NaN;
>
>         double val = args(0).double_value ();
>
>         if (! error_state)
>           {
>             h = gh_manager::lookup (val);
>
>             if (! xisnan (h))
>               {
>                 graphics_object obj = gh_manager::get_object (h);
>
>                 if (obj.isa ("line"))
>                   {
>                     line::line_properties& lp
>                       = dynamic_cast<line::line_properties&> 
> (obj.get_properties ());
>                     retval = lp.get_xdata ();
>                     Matrix m (1, 5);
>                     m(0) = 0;
>                     m(1) = 1;
>                     m(2) = 2;
>                     m(3) = 3;
>                     m(4) = 4;
>                     lp.set_xdata (m);
>                     m(0) = 0;
>                     m(1) = 1;
>                     m(2) = 0.2;
>                     m(3) = 0.8;
>                     m(4) = 0.4;
>                     lp.set_ydata (m);
>
>                     feval ("__request_drawnow__");
>                   }
>                 else
>                   error ("doit: looking for line");
>               }
>             else
>               error ("doit: invalid graphics object (= %g)", val);
>           }
>         else
>           error ("doit: invalid graphics object");
>       }
>     else
>       print_usage ();
>
>     return retval;
>   }
>
> Using this function, try something like
>
>   h = line ();
>   doit (h);
>
> It will return the original xdata property from the handle and also
> modify the xdata and ydata properties for the object.  Instead of
> using the graphics_object "isa" method, we could use run-time type
> identification, but I don't think that detail matters much.
>
> jwe
>
>
>
> 2007-08-28  John W. Eaton  <address@hidden>
>
>         * graphics.h (OCTAVE_GRAPHICS_PROPERTY_INTERNAL): Also define
>         set_X functions for properties.
>         (base_properties): Move class definition before definition of
>         base_graphics_object class.  Provide forward declaration of
>         base_graphics_object prior to definition of base_properties.
>         (base_graphics_object::get_properties): New virtual function.
>         (graphics_object::get_properties, root_figure::get_properties,
>         figure::get_properties, axes::get_properties,
>         line::get_properties, text::get_properties, image::get_properties,
>         patch::get_properties, surface::get_properties): New functions.
>         (root_figure::properties, figure::properties, axes::properties,
>         line::properties, text::properties, image::properties,
>         patch::properties, surface::properties): Data member now private.
>
>
> Index: src/graphics.h
> ===================================================================
> RCS file: /cvs/octave/src/graphics.h,v
> retrieving revision 1.11
> diff -u -u -r1.11 graphics.h
> --- src/graphics.h      27 Aug 2007 19:50:23 -0000      1.11
> +++ src/graphics.h      28 Aug 2007 21:20:07 -0000
> @@ -40,8 +40,11 @@
>  #include "ov.h"
>
>  #define OCTAVE_GRAPHICS_PROPERTY_INTERNAL(TAG, TYPE, NAME) \
> -  private: TAG TYPE NAME; \
> -  public: const TYPE& get_ ## NAME () const { return NAME; } \
> +  private: \
> +    TAG TYPE NAME; \
> +  public: \
> +    const TYPE& get_ ## NAME () const { return NAME; } \
> +    void set_ ## NAME (const TYPE& val) { NAME = val; mark_modified (); } \
>    private:
>
>  #define OCTAVE_GRAPHICS_PROPERTY(TYPE, NAME) \
> @@ -421,6 +424,57 @@
>
>  // ---------------------------------------------------------------------
>
> +class base_graphics_object;
> +
> +class base_properties
> +{
> +public:
> +  base_properties (const std::string& t = "unknown",
> +                  const graphics_handle& mh = octave_NaN,
> +                  const graphics_handle& p = octave_NaN)
> +    : type (t), __modified__ (true), __myhandle__ (mh), parent (p),
> +      children () { }
> +
> +  virtual ~base_properties (void) { }
> +
> +  virtual std::string graphics_object_name (void) const { return "unknonwn"; 
> }
> +
> +  void mark_modified (void);
> +
> +  void override_defaults (base_graphics_object& obj);
> +
> +  // Look through DEFAULTS for properties with given CLASS_NAME, and
> +  // apply them to the current object with set (virtual method).
> +
> +  void set_from_list (base_graphics_object& obj, property_list& defaults);
> +
> +  virtual void set (const property_name&, const octave_value&) { }
> +
> +  graphics_handle get_parent (void) const { return parent; }
> +
> +  void remove_child (const graphics_handle& h);
> +
> +  void adopt (const graphics_handle& h)
> +  {
> +    octave_idx_type n = children.numel ();
> +    children.resize (1, n+1);
> +    children(n) = h;
> +  }
> +
> +  void set_parent (const octave_value& val);
> +
> +  void reparent (const graphics_handle& new_parent) { parent = new_parent; }
> +
> +  virtual void delete_children (void);
> +
> +protected:
> +  std::string type;
> +  bool __modified__;
> +  graphics_handle __myhandle__;
> +  graphics_handle parent;
> +  Matrix children;
> +};
> +
>  class base_graphics_object
>  {
>  public:
> @@ -511,6 +565,13 @@
>      error ("base_graphics_object::default: invalid graphics object");
>    }
>
> +  virtual base_properties& get_properties (void)
> +  {
> +    static base_properties properties;
> +    error ("base_graphics_object::get_properties: invalid graphics object");
> +    return properties;
> +  }
> +
>    virtual bool valid_object (void) const { return false; }
>
>    virtual std::string type (void) const { return "unknown"; }
> @@ -625,6 +686,8 @@
>
>    bool isa (const std::string& go_name) const { return rep->isa (go_name); }
>
> +  base_properties& get_properties (void) { return rep->get_properties (); }
> +
>    bool valid_object (void) const { return rep->valid_object (); }
>
>    operator bool (void) const { return rep->valid_object (); }
> @@ -633,55 +696,6 @@
>    base_graphics_object *rep;
>  };
>
> -class base_properties
> -{
> -public:
> -  base_properties (const std::string& t = "unknown",
> -                  const graphics_handle& mh = octave_NaN,
> -                  const graphics_handle& p = octave_NaN)
> -    : type (t), __modified__ (true), __myhandle__ (mh), parent (p),
> -      children () { }
> -
> -  virtual ~base_properties (void) { }
> -
> -  virtual std::string graphics_object_name (void) const = 0;
> -
> -  void mark_modified (void);
> -
> -  void override_defaults (base_graphics_object& obj);
> -
> -  // Look through DEFAULTS for properties with given CLASS_NAME, and
> -  // apply them to the current object with set (virtual method).
> -
> -  void set_from_list (base_graphics_object& obj, property_list& defaults);
> -
> -  virtual void set (const property_name& name, const octave_value& val) = 0;
> -
> -  graphics_handle get_parent (void) const { return parent; }
> -
> -  void remove_child (const graphics_handle& h);
> -
> -  void adopt (const graphics_handle& h)
> -  {
> -    octave_idx_type n = children.numel ();
> -    children.resize (1, n+1);
> -    children(n) = h;
> -  }
> -
> -  void set_parent (const octave_value& val);
> -
> -  void reparent (const graphics_handle& new_parent) { parent = new_parent; }
> -
> -  virtual void delete_children (void);
> -
> -protected:
> -  std::string type;
> -  bool __modified__;
> -  graphics_handle __myhandle__;
> -  graphics_handle parent;
> -  Matrix children;
> -};
> -
>  // ---------------------------------------------------------------------
>
>  class root_figure : public base_graphics_object
> @@ -713,6 +727,7 @@
>      static std::string go_name;
>    };
>
> +private:
>    root_figure_properties properties;
>
>  public:
> @@ -808,6 +823,8 @@
>
>    void reparent (const graphics_handle& np) { properties.reparent (np); }
>
> +  base_properties& get_properties (void) { return properties; }
> +
>    bool valid_object (void) const { return true; }
>
>  private:
> @@ -854,6 +871,7 @@
>      static std::string go_name;
>    };
>
> +private:
>    figure_properties properties;
>
>  public:
> @@ -935,6 +953,8 @@
>
>    void reparent (const graphics_handle& np) { properties.reparent (np); }
>
> +  base_properties& get_properties (void) { return properties; }
> +
>    bool valid_object (void) const { return true; }
>
>  private:
> @@ -1023,6 +1043,7 @@
>      static std::string go_name;
>    };
>
> +private:
>    axes_properties properties;
>
>  public:
> @@ -1106,6 +1127,8 @@
>
>    void reparent (const graphics_handle& np) { properties.reparent (np); }
>
> +  base_properties& get_properties (void) { return properties; }
> +
>    bool valid_object (void) const { return true; }
>
>  private:
> @@ -1154,6 +1177,7 @@
>      static std::string go_name;
>    };
>
> +private:
>    line_properties properties;
>
>  public:
> @@ -1204,6 +1228,8 @@
>
>    void reparent (const graphics_handle& h) { properties.reparent (h); }
>
> +  base_properties& get_properties (void) { return properties; }
> +
>    bool valid_object (void) const { return true; }
>  };
>
> @@ -1240,6 +1266,7 @@
>      static std::string go_name;
>    };
>
> +private:
>    text_properties properties;
>
>  public:
> @@ -1290,6 +1317,8 @@
>
>    void reparent (const graphics_handle& h) { properties.reparent (h); }
>
> +  base_properties& get_properties (void) { return properties; }
> +
>    bool valid_object (void) const { return true; }
>  };
>
> @@ -1323,6 +1352,7 @@
>      static std::string go_name;
>    };
>
> +private:
>    image_properties properties;
>
>  public:
> @@ -1373,6 +1403,8 @@
>
>    void reparent (const graphics_handle& h) { properties.reparent (h); }
>
> +  base_properties& get_properties (void) { return properties; }
> +
>    bool valid_object (void) const { return true; }
>  };
>
> @@ -1416,6 +1448,7 @@
>      static std::string go_name;
>    };
>
> +private:
>    patch_properties properties;
>
>  public:
> @@ -1466,6 +1499,8 @@
>
>    void reparent (const graphics_handle& h) { properties.reparent (h); }
>
> +  base_properties& get_properties (void) { return properties; }
> +
>    bool valid_object (void) const { return true; }
>  };
>
> @@ -1500,6 +1535,7 @@
>      static std::string go_name;
>    };
>
> +private:
>    surface_properties properties;
>
>  public:
> @@ -1550,6 +1586,8 @@
>
>    void reparent (const graphics_handle& h) { properties.reparent (h); }
>
> +  base_properties& get_properties (void) { return properties; }
> +
>    bool valid_object (void) const { return true; }
>  };
>
>
>