The new patch worked. I've put some comments in line.
Great job, Mike! http://codereview.appspot.com/1425041/diff/1/8 File scm/lily-library.scm (right): http://codereview.appspot.com/1425041/diff/1/8#newcode492 scm/lily-library.scm:492: (cons In general, I prefer to have the first argument on the line with (cons. There may be a case where wrapping works better if it's split, but that should be the exception, not the rule. Is this really intended as an interval function [(x-min . x-max )] or is it intended as a coordinate function[(x . y)]? If these functions are applied to (x . y) pairs, let's change the name to (coord-* instead of (interval-*. http://codereview.appspot.com/1425041/diff/1/8#newcode514 scm/lily-library.scm:514: (define-public (interval-rotate interval degrees-in-radians) This looks like you're using an interval as an x-y pair. But intervals are (x-min . x-max) or (y-min . y-max) pairs. Used coord (see bezier.scm, and maybe move all the coord stuff to lily-library.scm) instead of intervals. http://codereview.appspot.com/1425041/diff/1/8#newcode520 scm/lily-library.scm:520: (radius (radius should line up with (interval, not ((interval http://codereview.appspot.com/1425041/diff/1/8#newcode588 scm/lily-library.scm:588: (if It's more standard practice to put the test on the same line as the if. http://codereview.appspot.com/1425041/diff/1/8#newcode610 scm/lily-library.scm:610: (/ I think you should not have the operators by themselves on a line. http://codereview.appspot.com/1425041/diff/1/10 File scm/output-ps.scm (right): http://codereview.appspot.com/1425041/diff/1/10#newcode105 scm/output-ps.scm:105: (define (connected-shape pointlist thick x-scale y-scale connect fill) I find it much easier to read this function in this indentation format: (define (connected-shape pointlist thick x-scale y-scale connect fill) (ly:format "~a~4f ~4f ~4f ~4f ~a ~a draw_connected_shape" (string-concatenate (map (lambda (x) (apply (if (eq? (length x) 6) (lambda (x1 x2 x3 x4 x5 x6) (ly:format "~4f ~4f ~4f ~4f ~4f ~4f 6 " x1 x2 x3 x4 x5 x6)) (lambda (x1 x2) (ly:format "~4f ~4f 2 " x1 x2))) x)) (reverse pointlist))) (length pointlist) x-scale y-scale thick (if connect "true" "false") (if fill "true" "false"))) http://codereview.appspot.com/1425041/diff/1/10#newcode112 scm/output-ps.scm:112: (eqv? (length x) 6) Integer comparisons can always be eq? http://codereview.appspot.com/1425041/diff/1/11 File scm/output-svg.scm (right): http://codereview.appspot.com/1425041/diff/1/11#newcode359 scm/output-svg.scm:359: (new-start-angle line up with second (new-start-angle, not ((new-start-angle http://codereview.appspot.com/1425041/diff/1/11#newcode363 scm/output-svg.scm:363: (sqrt line up with (* http://codereview.appspot.com/1425041/diff/1/11#newcode365 scm/output-svg.scm:365: (* No space is saved by breaking the lines after these operators, because the indent is the same place as you'd be without the break. So you should keep them on the same line: (+ (* (* y-radius y-radius)) (* (cos new-start-angle) (cos new-start-angle)) etc. http://codereview.appspot.com/1425041/diff/1/11#newcode446 scm/output-svg.scm:446: "M0 0~a ~a" can we avoid the ~a ~a construct? That is a potential place for a user to insert arbitrary PS code. If we can't, we should either check the data or identify (in a comment) the potential security risk. http://codereview.appspot.com/1425041/diff/1/12 File scm/stencil.scm (right): http://codereview.appspot.com/1425041/diff/1/12#newcode538 scm/stencil.scm:538: "Returns a function drawing an arrow from here to @var{destination}." Should be more like "Returns a function drawing a line from current point to @var{destination}, with optional arrows of @var{max-size} on start and end controlled by @var{start?} and @var{end?}." http://codereview.appspot.com/1425041/show _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
