Reviewers: Carl.D.Sorensen,
http://codereview.appspot.com/11857/diff/1/2 File input/regression/fret-diagrams.ly (right): http://codereview.appspot.com/11857/diff/1/2#newcode1 Line 1: \version "2.12.0" This regtest is getting quite large. Is there a logical way to split it up (eg. fret-diagrams-landscape, fret-diagrams-string-count, etc)? http://codereview.appspot.com/11857/diff/1/4 File scm/fret-diagrams.scm (right): http://codereview.appspot.com/11857/diff/1/4#newcode1 Line 1: ;;;; fret-diagrams.scm -- I don't know enough about fret diagrams to really understand what's going on, but I have one fairly general comment: the code is sprinkled with (cond ((eq? orientation 'landscape) foo) ((eq? orientation 'opposing-landscape) bar) (else baz)) where foo, bar and baz are almost the same except that they have differences in signs and the X,Y are swapped around. It would be great if this cond could be isolated to a few functions. For example: (define (real-coordinate string-coordinate fret-coordinate orientation) (cond ((eq? orientation 'landscape) (cons fret-coordinate string-coordinate)) ((eq? orientation 'opposing-landscape) (cons (-fret-coordinate string-coordinate)) (else (cons string-coordinate fret-coordinate)))) and then make-straight-barre-stencil (for example) becomes (define (make-straight-barre-stencil size half-thickness fret-coordinate start-string-coordinate end-string-coordinate orientation) (let ((one-point (real-coordinate start-string-coordinate fret-coordinate orientation)) (other-point (real-coordinate end-string-coordinate fret-coordinate orientation))) (ly:make-line-stencil half-thickness (car one-point) (cdr one-point) (car other-point) (cdr other-point))) If you're in a hurry to get the functionality in, don't let this hold you up. http://codereview.appspot.com/11857/diff/1/4#newcode91 Line 91: (cond Here again you have cond leading to duplicate code. If you had (define (combine-stencils a b c d) (ly:stencil-combine-at-edge a (car b) (cdr b) c d)) then you could move the cond inside: (combine-stencils string-stencil (cond ((eq? orientation 'landscape) (cons Y UP)) ((eq? orientation 'opposing-landscape) (cons Y DOWN)) (else (cons X RIGHT))) (draw-strings (- string-count 1) fret-range th size orientation) gap)) Description: Updates to fret-diagrams Add new orientation opposing-landscape as requested by user Revise orientation code so 'normal is always a default (in the else clause of a cond) Adjust the origin of the fret diagram so that the top fret on the diagram is always at the origin Clean up calls for fret-diagram-details so that merge-details is always used. Clean up text stencil alignments Fix bug in thick top fret Revise input/regression/fret-diagrams.ly so that it tests all fret-diagram code functionality. Please review this at http://codereview.appspot.com/11857 Affected files: M input/regression/fret-diagrams.ly M scm/define-grob-properties.scm M scm/fret-diagrams.scm _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
