Re: Updates to fret-diagrams

[Carl D. Sorensen]

Thanks for the review, Joe.


On 1/2/09 4:17 PM, "address@hidden" <address@hidden> wrote:

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

The regtest can easily be split up.  Is there a reason to do so?  I would
think that any time a change was made to the fret diagram code, the whole
regtest would need to be run anyway.

I'm not trying to be argumentative.  I had the same impression, that the
regtest was too long, but I needed all of those tests to make sure that
everything worked properly.

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

Thanks for a great idea.  I've hated the number of times I've checked
orientation for what was basically the same thing.  I hadn't thought about
this kind of pattern. I'll look into it.

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

It won't be in until release 2.12.2, so I have time to get it done right.

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

I'll go through them all again, and try to localize the orientation check.

Thanks,

Carl