lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Part-combine: Add a way to override the part-combination decision by a c


From: n . puttock
Subject: Part-combine: Add a way to override the part-combination decision by a ctx prop (issue1698054)
Date: Thu, 16 Sep 2010 19:59:00 +0000

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{ }

http://codereview.appspot.com/1698054/diff/2001/input/regression/part-combine-force.ly#newcode11
input/regression/part-combine-force.ly:11: \version "2.13.29"
"2.13.34"

http://codereview.appspot.com/1698054/diff/2001/input/regression/part-combine-force.ly#newcode13
input/regression/part-combine-force.ly:13: mI = \relative c' {
test \partcombineUnisono[Once] too?

http://codereview.appspot.com/1698054/diff/2001/input/regression/part-combine-force.ly#newcode14
input/regression/part-combine-force.ly:14: \partcombineApartOnce e4 e
\partcombineApartOnce c2 |
fix indentation

http://codereview.appspot.com/1698054/diff/2001/input/regression/part-combine-force.ly#newcode21
input/regression/part-combine-force.ly:21: c \partcombineChords c |
If this (conflicting) setting is deliberate, should mention error
message expected and add

(ly:set-option 'warning-as-error #f)

http://codereview.appspot.com/1698054/diff/2001/scm/define-context-properties.scm
File scm/define-context-properties.scm (right):

http://codereview.appspot.com/1698054/diff/2001/scm/define-context-properties.scm#newcode365
scm/define-context-properties.scm:365: combiner. Possible values are
#'apart, #'chords, #'unisono,
@code{#'apart}

etc.

http://codereview.appspot.com/1698054/diff/2001/scm/define-context-properties.scm#newcode367
scm/define-context-properties.scm:367: This property is handled entirely
different (in scm/part-combiner.scm)
Is this comment necessary here?

http://codereview.appspot.com/1698054/diff/2001/scm/define-context-properties.scm#newcode368
scm/define-context-properties.scm:368: than all other context
properties!")
If this message is staying, change the exclamation mark to a full stop.

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

http://codereview.appspot.com/1698054/diff/2001/scm/part-combiner.scm#newcode269
scm/part-combiner.scm:269: ;; Go through all moments recursively and
check if the events of that
The indentation throughout this new code is poor.

http://codereview.appspot.com/1698054/diff/2001/scm/part-combiner.scm#newcode278
scm/part-combiner.scm:278: (define (f? x)
needs a more descriptive name

http://codereview.appspot.com/1698054/diff/2001/scm/part-combiner.scm#newcode281
scm/part-combiner.scm:281: (equal? (ly:event-property x 'class)
'UnsetProperty))
(or (ly:in-event-class? x 'SetProperty)
    (ly:in-event-class? x 'UnsetProperty))

http://codereview.appspot.com/1698054/diff/2001/scm/part-combiner.scm#newcode291
scm/part-combiner.scm:291: (if (equal? (ly:event-property x 'class)
'UnsetProperty)
(ly:in-event-class? x 'UnsetProperty)

http://codereview.appspot.com/1698054/diff/2001/scm/part-combiner.scm#newcode296
scm/part-combiner.scm:296: (let* ((res (car (map new-val (reverse
evs)))))
let

http://codereview.appspot.com/1698054/diff/2001/scm/part-combiner.scm#newcode303
scm/part-combiner.scm:303: (let* ((val1 (car force1))
let

http://codereview.appspot.com/1698054/diff/2001/scm/part-combiner.scm#newcode308
scm/part-combiner.scm:308: ((equal? val1 val2) val1)
 ;; both voices have same value => easy
These comments make the lines too long; I think they'd be better off
above each line.

(same below in body of `analyse-forced-combine')

http://codereview.appspot.com/1698054/



reply via email to

[Prev in Thread] Current Thread [Next in Thread]