Re: AspectRatio Patches [changeset]

[John W. Eaton]

On 28-Oct-2010, logari81 wrote:

| thank you for reviewing the patches. The changelog entry seems to be ok.

The entry is just

| +2010-10-28  Konstantinos Poulios <address@hidden>
| +     * Fix the dataspectratio and plotboxaspectratio for the fltk
| +     backend.
| +

but it should really say which functions changed, and what changed.
Something like:

2010-10-28  Konstantinos Poulios  <address@hidden>

        * graphics.cc (updating_aspectratios): New file-scope variable.
        (axes::properties::update_aspectratios, axes::update_axis_limits):
        Return immediately if updating_aspecratios is true.
        (axes::properties::update_aspectratios):
        Some explanation of the other changes to this function
        (PLEASE ADD; I don't know what they are about).
        * graphics.h.in (class axes::properties): Tag dataaspectratio,
        dataaspectratiomode, plotboxaspectratio, and
        plotboxaspectratiomode with "u" qualifier.
        (axes::update_dataaspectratio,
        axes::update_dataaspectratiomode,
        axes::update_plotboxaspectratio,
        axes::update_plotboxaspectratiomode): New functions.

There seems to be a lot of duplicated code in the new 
axes::properties::update_aspectratios function (it looks like
essentially the same block of code for x, y, and z.  How about writing
a function that could be called for the x, y, and z cases to eliminate
some of the duplication?

| +          s = ( zlimits(1) - zlimits(0) ) / ( pba(2) * da(2) );

In Octave code, please don't put spaces after '(' and before ')';
write

  s = (zlimits(1) - zlimits(0)) / (pba(2) * da(2));

instead.

This looks like an unintended change:

| @@ -3381,7 +3381,7 @@
|    linestyleorder = "-";
|    linewidth = 0.5;
|    minorgridlinestyle = ":";
| -  // Note: plotboxaspectratio will be set through update_aspectratiors
| +  // Note: plotboxaspectratio will be set through update_aspectratios
|    plotboxaspectratiomode = "auto";
|    projection = "orthographic";
|    tickdir = "in";

Also, what is this change doing in the changeset?

| diff --git a/src/genprops.awk b/src/genprops.awk
| --- a/src/genprops.awk
| +++ b/src/genprops.awk
| @@ -56,7 +56,8 @@
|  ##   }
|  ##
|  ## If present, the QUALIFIERS string may include any of the characters
| -## g, G, m, s, S, o, O, h, which have the following meanings:
| +## g, G, s, S, o, O, a, l, m, h, r, u, U, f which have the following
| +## meanings:
|  ##
|  ##   g:  There is a custom inline definition for the get function,
|  ##       so we don't emit one.

This change should probably be made if all those extra codes are
recognized by the genprops.awk script now, but it should probably be
in a separate changeset.

Thanks,

jwe