[Octave-bug-tracker] [bug #37290] gnuplot / demo plotyy followed by sombrero errors out

[Dan Sebald]

Follow-up Comment #32, bug #37290 (project octave):

Having thought about this for a few days, I've concluded there's too much work
here for me to undertake right now.  But I do think there is a potentially
great summer student project here, or maybe a group effort code sprint for
OctConf 13.  I'll summarize why at the end.

There isn't anything obvious to me that is a simple solution to make this work
the way I understand it should work.  The things I initially thought were
problematic are actually sort of correct, if perhaps some minor fixes here and
there.  (I was kind of thinking about things differently on first
inspection.)

John, the attached changeset is probably the best I can do for now.  It
doesn't really fix things, more like the old-college-try; might even make
things worse.  You can maybe improve on this, but I don't want to spend too
much more effort on it because I'm thinking there isn't a good short-term
solution.  Basically, the patch resets properties to the factory settings, but
it is a little tricky because properties are defined within the derived
graphic object classes (not the base graphics object) which means that each
graphics object needs to have its own member function for accessing those
properties.  Hence, there is some code duplication.  I'm not really happy with
the solution, as it sort of works its way in the direction of the children,
when I think the more natural hierarchy direction is in the direction of the
parent.  The reason some things break is because factory settings apparently
aren't always good for the graphics system.

I know that the intermediate "defaultXYZ" isn't (and wasn't) working because
there is this bug that currently exists:


>> set(gca, "defaultlinewidth", 2.3)

error: invalid line property 'width'


I've fixed that so that at least there is no longer an error message, but I'm
sure the results are dodgy because I didn't do much beyond that.

Also, I don't think there is a way to get rid of the error message about
unknown properties when they are readonly ('r' in genprops.awk).  For
example:


>> reset(hax)
error: set: unknown property "tightinset"


The reason is that message is generated at the end of a long if-else
statement.  The valid settable properties is contained within the following
if-else structure and is inaccessible


  const std::set<std::string>& pnames = xproperties.all_property_names ();
  for (p = pm.begin (); p != pm.end (); p++)
    {
      // Read-only properties (i.e., 'r' described in genprops.awk)
      // do not appear within the test structure for valid properties
      // (i.e., can only get read-only props, not set them).  So,
      // first check to validate it is present to avoid an error
      // message.
      caseless_str pname (p->first);
      pname = validate_property_name ("get",
                                      xproperties.graphics_object_name (),
                                      pnames, pname);


The above is the equivalent of "have_property" without getting the list of
properties every time a test is desired.  However, that includes all
properties, and does not distinguish if the property can be set. Probably the
easiest way to fix this is to change the genprops.awk script to calling
read-only set_XYZ() but make the associated function empty, i.e., do nothing.

Note further that some of the error messages actually pertain to bad values
for the factory settings.  For example:


error: invalid color specification: {auto}|none
error: invalid value for color property "markeredgecolor" (value=
{auto}|none)
error: invalid color specification: auto|{none}
error: invalid value for color property "markerfacecolor" (value=
auto|{none})


Anyway, what I've done is put into the "set" and "reset" documentation a note
stating we are aware of some problems therefore don't submit a bug report. 
That way, we can close out this bug report and bug report
https://savannah.gnu.org/bugs/?35511 with an eye toward a student project for
the release after the June release.

If John can't improve on the attached patch, maybe we should just leave it as
is, with some added documentation for "reset" about known bugs.  My opinion is
that any more effort to patch things isn't going to amount to much.  I may be
wrong.

...

I think conceptually, on the whole, the property management scheme isn't
falling into place quite right.  Now, that's not to say the code has wandered
off course.  The individual parts are all about right, but it's as though
organizing things in a slightly different way would in some conceptual ways
and some programming ways simplify matters.  I'll give an example.

I like the use of std::map, and there are macro definitions for all the
possible properties.  Furthermore, there are some member functions for
determining if a certain graphics object has the property specified. And the
property_list is nice, i.e., all these graphics objects have a list containing
properties set by the user.  But there is a sort of disconnect between the
organization of the properties and the way they are referenced by Octave code.
 For example, here is the creation of factory properties:


property_list::plist_map_type
root_figure::init_factory_properties (void)
{
  property_list::plist_map_type plist_map;

  plist_map["figure"] = figure::properties::factory_defaults ();
  plist_map["axes"] = axes::properties::factory_defaults ();
  plist_map["line"] = line::properties::factory_defaults ();

etc.
}


The groupings makes sense, but it unfortunately doesn't retain any
hierarchical information.  If things were organized to retain hierarchy, then
perhaps the algorithm that does a "get" on the desired variable might be
somewhat simple and abstract.  Ben describes how the default syntax works,
e.g., "defaultfigureaxeslinelinewidth" at the root_figure level,
"defaultaxeslinelinewidth" at the figure level, "defaultlinelinewidth" at the
axes level, if I'm understanding correctly.  What if those names were used as
the index names for the map list?  Here's a sample.  Say the desired property
is the linewidth of some line:

dprop = "linewidth"

level 1 (line) Is there a dprop in my user_set_plist_map[]?  If yes, get
user_set_plist_map[dprop].  If no, add prefix "line" to create dprop = "line"
+ dprop.  Pass dprop to the parent get routine.

level 2 (axes) Is there a dprop in my user_set_plist_map[]?  If yes, get
user_set_plist_map[dprop].  If no, add prefix "axes" to create dprop = "axes"
+ dprop.  Pass dprop to the parent get routine.

level 3 (figure) Is there a dprop in my user_set_plist_map[]?  If yes, get
user_set_plist_map[dprop].  If no, add prefix "figure" to create dprop =
"figure" + dprop.  Pass dprop to the parent get routine.

level 4 (root_figure) Is there a dprop in my user_set_plist_map[]?  If yes,
get user_set_plist_map[dprop].  If no, get factory_plist_map[dprop].

So, at the point of reaching level 4, the index for the map list becomes
"figureaxeslinelinewidth".  So, that name coincides with
"defaultfigureaxeslinelinewidth" and at the same time utilizes a very generic
routine that is almost the same at all levels.  I left the "line", "axes", and
"figure" in for clarity, but really that could be replaced by an identity
variable, say gobjid.  With that, the routine is the same at all levels,
except the root figure level.

One side note: A map with so many indeces might get kind of slow, especially
at the root level.  The map could also be done in a hierarchical fashion
(similar to multiple levels it currently is), and processing could be made a
bit easier by using a delimiter behind the scenes, e.g.,
"figure:axes:line:linewidth".  That would make parsing the map indeces pretty
straightforward.  When it comes time to displaying this parameter, the
delimiter symbols could be extracted. So, one could use probably the internal
regexp routine to get a cell array consisting of {figure, axes, line,
linewidth} and access the property via

user_set_plist_map[p{1}].m[p{2}].m[p{3}].m[p{4}]

I'm guessing that is much faster than trying to index a map list with possibly
hundreds of entries.  But above the root_figure level, the parsing probably
isn't necessary because there won't be more than a dozen user defined
properties.

Well anyway, I hope I've conveyed something that might give John and others
some ideas for what I think could be a nice student project.  The reasons I
think it's good are:

1) Many of the pieces are already worked out in the current code, so the
student wouldn't have to work from a clean slate and could learn a lot by just
studying the code for a week or two.

2) It uses some nice standardized features like std::map.  That's something a
computer science/software engineer would benefit from learning.

3) The project is somewhat substantial in effort, but not huge.

4) The coding is very object oriented, using things like function inheritance
and/or virtual functions.

5) The structure has a hierarchy nature to it.  This feels like it is in the
realm of database management, so any student interested in that field could
really benefit from a project like this.

I could maybe mentor such a thing, but I won't have much time after the end of
May.


(file #28111)
    _______________________________________________________

Additional Item Attachment:

File name: octave-graphics_reset-2013may19.patch Size:35 KB


    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?37290>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/