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