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 <[EMAIL PROTECTED]> 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:[EMAIL PROTECTED] >> 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 - [EMAIL PROTECTED] - http://www.xs4all.nl/~hanwen _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
