lilypond-devel
[Top][All Lists]
Advanced

[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





reply via email to

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