Re: Implementing Chord Semantics as a part of the EventChord structure, (issue 321250043 by address@hidden)

[Carl . D . Sorensen]

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#newcode99
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#newcode103
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.scm
File scm/define-context-properties.scm (right):

https://codereview.appspot.com/321250043/diff/1/scm/define-context-properties.scm#newcode202
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

https://codereview.appspot.com/321250043/