Implementing Chord Semantics as a part of the EventChord structure,

Implementing Chord Semantics as a part of the EventChord structure,
and using semantics to create a chord name with the Chord Name Engraver

[Dan Eble, 2017-07-06 21:57:23]

I reviewed the C++ file.  I believe others are more qualified to review the
patch as a whole.

https://codereview.appspot.com/321250043/diff/1/lily/chord-name-engraver.cc
File lily/chord-name-engraver.cc (right):

https://codereview.appspot.com/321250043/diff/1/lily/chord-name-engraver.cc#n...
lily/chord-name-engraver.cc:99: SCM name_proc = get_property
("chordSemanticsNameFunction");
1. Should chordSemanticsNameFunction be included in the list at the bottom of
this file? (I think so.)
2. Does the type of name_proc need to be validated before passing it to
scm_call_2? (I really don't remember.)

https://codereview.appspot.com/321250043/diff/1/lily/chord-name-engraver.cc#n...
lily/chord-name-engraver.cc:103: // Ugh, we created a grob, now we better
populate it.
If this was not expected, then as a user, I would like to see lilypond emit a
warning.

[pkx166h, 2017-07-08 11:35:14]

I've created a tracker issue for this so that it gets entered into the patch
cycle proper.

https://sourceforge.net/p/testlilyissues/issues/5153/

Charles I also see that you have rights to update trackers, so you will also get
notifications from this as it goes through the review process.

[Carl, 2017-07-18 18:39:49]

Looks generally good to me.  It's not yet complete, so I don't think it's a
candidate for pushing yet.  But I think you've got the right stuff in and are
moving forward well.

Good job!

https://codereview.appspot.com/321250043/diff/1/lily/chord-name-engraver.cc
File lily/chord-name-engraver.cc (right):

https://codereview.appspot.com/321250043/diff/1/lily/chord-name-engraver.cc#n...
lily/chord-name-engraver.cc:99: SCM name_proc = get_property
("chordSemanticsNameFunction");
On 2017/07/06 21:57:23, Dan Eble wrote:
> 1. Should chordSemanticsNameFunction be included in the list at the bottom of
> this file? (I think so.)

chordSemanticsNameFunction should definitely be included as a property that is
read.

https://codereview.appspot.com/321250043/diff/1/lily/chord-name-engraver.cc#n...
lily/chord-name-engraver.cc:103: // Ugh, we created a grob, now we better
populate it.
On 2017/07/06 21:57:23, Dan Eble wrote:
> If this was not expected, then as a user, I would like to see lilypond emit a
> warning.

I agree.

https://codereview.appspot.com/321250043/diff/1/scm/chord-entry.scm
File scm/chord-entry.scm (right):

https://codereview.appspot.com/321250043/diff/1/scm/chord-entry.scm#newcode93
scm/chord-entry.scm:93: (interpret-additions (cons (make-chord-entry-from-pitch
(car mods))
To let you fit on an 80-column line, you may wish to move  (cons down to the
next line and indent it two spaces.  The other arguments to interpret additions
would then align with (cons

https://codereview.appspot.com/321250043/diff/1/scm/chord-entry.scm#newcode163
scm/chord-entry.scm:163: ;; TODO: this omits 3 in power chord. Change to only do
so if lead-mod is null.
Nice to note this; please make sure to add the extra clause in the if here.

https://codereview.appspot.com/321250043/diff/1/scm/chord-entry.scm#newcode206
scm/chord-entry.scm:206: (set! complete-chord (map (lambda (x)
(chord-pitch-transpose x root))
Maybe the name chord-pitch-transpose should be changed, since two things are
happening in that function:  1) The proper pitches are being created (using
ly:pitch-transpose) and
2) The given chord steps are being added.

Maybe make-chord-step or just chord-step

(set! complete-chord (map (lambda (x) (chord-step x root)) complete-chord)

seems to be a bit clearer to me.  As it is, I wondered why you created your own
transpose function instead of using ly:pitch-transpose.  And then I saw it was
because the chord has both pitches and steps now.

https://codereview.appspot.com/321250043/diff/1/scm/chord-entry.scm#newcode309
scm/chord-entry.scm:309: ;; chord modifiers change the pitch list.
maybe change the comment to say chord modifiers change the pitch list and the 
chord steps?

https://codereview.appspot.com/321250043/diff/1/scm/chord-entry.scm#newcode349
scm/chord-entry.scm:349: (maj . , maj7-modifier)
I thought you were going to adjust this so maj was not maj7.  That way c:5 could
be power chord, and c:maj could be major triad.  c:maj7 would be a major 7.

Is this plan gone now?

https://codereview.appspot.com/321250043/diff/1/scm/define-context-properties...
File scm/define-context-properties.scm (right):

https://codereview.appspot.com/321250043/diff/1/scm/define-context-properties...
scm/define-context-properties.scm:202: (chordSemanticsNameFunction ,procedure?
"The function that converts
Should be "A" function, not "The" function, IMO.  See the parallel with
chordNoteNamer