lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: line_count related patches in a single commit for review (issue 6419


From: k-ohara5a5a
Subject: Re: line_count related patches in a single commit for review (issue 6419064)
Date: Mon, 23 Jul 2012 20:40:03 +0000

`make check` succeeded for me, and I cannot see anything that would
cause a problem for the patchy script.

The code looks fine; regression tests have a minor problem.

I compared this with the previous patch at
http://codereview.appspot.com/6351107/diff2/2003:8001/lily/bar-line.cc


http://codereview.appspot.com/6419064/diff/1/input/regression/repeat-sign.ly
File input/regression/repeat-sign.ly (right):

http://codereview.appspot.com/6419064/diff/1/input/regression/repeat-sign.ly#newcode128
input/regression/repeat-sign.ly:128: \override StaffSymbol #'staff-space
= #0
This produces a lot of warnings (inf/nan in output) and the last
note-head collides with the repeat dots.   If you really want to set
music with staff-space = 0, you will need to protect every case of
division by staff_space through the code base.

Overall, this test is quite long in comparison to the function it is
testing.

http://codereview.appspot.com/6419064/diff/1/input/regression/staff-ledger-positions.ly
File input/regression/staff-ledger-positions.ly (right):

http://codereview.appspot.com/6419064/diff/1/input/regression/staff-ledger-positions.ly#newcode16
input/regression/staff-ledger-positions.ly:16: g,4 c e b' \bar ":|" c''
e g
This is a very difficult case to set nice-looking repeat dots, so don't
add the repeat bar to the test.  If the dots are correct we will spend
more time wondering about them, and if they change to something wrong we
might not be able to decide which is better.

http://codereview.appspot.com/6419064/



reply via email to

[Prev in Thread] Current Thread [Next in Thread]