Rewrite chordnames - disentangle data from formatting

Rewrite chordnames - disentangle data from formatting

Issue 4344

Store all data to print a chordmarkup in an alist.
Provide formatter for the elements of a chord and a final formatter
outputting the result.

Rename some rewritten procedures from scm/chord-name.scm reflecting
the new behaviour. With the need to change those names in
ly/engraver-init.ly and ly/property-init.ly as well.

The old 'alteration->text-accidental-markup' was used for bass-figures in
scm/translation-functions.scm. Thus inserted there.

scm/chord-generic-names.scm is fixed only so far that it does compile.
For a full test uncomment the section with the Banter-style in
/Documentation/included/chord-names-jazz.ly

[thomasmorley651, 2015-04-11 22:06:49]

Fix tabs

[thomasmorley651, 2015-04-11 22:17:53]

Please review.

Intended is a pure maintaining change. Maybe a first step for more flexibility,
easier customizing etc.

If I sorted it correctly, no change in visible output should occur.

Apart from the Banter-style (see commit-message) no bug-fixing happens, although
I noticed one (see FIXME in ignatzek-chord-names.scm). Some other TODOs, maybe
worth other issues.

[marc, 2015-04-12 06:43:43]

Hi Harm,

I cannot comment much on the code, but I like the general approach of separating
chord information and chord markup very much!

Two tiny remarks below.

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/chord-ignatzek-names.scm:167: (glue-word-to-step "huh" mod))) ;; What does
"huh" mean??
'huh' is meant to appear in cases where (pitch-step mod) returns strange values
and has no musical meaning ;-)
I don't know if/when this is triggered, but an error message would be more
appropriate IMHO.

https://codereview.appspot.com/223420043/diff/20001/scm/chord-ignatzek-names....
scm/chord-ignatzek-names.scm:288: ;; Build the list for the chord-data from
'root-info, 'slash-chord-separato,
s/separato/separator/

[Carl, 2015-04-13 02:22:15]

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#newco...
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#newco...
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.s...
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.s...
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/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/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/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.

[thomasmorley651, 2015-04-26 20:17:57]

deeper disentangle, fixing TODOs

[thomasmorley651, 2015-04-26 20:24:08]

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#newco...
> 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#newco...
> 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.s...
> 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.s...
> 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/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/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/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

[thomasmorley651, 2015-04-26 20:30:11]

Although, this is not the final patch for this topic, James, may I ask you to
run patchy against it?