lilypond-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Woodwind diagrams (issue1425041)


From: Carl . D . Sorensen
Subject: Woodwind diagrams (issue1425041)
Date: Mon, 31 May 2010 14:06:49 +0000

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]