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 _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
