RE: fret diagram comments

[Carl D. Sorensen]

> -----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.
>
> - for the properties inside the fret0-diagram-details, would
> it make sense to drop the fret- prefix?

I suppose we could.  I guess I put the fret- prefix in to make it
clear it was in a different name.

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

OK -- I think this is old code, so I haven't checked it.

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


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

Will do.

>
> +  "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

How would you like me to put in an example?  Would you like an alist
with two different entries for the key?

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

OK -- no need to recreate the wheel.  I'll check the code and see  if it's
the same, or if not, if it will work in this situation.

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

It has to do with the override-markup function.  It merges the props from
the override-markup function with the props from the context props.

>
>     fix documentation files for new fret-diagram-details
>
>     change Fretboard documentation to replace finger-code
>     with fret-diagram-details.
>
> this commit reindents all of define-grobs.scm.  Is that necessary?
> Maybe this is a tab/space issue, but please make sure you don't touch
> code without reason?
>
> I see you fixed this in further commits. Maybe you can use cherry
> picking to construct a list of commits which does not touch the file
> unnecessarily?

Yes, I had problems with a Windows editor behaving improperly, so that's why
I went to Fedora for this development.  And then I had problems with Git.  Now
I think I've got it down, so I ought to be able to fix this.

I think that I will do a git reset --soft to get me with the files I want, then 
a git-commit to a new commit that has just the changes I want to keep.

>
>      (if (= fret-count 1)
> -       fret-line
> +    fret-line
>
> 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)

I'm now using vi.

I hope I didn't add trailing whitespace.  I may have removed some.

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

OK -- will fix.

>
>       ((equal?   number-type  'roman-lower) (fancy-format #f
> "~(~:@r~)" ba
>
> double spaces
>

OK -- will fix, and will check some more.


Thanks for the feedback.

Carl