Re: fret diagram comments

[Han-Wen Nienhuys]

a few nits; please apply after fixing those (maybe in a separate commit).

- sans-serif-stencil. is this a markup function? If yes, put in
define-markup.scm

-          (finger-yoffset (- (* size 0.5)))

-> (* -0.5 size)

+  "  Put together a fret-list in the format desired by parse-string "

drop space around doc string. All doc strings should start with capital.


On Wed, Jun 25, 2008 at 2:27 AM, Carl D. Sorensen <address@hidden> wrote:
> 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
>



-- 
Han-Wen Nienhuys - address@hidden - http://www.xs4all.nl/~hanwen