Updates to fret-diagrams

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.

[joeneeman, 2009-01-02 23:17:03]

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

[hanwenn, 2009-01-04 23:02:01]

I agree with joe that there is a lot of duplicate here; it must be possible to
rewrite this more tightly. 

That said, the fret-diagram code is completely your domain, so it's your call.

http://codereview.appspot.com/11857/diff/1/4
File scm/fret-diagrams.scm (right):

http://codereview.appspot.com/11857/diff/1/4#newcode189
Line 189: (let* ((sth (* th size))
this indent is broken.

Also, I used to write rather abbreviated var names like sth, but I now think it
is better to be more verbose.  However, it looks as if sth is not used at all.