Re: Fix 1382 (issue3100041)

[Carl . D . Sorensen]

On 2010/11/14 11:22:44, Valentin Villenave wrote:
> Hi Carl,

> looks good to me! I just have minor questions wrt coding style, but
these are
> mainly because of my own ignorance :)


http://codereview.appspot.com/3100041/diff/2001/input/regression/zero_staff_space.ly
> File input/regression/zero_staff_space.ly (right):


http://codereview.appspot.com/3100041/diff/2001/input/regression/zero_staff_space.ly#newcode12
> input/regression/zero_staff_space.ly:12: \override StaffSymbol
#'staff-space =
> #0.0
> Just a question: why did you chose to use "0.0" instead of plain "0"
here?

Because that was in the original bug report.  It doesn't need to be
there; I can change it to just zero.




http://codereview.appspot.com/3100041/diff/2001/lily/staff-symbol-referencer.cc
> File lily/staff-symbol-referencer.cc (right):


http://codereview.appspot.com/3100041/diff/2001/lily/staff-symbol-referencer.cc#newcode87
> lily/staff-symbol-referencer.cc:87: Real denom =
Staff_symbol::staff_space (st);
> Is there a reason why you're using "denom" instead of "denominator"?
From what I
> can see Lily's source code doesn't use this abbrev. anywhere else...


Sometimes it uses the abbreviation "den" for denominator.  But the
current rule says we shouldn't use abbreviations.  I'll fix it.



http://codereview.appspot.com/3100041/diff/2001/lily/staff-symbol-referencer.cc#newcode90
> lily/staff-symbol-referencer.cc:90: st->warning (_ ("staff-space is 0,
setting
> staff-position to 0"));
> I may be completely deluded here, but wouldn't it be cleaner to put
property
> names as %d variables in the warning string? This could make it easier
(for
> localization, etc.) if we were to change the property names in the
future.

I don't see how putting the names as variables helps anything.  The
whole string will need to be translated for localization, rather than
doing it one word at a time.

That being said, this warning generates a huge number of warnings, and
the code seems to do the right thing anyway, so I'm leaning toward
eliminating the warning.

Thanks,

Carl


http://codereview.appspot.com/3100041/