Uploaded new patch that fixes all of Neil's concerns and clean up the regtests a bit. AFAICT, this should now be the final patch.
http://codereview.appspot.com/4630070/diff/17001/input/regression/dynamics-alignment-breaker-linebreak.ly File input/regression/dynamics-alignment-breaker-linebreak.ly (right): http://codereview.appspot.com/4630070/diff/17001/input/regression/dynamics-alignment-breaker-linebreak.ly#newcode5 input/regression/dynamics-alignment-breaker-linebreak.ly:5: dynamic spanner is across a line break. On 2011/07/21 18:48:50, Neil Puttock wrote:
crosses a line break
Changed, thanks. http://codereview.appspot.com/4630070/diff/17001/input/regression/dynamics-alignment-breaker-linebreak.ly#newcode9 input/regression/dynamics-alignment-breaker-linebreak.ly:9: % Both lines should give the same output, although the \breakDynamicSpan On 2011/07/21 18:48:50, Neil Puttock wrote:
should go in texidoc
Edit: actually, I'm a bit confused since there are three lines and
they're all
different
I have now completely redone the regtests, splitting them up into several smaller tests. In this test I was actually checking three different things at once (line breaks, exact position of \breakDyanmicSpan, and that the break does not have an effect on subsequent spanners). http://codereview.appspot.com/4630070/diff/17001/input/regression/dynamics-alignment-breaker.ly File input/regression/dynamics-alignment-breaker.ly (left): http://codereview.appspot.com/4630070/diff/17001/input/regression/dynamics-alignment-breaker.ly#oldcode12 input/regression/dynamics-alignment-breaker.ly:12: \dimTextDim Added that again, so make check should not show and difference to before. http://codereview.appspot.com/4630070/diff/17001/lily/dynamic-align-engraver.cc File lily/dynamic-align-engraver.cc (right): http://codereview.appspot.com/4630070/diff/17001/lily/dynamic-align-engraver.cc#newcode49 lily/dynamic-align-engraver.cc:49: Spanner *ended_line_; // Spanner was manually broken, create a new one, but store the old so we can properly finish it. On 2011/07/21 18:48:50, Neil Puttock wrote:
comment is a bit long
Shortened http://codereview.appspot.com/4630070/diff/17001/lily/dynamic-align-engraver.cc#newcode93 lily/dynamic-align-engraver.cc:93: programming_error ("Already have a force-ended DynamicLineSpanner."); On 2011/07/21 18:48:50, Neil Puttock wrote:
"already have a force-ended DynamicLineSpanner"
Changed http://codereview.appspot.com/4630070/diff/17001/lily/dynamic-align-engraver.cc#newcode134 lily/dynamic-align-engraver.cc:134: // TODO: Compare the direction of the existing spanner with On 2011/07/21 18:48:50, Neil Puttock wrote:
remove
(issue #1111)
Actually, I explicitly added it because of issue #1111 to mark the possible entry point for a fix. http://codereview.appspot.com/4630070/diff/17001/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/4630070/diff/17001/scm/define-grob-properties.scm#newcode780 scm/define-grob-properties.scm:780: (spanner-broken ,boolean? "Indicates whether dynamics spanner On 2011/07/21 18:48:50, Neil Puttock wrote:
internal?
Okay, made it internal.
I'd remove any reference to dynamics here, since this is generic
enough to be
useful elsewhere (e.g. pedals)
Good idea. http://codereview.appspot.com/4630070/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
