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" <xmichae...@web.de> 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
_______________________________________________
lilypond-devel mailing list
lilypond-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/lilypond-devel
