Re: AspectRatio Patches [changeset]

[Ben Abbott]

On Oct 28, 2010, at 8:27 PM, logari81 wrote:

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

To conform with Octave's code style, I've inserted a space between the 
functions names and the opening parenthesis.

John, is it ok to push?

Ben

aspectratios-changeset.patch
Description: Binary data