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

[thomasmorley65]

On 2015/04/13 02:22:15, Carl wrote:
> 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.

Hi Carl,

most of the above comments needs still to be adressed, especially the
ones about namings.
Though, with the recent patchset I try to do a much deeper
disentangeling of structure and output.

Cheers,
  Harm

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