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

Issue 6272046: Narrowly target warnings for multiple slurs; Issue 1967 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 3 months ago by Keith
Modified:
7 years, 3 months ago
Reviewers:
dak
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : phrasing slurs, too #

Total comments: 6

Patch Set 3 : revert b986b38f14 #

Patch Set 4 : same as patch set 2 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+27 lines, -36 lines) Patch
M input/regression/phrasing-slur-multiple.ly View 1 3 1 chunk +1 line, -1 line 0 comments Download
M input/regression/slur-multiple.ly View 3 1 chunk +3 lines, -3 lines 0 comments Download
M lily/phrasing-slur-engraver.cc View 3 3 chunks +3 lines, -2 lines 0 comments Download
M lily/slur-engraver.cc View 3 3 chunks +20 lines, -30 lines 2 comments Download

Messages

Total messages: 5
dak
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.ly#newcode17 input/regression/slur-multiple.ly:17: \relative c'' { If you want a different test ...
7 years, 3 months ago (2012-06-03 10:08:54 UTC) #1
Keith
Patch Set 3 shows the revert of the original patch for issue 1967, so you ...
7 years, 3 months ago (2012-06-04 06:54:18 UTC) #2
dak
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 ...
7 years, 3 months ago (2012-06-05 12:04:36 UTC) #3
Keith
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: ...
7 years, 3 months ago (2012-06-06 06:24:22 UTC) #4
dak
7 years, 3 months ago (2012-06-06 18:50:01 UTC) #5
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.
Sign in to reply to this message.

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