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

Issue 1698054: Part-combine: Add a way to override the part-combination decision by a ctx prop (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 11 months ago by Reinhold
Modified:
8 years, 7 months ago
Reviewers:
carl.d.sorensen, Neil Puttock
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

Part-combine: Add a way to override the part-combination decision Add functions partcombineApart[Once], partcombineChords[Once], partcombineUnisono[Once] and partcombineAutomatic to tweak the decision of the part-combiner. Internally, they are implemented as \once\set partcombineForced = #... \set partcombineForced = #... \unset partcombineForced The partcombineForced "context property" is not handled as a context property, but the \set events are rather handled directly in the part-combiner (scm/part-combine.scm, function analyse-forced-combine). As a result, you can't initialize a context to a given default value of partcombineForced, since the part-combiner never reads the context property itself, but tries to catch the corresponding \set/\unset events. For the *Once functions, we rely on the property_iterator to add a corresponding UnsetProperty event at the beginning of the next time step, though.

Patch Set 1 : Move the part-combine overriding to scm/part-combine.scm #

Total comments: 15

Patch Set 2 : Use a dedicated music event instead of abusing \set #

Patch Set 3 : Properly fix warnings about simultaneous events and about unhandled events #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+163 lines, -10 lines) Patch
M Documentation/changes.tely View 1 chunk +8 lines, -0 lines 0 comments Download
A input/regression/part-combine-force.ly View 1 2 1 chunk +29 lines, -0 lines 0 comments Download
A input/regression/part-combine-force-once.ly View 1 chunk +29 lines, -0 lines 0 comments Download
M ly/music-functions-init.ly View 1 2 1 chunk +18 lines, -0 lines 1 comment Download
M scm/define-event-classes.scm View 2 5 chunks +10 lines, -9 lines 0 comments Download
M scm/define-music-types.scm View 2 1 chunk +5 lines, -0 lines 0 comments Download
M scm/part-combiner.scm View 1 3 chunks +64 lines, -1 line 1 comment Download

Messages

Total messages: 2
Neil Puttock
LGTM. http://codereview.appspot.com/1698054/diff/2001/input/regression/part-combine-force.ly File input/regression/part-combine-force.ly (right): http://codereview.appspot.com/1698054/diff/2001/input/regression/part-combine-force.ly#newcode4 input/regression/part-combine-force.ly:4: \partcombineApart and \partcombineApartOnce are internally implemented as @code{ ...
8 years, 9 months ago (2010-09-16 19:59:00 UTC) #1
Carl
8 years, 8 months ago (2010-10-24 10:38:23 UTC) #2
LGTM

Carl

http://codereview.appspot.com/1698054/diff/17001/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/1698054/diff/17001/ly/music-functions-init.ly#n...
ly/music-functions-init.ly:602: #(define (symbol-or-boolean? x) (or (symbol? x)
(boolean? x)))
add to scm/c++.scm, and add entry to scm/lily.scm

http://codereview.appspot.com/1698054/diff/17001/scm/part-combiner.scm
File scm/part-combiner.scm (right):

http://codereview.appspot.com/1698054/diff/17001/scm/part-combiner.scm#newcod...
scm/part-combiner.scm:548: (analyse-forced-combine 0 #f)
U.S. spelling --> analyze

But I see that the standard in this file is analyse, so I'm not sure what the
best way to proceed is.
Sign in to reply to this message.

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