Re: [PATCH] Implement new handling for margin and line-width settings.

[Carl Sorensen]

Thanks for the reply Michael.

Usually we like to keep responses to reviews on the -devel list, so I've
copied this there.


On 8/11/09 5:51 PM, "Michael Käppler" <address@hidden> wrote:

> Hi Carl,
> thanks for your review:
>>      Don't use lmargin, rmargin, and lwidth as variable names.  LilyPond
>> standards call for using full words in variable names.  You might want to
>> use something like scm_left_margin, scm_right_margin, and scm_line_width.
>>  
> Fixed. I used lmargin etc. because I saw it in layout->page-init as a
> temporary variable, therefore I thought there would be nothing against it.

Yes, we have old code that uses that kind of variable names.  As we rewrite,
we're supposed to go to full-word names.

>> I'd prefer the variable name consistent, instead of consistency
>>  
> Fixed.
>>> +
>>> + // Consistency checks. FIXME: Print warnings just once
>>>    
>> 
>> This would be easy to add by putting the test in an if/elseif/endif block.
>>  
> Hmm... I don't really understand this. The problem is that
> foo->normalize () is called every time an Output_def is needed to get
> information about line-width and/or margins. So if one sets wrong values
> in \paper {} the function displays the error several times. But I
> decided to not overwrite the original paper settings during rendering,
> because maybe the original settings are later needed again. (However,
> I'm pretty unsure of this)


Oh -- I misunderstood the meaning of the FIXME.

I think I'd recommend changing the settings to something that is rational.
Since we're using the changed settings, we might as well change them so we
don't keep getting errors.

>> It might be cleaner to define a function set_layout_props (line_width,
>> left_margin, right_margin) and then call it with different arguments.  The
>> function call could be within the if/elseif/endif block described above,
>> and then there would be no need for the variable consistency.
>>  
> See above.

I still think my suggestion for avoiding the variable consistency is a good
one.  I have very rough pseudocode below

if (paper_width > (line_width + left_margin + right_margin))
  { 
     warning ("paper not wide enough for margins + line");
     set_paper_layout (default_left_margin, default_right_margin,
default_line_width);
  }
else if ((left_margin < 0) || (right_margin < 0))
  {
     warning ("right or left margin has a negative value");
     set_paper_layout (default_left_margin, default_right_margin,
default_line_width);
  }
else
  set_paper_layout (left_margin, right_margin, line_width);


You could even be smarter in your choice of margins and line-widths to set.

For example, if left_margin > 0 and right_margin <0, you could do

set_paper_layout (left_margin, line_width + right_margin, 0)

or something like that.


None of my suggestions are mandatory.  They're just suggestions.

Thanks for taking on this task!  It will be good for LilyPond users in the
future.

Carl