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 _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
