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/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel