Re: AspectRatio Patches [changeset]

[logari81]

On Thu, 2010-10-28 at 03:31 -0400, John W. Eaton wrote: 
> 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.
> 

thank you for this template, in the attached changeset I have completed
it with (hopefully) all necessary information.

> 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?
> 
Please check my new version in the attached changeset. I think it is a
bit easier to read but I am not sure if the function definitions and
names are optimal. Any hints are welcome.

> | +          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.
> 
ok, I hadn't noticed it. It is also fixed in the attached changeset.

> 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";
> 
I thought "update_aspectratiors" is a typo, isn't it? Anyway I left this
change out.

> 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.
> 
Ok, I have removed it. I can send a separate changeset for this later.

> Thanks,
> 
> jwe

I would be glad for any further suggestions/corrections.

Best regards,

Kostas

aspectratios.changeset
Description: Text document