Re: Rewrite chordnames - disentangle data from formatting (issue 223420043 by address@hidden)

[Carl . D . Sorensen]

I think that this is a great start.  You're working in a really complex
area, and trying to sort it out.  Good for you!

I've made some specific comments below.  I think the separation between
list creation and markup creation needs to be made stronger and more
explicit; probably we need to change the property names.




https://codereview.appspot.com/223420043/diff/20001/ly/engraver-init.ly
File ly/engraver-init.ly (right):

https://codereview.appspot.com/223420043/diff/20001/ly/engraver-init.ly#newcode689
ly/engraver-init.ly:689: chordRootNamer = #note-name->list
Perhaps we should rename chordRootNamer to something like chordAnalyzer.
 That would show the meaning much more clearly.

Or perhaps more importantly, we should have a chordAnalyzer that takes a
chord and returns a list, and a chordNamer that takes a list an returns
a markup.  At that point, we'd really have a clear separation of the two
functions, and if you were happy with the Analyzer, you would only need
to change the Namer.

https://codereview.appspot.com/223420043/diff/20001/ly/property-init.ly
File ly/property-init.ly (right):

https://codereview.appspot.com/223420043/diff/20001/ly/property-init.ly#newcode134
ly/property-init.ly:134: \set chordRootNamer =
#(chord-name->italian-list #t)
In scm/chord-generic-names.scm you are saying that a namer returns a
markup, not a list.  I think that is probably a pretty good use.

Perhaps we should think of two different kinds of things:  "analyzers"
that convert chord names to lists, and "namers" that convert lists to
markups.  And we should hold strong to that convention.

I think it's a mistake to have a "namer" return a list, since the thing
that is printed as a ChordName is a markup.  A namer should produce a
markup, in my opinion.

https://codereview.appspot.com/223420043/diff/20001/scm/chord-generic-names.scm
File scm/chord-generic-names.scm (right):

https://codereview.appspot.com/223420043/diff/20001/scm/chord-generic-names.scm#newcode33
scm/chord-generic-names.scm:33: "Return a markup for @var{pitch}, with
name of @var{pitch} and a (possible)
I think the name of this function should change to something like

note-markup

https://codereview.appspot.com/223420043/diff/20001/scm/chord-generic-names.scm#newcode35
scm/chord-generic-names.scm:35: (let* ((note-namer (note-name->list
pitch #f))
I don't like the name note-namer here; it sounds like a function, rather
than an alist.

Perhaps note-alist  or note-name-alist   or structure-alist   or even
namer-alist.

Or you could eliminate the alist part and call it

note-structure  or note-elements (although elements has a specific
lilypond meaning that probably makes this name undesirable) or
note-parts.

https://codereview.appspot.com/223420043/diff/20001/scm/chord-ignatzek-names.scm
File scm/chord-ignatzek-names.scm (right):

https://codereview.appspot.com/223420043/diff/20001/scm/chord-ignatzek-names.scm#newcode288
scm/chord-ignatzek-names.scm:288: ;; Build the list for the chord-data
from 'root-info, 'slash-chord-separato,
Isn't slash-chord-separator part of the display, rather than part of the
chord structure?

It seems to me that this patch is mostly maintaining the mix of parsing
and display; it's just putting the stuff into a list first.

https://codereview.appspot.com/223420043/diff/20001/scm/chord-ignatzek-names.scm#newcode395
scm/chord-ignatzek-names.scm:395: ;;;; Step 2: Define formatter for the
chord-elements using this list
I'm not sure how this separation between step 1 and step 2 really
accomplishes the stated goal of the patch.  Can you give an example of
how this makes it easier to define a new display style for a chord?

https://codereview.appspot.com/223420043/diff/20001/scm/chord-ignatzek-names.scm#newcode427
scm/chord-ignatzek-names.scm:427: (make-hspace-markup (if (= alteration
FLAT) 0.57285385 0.5))
Magic numbers are of concern, both here and later.  At the least, there
should be a TODO comment.

https://codereview.appspot.com/223420043/diff/20001/scm/chord-name.scm
File scm/chord-name.scm (right):

https://codereview.appspot.com/223420043/diff/20001/scm/chord-name.scm#newcode53
scm/chord-name.scm:53: (define-safe-public ((chord-name->german-list
B-instead-of-Bb)
Shouldn't all these functions be combined into one

(define-safe-public ((chord-name->note-alteration-list language-name
options) ... )

It seems like to really separate it out, we need to do more than just
return a list instead of a markup.  We should create a real logical
structure that will separate things out cleanly.

https://codereview.appspot.com/223420043/