Narrowly target warnings for multiple slurs; Issue 1967

When \partcombine merges two streams into one voice, it can create duplicate slurs (with the same spanner-id) in these patterns
  { <c e>(( <d f>))  <c e>(( d) c( <d f>)) }
All should show a single merged slur.  We must end these slurs at the d, preferably without warning, but should still warn for other cases that are more likely input errors.

This is a re-implementation of the original patch for issue 1967, which broke partcombine.

[dak, 2012-06-03 10:08:54]

http://codereview.appspot.com/6272046/diff/5/input/regression/slur-multiple.ly
File input/regression/slur-multiple.ly (right):

http://codereview.appspot.com/6272046/diff/5/input/regression/slur-multiple.l...
input/regression/slur-multiple.ly:17: \relative c'' {
If you want a different test case, maybe add it to the end?  Otherwise one can't
judge the effect of your changes on the existing regtest.

http://codereview.appspot.com/6272046/diff/5/lily/slur-engraver.cc
File lily/slur-engraver.cc (right):

http://codereview.appspot.com/6272046/diff/5/lily/slur-engraver.cc#newcode185
lily/slur-engraver.cc:185: ended = true;
Why is this change going to work with double slurs?  Those have multiple start
events and only one end event.

http://codereview.appspot.com/6272046/diff/5/lily/slur-engraver.cc#newcode212
lily/slur-engraver.cc:212: else
What difference do you expect from this change?  it is not clear to me.

[Keith, 2012-06-04 06:54:18]

Patch Set 3 shows the revert of the original patch for issue 1967, so you can
use "delta from patch set 3" to see the re-implementation of the fix that
preserves the functionality of partcombine.

http://codereview.appspot.com/6272046/diff/5/input/regression/slur-multiple.ly
File input/regression/slur-multiple.ly (right):

http://codereview.appspot.com/6272046/diff/5/input/regression/slur-multiple.l...
input/regression/slur-multiple.ly:17: \relative c'' {
On 2012/06/03 10:08:54, dak wrote:
> If you want a different test case, maybe add it to the end? 
No, I cannot both fix issue 1967 and leave the old test case in place, because
the old test case generated a warning for c4(( which is a pattern generated by
partcombine.  I want to remove that warning, but this regtest expects warnings.

> Otherwise one can't
> judge the effect of your changes on the existing regtest.
I wiĺl put the old test output on the tracker.

http://codereview.appspot.com/6272046/diff/5/lily/slur-engraver.cc
File lily/slur-engraver.cc (right):

http://codereview.appspot.com/6272046/diff/5/lily/slur-engraver.cc#newcode185
lily/slur-engraver.cc:185: ended = true;
On 2012/06/03 10:08:54, dak wrote:
> Why is this change going to work with double slurs?  Those have multiple
start
> events and only one end event.

Double slurs have only one start event.  If the property doubleSlurs is true,
then two slurs are created from that one event (line 221ff), both both with the
same spanner-id.  Each slur is ended at their common end event, on successive
passes through this loop.

http://codereview.appspot.com/6272046/diff/5/lily/slur-engraver.cc#newcode212
lily/slur-engraver.cc:212: else
On 2012/06/03 10:08:54, dak wrote:
> What difference do you expect from this change?  it is not clear to me.

None.  This part of the patch is just a revert of the commit that caused the
regression.  I'll repost as a series.

[dak, 2012-06-05 12:04:36]

http://codereview.appspot.com/6272046/diff/9001/lily/slur-engraver.cc
File lily/slur-engraver.cc (right):

http://codereview.appspot.com/6272046/diff/9001/lily/slur-engraver.cc#newcode174
lily/slur-engraver.cc:174: bool ended = false;
Moving this out of the loop is simply wrong.  This will get a warning only if
_all_ closing events don't have an associated starting event regardless of the
spanner-id.  This needs to be done more carefully instead of just changing the
code until it stops giving warnings.

[Keith, 2012-06-06 06:24:22]

http://codereview.appspot.com/6272046/diff/9001/lily/slur-engraver.cc
File lily/slur-engraver.cc (right):

http://codereview.appspot.com/6272046/diff/9001/lily/slur-engraver.cc#newcode174
lily/slur-engraver.cc:174: bool ended = false;
On 2012/06/05 12:04:36, dak wrote:
> Moving this out of the loop is simply wrong.  This will get a warning only if
> _all_ closing events don't have an associated starting event regardless of
the
> spanner-id.  This needs to be done more carefully instead of just changing
the
> code until it stops giving warnings.

Thanks, I think.
We could simply move this initializer back where it was, and issue 1967 would
still be as "fixed" as much as it is in current master.

I think it would be clearest to have a separate loop through stop_events_ at
this point to remove redundant ')'s with duplicate 'spanner-id's.  Then that
whole loop could go when partcombine is improved. But I won't be running Linux
to test the code for a day or so.

[dak, 2012-06-06 18:50:01]

On 2012/06/06 06:24:22, Keith wrote:
> 
> Thanks, I think.

Not much cause for it, I am afraid.  You were right in the original "fix" from
me not having correctly guessed the scope of the problem, and your main idea of
just ignoring immediate duplicates was probably the most straightforward
approach.  I submitted a more thorough version of that approach (and tried
taking direction into account as well) and tried to keep the regtests
comparable.

But while I hijacked the credit for creating the "proper" fix of the code, the
credit for fixing my head in this case, arguably the harder task, lies with you.

Apologies for that bunch of pettiness.