[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Rewrite chordnames - disentangle data from formatting (issue 2234200
From: |
Carl . D . Sorensen |
Subject: |
Re: Rewrite chordnames - disentangle data from formatting (issue 223420043 by address@hidden) |
Date: |
Mon, 13 Apr 2015 02:22:16 +0000 |
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/
Re: Rewrite chordnames - disentangle data from formatting (issue 223420043 by address@hidden), thomasmorley65, 2015/04/26
Re: Rewrite chordnames - disentangle data from formatting (issue 223420043 by address@hidden), thomasmorley65, 2015/04/26