RE: fret diagram comments

[Carl D. Sorensen]

Han-Wen,

I've got one patch that has all the changes and nothing else available on the 
csorensen fork.

The URL is http://repo.or.cz/w/lilypond/csorensen.git

The fret-diagram-details is the branch you want.  The "Move fret diagram 
specific properties" commit should work properly.

The code has been tested against the new version of the regression test and 
works properly.

You made some comments about the fret-diagram code on Saturday.  I've put my 
comments below about how I dealt with things.

> -----Original Message-----
> From: Han-Wen Nienhuys [mailto:address@hidden
> Sent: Saturday, June 21, 2008 4:17 PM
> To: lily-devel
> Subject: fret diagram comments
>
> hi Carl,
>
> here some quick notes on the fret diagram code.  I am not
> familiar with  frets, so I have little comment on the code
> itself, but it seems a little sloppy with lots of random
> formatting changes all over the place.

I used emacs to indent the code, and inserted strategic line breaks to get
everything to line up nicely in 80-column pages.  Thanks for the emacs tip!

>
> - for the properties inside the fret0-diagram-details, would
> it make sense to drop the fret- prefix?

As mentioned earlier, the fret- prefix properties are to distinguish between 
frets and strings.

>
>           (xo-list (cdr (assoc 'xo-list parameters)))
>
> what if xo-list is not in the list? Use something that has a
> default, so it won't crash on (cdr '())

I believe that this is not a problem, because parameters is not a user-generated
list.  It comes from the fret-diagram parser, and there is always a value for
'xo-list.  The fret-diagram parser is not a public function.


>
>  ;            (barre-vertical-offset (chain-assoc-get
> 'barre-vertical-offset props 0.5))
>
> make sure there is no commented out code left after you
> finish. If you want to preserve a snippet of code, add a
> explaining comment.  I think there is a finger-code comented
> out somewhere.

All of this code has been removed.


>
> +(define (merge-markup-override x alist-list . default)
> +  "Return ALIST-LIST entries for  key X, in one combined alist.
>
> rename x to key
>

Renamed x to key.

>
> +  "Return ALIST-LIST entries for  key X, in one combined alist.
> +  There can be two ALIST-LIST entries for a given key.  The first
> +  comes from the override-markup function, the second comes
> +  from property settings during a regular override.
> +  Return DEFAULT (optional, else #f) if not found."
> +
>
> maybe an example would clarify

Added some clarification to the doc string.

>
> +(define (assoc-default key alist default)
>
> I think we have a ly:assoc-get that does this already

I used assoc-get, which is defined to be equal to ly:assoc-get in one of the 
system
scheme files (although I forget which one at the moment).


>
> +(define (merge-markup-override x alist-list . default)
>
> the code has nothing to do with markup. Rename?

I changed the name to merge-details, because its purpose is to merge two 
different
fret-diagram-details alists.

>
> you seem to have indent issues with your editor.  Which one are you
> using?  I also saw some lines where you were adding/removing trailing
> whitespace.  Did I leave trailing whitespace, or did you add it? (we
> shouldn't have any)

There were a couple of lines on the main lilypond code with trailing
whitespace, which I have removed.

>
>
> +(define (draw-frets  fret-range string-count th size orientation)
>
> 2 spaces after func name

Removed every occurence of two consecutive spaces inside the code and
not in quoted strings.  There were about half a dozen.



I hope this will meet your needs.  Please let me know if I need to make
other changes.

Thanks,

Carl