[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Woodwind diagrams (issue1425041)
From: |
pnorcks |
Subject: |
Re: Woodwind diagrams (issue1425041) |
Date: |
Fri, 18 Jun 2010 22:42:00 +0000 |
Hi Mike,
This is very impressive. Thanks for your work on this. I just have a
few comments for you about the SVG-related code.
Thanks,
Patrick
http://codereview.appspot.com/1425041/diff/12001/13007
File scm/lily-library.scm (right):
http://codereview.appspot.com/1425041/diff/12001/13007#newcode583
scm/lily-library.scm:583:
If you want to use these procedures in output-svg.scm, they all need to
be `define-public'-ified. Like so:
(define-public PI (* 4 (atan 1)))
etc.
http://codereview.appspot.com/1425041/diff/12001/13010
File scm/output-svg.scm (right):
http://codereview.appspot.com/1425041/diff/12001/13010#newcode352
scm/output-svg.scm:352: (define (partial-ellipse x-radius y-radius
start-angle end-angle thick connect fill)
Move the definition of `new-start-angle' to here so that this code will
compile (`new-start-angle' is used in `make-ellipse-radius')
http://codereview.appspot.com/1425041/diff/12001/13010#newcode362
scm/output-svg.scm:362: (new-end-angle (* PI-OVER-180 (angle-0-360
end-angle)))
PI-OVER-180 and other procedures will work once you make the changes in
lily-library.scm.
http://codereview.appspot.com/1425041/diff/12001/13010#newcode399
scm/output-svg.scm:399: (if
Please condense this block like so:
(if connect
(ly:format "L~4f,~4f"
(* start-radius (cos new-start-angle))
(- (* start-radius (sin new-start-angle))))
"")))))))
http://codereview.appspot.com/1425041/diff/12001/13010#newcode418
scm/output-svg.scm:418: ;; Hopefully will be mitigated by ~4f below.
Uh, how is this a security risk? PS code is not interpreted by SVG
agents, and here we are just dealing with simple numbers and strings
that turn into SVG markup.
http://codereview.appspot.com/1425041/diff/12001/13010#newcode421
scm/output-svg.scm:421: (string-concatenate
I would condense this block too, to make it more readable:
(string-concatenate
(map (lambda (x)
(apply
(if (eq? (length x) 6)
(lambda (x1 x2 x3 x4 x5 x6)
(ly:format "C~4f ~4f ~4f ~4f ~4f ~4f"
(* x1 x-scale)
(- (* x2 y-scale))
(* x3 x-scale)
(- (* x4 y-scale))
(* x5 x-scale)
(- (* x6 y-scale))))
(lambda (x1 x2)
(ly:format "L~4f ~4f"
(* x-scale x1)
(- (* y-scale x2)))))
x))
pointlist))
http://codereview.appspot.com/1425041/show
- Re: Woodwind diagrams (issue1425041),
pnorcks <=