Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1767)

Issue 6432047: reopened Issue 2584: please make partcombine merge slurs (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 11 months ago by dak
Modified:
6 years, 11 months ago
Reviewers:
Keith, Trevor Daniels
CC:
lilypond-devel_gnu.org
Base URL:
http://git.savannah.gnu.org/gitweb/?p=lilypond.git/trunk/
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -36 lines) Patch
M lily/phrasing-slur-engraver.cc View 2 chunks +15 lines, -18 lines 2 comments Download
M lily/slur-engraver.cc View 2 chunks +15 lines, -18 lines 2 comments Download

Messages

Total messages: 6
Trevor Daniels
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#newcode256 lily/phrasing-slur-engraver.cc:256: slurs_[j]->suicide (); no ...
6 years, 11 months ago (2012-07-19 09:30:15 UTC) #1
dak
On 2012/07/19 09:30:15, Trevor Daniels wrote: > Just nitpicking; can't comment substantively. > > Trevor ...
6 years, 11 months ago (2012-07-19 10:27:29 UTC) #2
Keith
LGTM Again, I suggest first committing the code in a state that merely fixes the ...
6 years, 11 months ago (2012-07-19 17:12:21 UTC) #3
dak
On 2012/07/19 17:12:21, Keith wrote: > LGTM > Again, I suggest first committing the code ...
6 years, 11 months ago (2012-07-19 17:25:45 UTC) #4
Keith
On 2012/07/19 17:25:45, dak wrote: > On 2012/07/19 17:12:21, Keith wrote: > > Again, I ...
6 years, 11 months ago (2012-07-19 17:52:13 UTC) #5
dak
6 years, 11 months ago (2012-07-19 18:09:34 UTC) #6
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b