Re: Woodwind diagrams (issue1425041)

[Mike Solomon]

Thank you Patrick!
All of your comments have been incorporated into a new patch, which draws
the svgs a-ok (see attached).

~Mike


On 6/19/10 12:42 AM, "address@hidden" <address@hidden> wrote:

> 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
>

svgtest.svg
Description: Binary data