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