reopened Issue 2584: please make partcombine merge slurs

reopened Issue 2584: please make partcombine merge slurs

This refrains from referencing a slur grob's direction field (often
inconclusive and not representative of the input), instead referencing
the direction of the causing event.

[Trevor Daniels, 2012-07-19 09:30:15]

Just nitpicking; can't comment substantively.

Trevor

http://codereview.appspot.com/6432047/diff/1/lily/phrasing-slur-engraver.cc
File lily/phrasing-slur-engraver.cc (right):

http://codereview.appspot.com/6432047/diff/1/lily/phrasing-slur-engraver.cc#n...
lily/phrasing-slur-engraver.cc:256: slurs_[j]->suicide ();
no tabs please

http://codereview.appspot.com/6432047/diff/1/lily/phrasing-slur-engraver.cc#n...
lily/phrasing-slur-engraver.cc:258: continue;
no tabs please

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

http://codereview.appspot.com/6432047/diff/1/lily/slur-engraver.cc#newcode257
lily/slur-engraver.cc:257: slurs_[j]->suicide ();
no tabs please

http://codereview.appspot.com/6432047/diff/1/lily/slur-engraver.cc#newcode259
lily/slur-engraver.cc:259: continue;
no tabs please

[dak, 2012-07-19 10:27:29]

On 2012/07/19 09:30:15, Trevor Daniels wrote:
> Just nitpicking; can't comment substantively.
> 
> Trevor
> 
> http://codereview.appspot.com/6432047/diff/1/lily/phrasing-slur-engraver.cc
> File lily/phrasing-slur-engraver.cc (right):
> 
>
http://codereview.appspot.com/6432047/diff/1/lily/phrasing-slur-engraver.cc#n...
> lily/phrasing-slur-engraver.cc:256: slurs_[j]->suicide ();
> no tabs please
> 
>
http://codereview.appspot.com/6432047/diff/1/lily/phrasing-slur-engraver.cc#n...
> lily/phrasing-slur-engraver.cc:258: continue;
> no tabs please
> 
> http://codereview.appspot.com/6432047/diff/1/lily/slur-engraver.cc
> File lily/slur-engraver.cc (right):
> 
> http://codereview.appspot.com/6432047/diff/1/lily/slur-engraver.cc#newcode257
> lily/slur-engraver.cc:257: slurs_[j]->suicide ();
> no tabs please
> 
> http://codereview.appspot.com/6432047/diff/1/lily/slur-engraver.cc#newcode259
> lily/slur-engraver.cc:259: continue;
> no tabs please

Ran M-x untabify RET on both files, but won't recommit to testing for just this
spacing change.

[Keith, 2012-07-19 17:12:21]

LGTM
Again, I suggest first committing the code in a state that merely fixes the
reported bugs about warnings, and then a commit to add the new capability to set
two slurs if they have opposite directions.

[dak, 2012-07-19 17:25:45]

On 2012/07/19 17:12:21, Keith wrote:
> LGTM
> Again, I suggest first committing the code in a state that merely fixes the
> reported bugs about warnings, and then a commit to add the new capability to
set
> two slurs if they have opposite directions.

Again, I state that the "new capability" is a side-effect of the rewritten
functionality.  One could remove line 264 in slur.cc (the if-statement making
the break conditional) to remove this functionality, but readding that line in a
separate commit would be rather artificial and make little sense as it does not
change the overall logic of the code but just makes a different design decision,
and there is no point in first making the less convenient decision.

[Keith, 2012-07-19 17:52:13]

On 2012/07/19 17:25:45, dak wrote:
> On 2012/07/19 17:12:21, Keith wrote:
> > Again, I suggest first committing the code in a state that 
> > merely fixes the reported bugs about warnings, 

>  One could remove line 264 in slur.cc 

Simpler to finish the loop over j at line 227, similarly to the ver2.14 code,
with

  if (j < old_slurs)
    ev->origin ()->warning (_ ("already have slur"));
  start_events_.erase (start_events_.begin () + i);
  break;
}

> and there is no point in first making the less convenient decision.

The point is to separate the fix to one regression (1967) from code that causes
another regression (2584).  Linked strings of regressions make it /look/ more
difficult than it really is to fall back to a state free of known regressions.

[dak, 2012-07-19 18:09:34]

On 2012/07/19 17:52:13, Keith wrote:
> On 2012/07/19 17:25:45, dak wrote:
> > On 2012/07/19 17:12:21, Keith wrote:
> > > Again, I suggest first committing the code in a state that 
> > > merely fixes the reported bugs about warnings, 
> 
> >  One could remove line 264 in slur.cc 
> 
> Simpler to finish the loop over j at line 227, similarly to the ver2.14 code,

That's not equivalent.

> with
> 
>   if (j < old_slurs)
>     ev->origin ()->warning (_ ("already have slur"));
>   start_events_.erase (start_events_.begin () + i);
>   break;
> }
> 
> > and there is no point in first making the less convenient decision.
> 
> The point is to separate the fix to one regression (1967) from code that
causes
> another regression (2584).

But there is no code in the current patch that would fix one and not the other. 
You are asking for artificially designing something doing only half the job, and
fitting it in between.

> Linked strings of regressions make it /look/ more
> difficult than it really is to fall back to a state free of known regressions.

But there is nothing won by designing and writing artificially half-broken code
that exists for no other purpose than being only half-broken.  For the current
code design, there is no version that would be a logical intermediate step,
fixing only one regression, and using the original code in a code path causing
the other regression to be unfixed.

Your proposal does not even make sense without committing a sequence of commits
starting with _reverting_ the original fix, then doing a version changing the
warning, then committing the state after the original fix _again_, then
committing the current fix.

If you really want that, create an issue with all those steps except the last
(resulting in a net patch changing nothing when all steps are combined) and
block this issue on it.  Once your issue passes, you can commit the sequence of
patches (making sure that each of those has the intended state), leaving the
work tree in the same state, and then I can commit the final patch of this issue
on top.

I consider that fabulously pointless, but if it is important to you, that would
be the way to proceed.