[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Woodwind diagrams (issue1425041)
From: |
n . puttock |
Subject: |
Re: Woodwind diagrams (issue1425041) |
Date: |
Mon, 31 May 2010 14:48:13 +0000 |
Reviewers: carl.d.sorensen_gmail.com, MikeSol,
Message:
Hi Mike,
This is super work, you're obviously a schemer extraordinaire. ;)
I've copied my comments from the original set, and added a few more
(you'll see some reiterate Carl's points).
I think woodwind-diagrams.scm is a bit unwieldy in its present form; it
would be easier to digest if the instrument alists were moved to a
separate file.
Cheers,
Neil
http://codereview.appspot.com/1425041/diff/1/2
File input/regression/woodwind-diagrams-empty.ly (right):
http://codereview.appspot.com/1425041/diff/1/2#newcode1
input/regression/woodwind-diagrams-empty.ly:1: \version "2.12.0"
"2.13.23"
(or latest development version when ready to apply)
http://codereview.appspot.com/1425041/diff/1/2#newcode12
input/regression/woodwind-diagrams-empty.ly:12: #'(1.0 0.1 #t ()) }}
#'(1.0 0.1 #t ())
}
}
etc.
http://codereview.appspot.com/1425041/diff/1/3
File input/regression/woodwind-diagrams-key-lists.ly (right):
http://codereview.appspot.com/1425041/diff/1/3#newcode1
input/regression/woodwind-diagrams-key-lists.ly:1: \version "2.12.0"
"2.13.23"
http://codereview.appspot.com/1425041/diff/1/8
File scm/lily-library.scm (right):
http://codereview.appspot.com/1425041/diff/1/8#newcode283
scm/lily-library.scm:283: (define-public (map-alist-keys function keys
alist)
this needs renaming: there's already a function called `map-alist-keys'
in this file
http://codereview.appspot.com/1425041/diff/1/8#newcode287
scm/lily-library.scm:287: @code{((a . -1) (b . 2) (c . 3) (d . 4))}"
the order's not preserved; I get the following output:
((b . 2) (a . -1) (c . 3) (d . 4))
http://codereview.appspot.com/1425041/diff/1/8#newcode289
scm/lily-library.scm:289: alist
(if (null? keys)
alist
(and all other `if' forms)
http://codereview.appspot.com/1425041/diff/1/8#newcode296
scm/lily-library.scm:296: (assoc-remove (car keys) alist)))
assoc-remove should be defined in this file, not in
woodwind-diagrams.scm
http://codereview.appspot.com/1425041/diff/1/8#newcode494
scm/lily-library.scm:494: (operator (interval-end operand) (interval-end
interval)))
(cons (operator (interval-start operand) (interval-start interval))
(operator (interval-end operand) (interval-end interval)))
(and all other cons pairs)
http://codereview.appspot.com/1425041/diff/1/8#newcode573
scm/lily-library.scm:573: (define-public (return-interval iv) iv)
= Guile's `identity' procedure
http://codereview.appspot.com/1425041/diff/1/8#newcode624
scm/lily-library.scm:624: (let* ((complex (make-polar
let
http://codereview.appspot.com/1425041/diff/1/10
File scm/output-ps.scm (right):
http://codereview.appspot.com/1425041/diff/1/10#newcode114
scm/output-ps.scm:114: (x1 x2 x3 x4 x5 x6)
(lambda (x1 x2 x3 x4 x5 x6)
(and all other lambda forms)
http://codereview.appspot.com/1425041/diff/1/11
File scm/output-svg.scm (right):
http://codereview.appspot.com/1425041/diff/1/11#newcode360
scm/output-svg.scm:360: (- new-start-angle (* TWO-PI (floor (/
new-start-angle TWO-PI)))))
move to a separate function rather than rebinding
(same for new-end-angle)
http://codereview.appspot.com/1425041/diff/1/11#newcode378
scm/output-svg.scm:378: (/
move to separate function to save duplication
http://codereview.appspot.com/1425041/diff/1/12
File scm/stencil.scm (right):
http://codereview.appspot.com/1425041/diff/1/12#newcode223
scm/stencil.scm:223: `(0.0 ,TWO-PI))))
insert space after this (and following other local defines)
http://codereview.appspot.com/1425041/diff/1/12#newcode249
scm/stencil.scm:249: (define (ordering-function-1 a b) (< (car a) (car
b)))
car< in lily-library.scm
http://codereview.appspot.com/1425041/diff/1/12#newcode250
scm/stencil.scm:250: (define (ordering-function-2 a b) (<= (car a) (car
b)))
rename car<= (could also be added to lily-library.scm)
http://codereview.appspot.com/1425041/diff/1/12#newcode331
scm/stencil.scm:331: ; Zeros of the bezier curve
;;
http://codereview.appspot.com/1425041/diff/1/12#newcode342
scm/stencil.scm:342: ; Apply L'hopital's rule to get the zeros if 0/0
;;
http://codereview.appspot.com/1425041/diff/1/12#newcode538
scm/stencil.scm:538: "Returns a function drawing an arrow from here to
@var{destination}."
doc start?/end?
http://codereview.appspot.com/1425041/diff/1/13
File scm/woodwind-diagrams.scm (right):
http://codereview.appspot.com/1425041/diff/1/13#newcode88
scm/woodwind-diagrams.scm:88: (if (member y input-list) #t #f)))
(pair? (memv y input-list))
http://codereview.appspot.com/1425041/diff/1/13#newcode117
scm/woodwind-diagrams.scm:117: (define (return-x x) x)
= identity
http://codereview.appspot.com/1425041/diff/1/13#newcode177
scm/woodwind-diagrams.scm:177: ;Makes the alist used to generate
woodwind-data-alist.
move inside function
http://codereview.appspot.com/1425041/diff/1/13#newcode234
scm/woodwind-diagrams.scm:234: (ly:stencil-in-color stencil 0.5 0.5
0.5))
(ly:stencil-in-color stencil grey))
http://codereview.appspot.com/1425041/diff/1/13#newcode420
scm/woodwind-diagrams.scm:420: (ly:text-interface::interpret-markup
interpret-markup
http://codereview.appspot.com/1425041/diff/1/13#newcode434
scm/woodwind-diagrams.scm:434: (ly:text-interface::interpret-markup
interpret-markup
http://codereview.appspot.com/1425041/diff/1/13#newcode799
scm/woodwind-diagrams.scm:799: (define (generate-flute-family-entry
flute-name)
I think it might be better to move these alists to a separate file.
http://codereview.appspot.com/1425041/diff/1/13#newcode805
scm/woodwind-diagrams.scm:805: `(,flute-name .
`(,flute-name
. (
etc.
(see scm/define-grobs.scm for an example of alist formatting)
http://codereview.appspot.com/1425041/diff/1/13#newcode2597
scm/woodwind-diagrams.scm:2597: (define (update-possb-list input-key
possibility-list cannonic-list)
canonic-list
http://codereview.appspot.com/1425041/diff/1/13#newcode2599
scm/woodwind-diagrams.scm:2599: (throw
use ly:error or ly:warning
http://codereview.appspot.com/1425041/diff/1/13#newcode2833
scm/woodwind-diagrams.scm:2833: The following instruments are supported:
you need to catch invalid instrument names
http://codereview.appspot.com/1425041/diff/1/13#newcode2928
scm/woodwind-diagrams.scm:2928: (assemble-stencils
markup commands return stencils, so (assemble-stencils ...) should
suffice
Description:
Woodwind diagrams
Implements woodwind diagrams in lilypond.
Please review this at http://codereview.appspot.com/1425041/show
Affected files:
A input/regression/woodwind-diagrams-empty.ly
A input/regression/woodwind-diagrams-key-lists.ly
ps/music-drawing-routines.ps
A scm/bezier-tools.scm
M scm/define-stencil-commands.scm
M scm/flag-styles.scm
M scm/lily-library.scm
M scm/lily.scm
M scm/output-ps.scm
M scm/output-svg.scm
M scm/stencil.scm
A scm/woodwind-diagrams.scm