Re: Woodwind diagrams (issue1425041)

[pnorcks]

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