Re: Fix issues 1259 and 1433 (\breakDynamicSpan and a spanner's style=#'none over a line break) (issue4630070)

[reinhold . kainhofer]

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/