Fix 1382

Fix 1382

[Carl, 2010-11-14 00:49:38]

Here's a fix for Issue 1382.  It sets the staff position to zero if staff space
is zero, which is a consistent outcome -- all the staff lines are in the same
position so zero works.

Please review.

Thanks,

Carl

[Valentin Villenave, 2010-11-14 11:22:44]

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_s...
File input/regression/zero_staff_space.ly (right):

http://codereview.appspot.com/3100041/diff/2001/input/regression/zero_staff_s...
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?

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

http://codereview.appspot.com/3100041/diff/2001/lily/staff-symbol-referencer....
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.

[Carl, 2010-11-14 14:17:19]

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_s...
> File input/regression/zero_staff_space.ly (right):
> 
>
http://codereview.appspot.com/3100041/diff/2001/input/regression/zero_staff_s...
> 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....
> 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....
> 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

[Valentin Villenave, 2010-11-14 14:32:48]

On Sun, Nov 14, 2010 at 3:17 PM,  <Carl.D.Sorensen@gmail.com> wrote:
> 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.

What I meant is that if we ever decide to change the property names
again, then it might be more convenient if the old property names
weren't hardcoded in the string. That being said, I'm not sure either
that a warning is needed here at all.

Thanks!

Valentin.

[Graham Percival (old account), 2010-11-15 15:28:17]

On 2010/11/14 00:49:38, Carl wrote:
> Here's a fix for Issue 1382.  It sets the staff position to zero if staff
space
> is zero, which is a consistent outcome -- all the staff lines are in the same
> position so zero works.

Looks good, and the regtest comparison looks fine.  Please push.

- Graham