[Top][All Lists]
[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/
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Part-combine: Add a way to override the part-combination decision by a ctx prop (issue1698054),
n . puttock <=