Hi Carl, I appreciate you taking the time to rework this patch, does it mean you’re intending to shepherd Charles’ work until it gets merged? In addition to Paul’s comments which you’ve nicely addressed, I had a few additional ones below, on other aspects of Charles’ approach (and taking into account that I’ve been trying to streamline some chord-related parts of LilyPond’s codebase in the meantime). BTW, is the end goal here to actually replace Lily’s default chord entry mode at some point? As you may have noticed, I’ve dropped some chord modes that hadn’t been working for the past 15 years anyway (namely Banter, and jazzExceptions as well), so still referring to Ignatzek exceptions as such whilst there are none other available has become sort of moot. Now that we’d be having an additional `semantics’ feature, it *could* sort of make sense to have two different systems in place (though not in the way chord names used to be implemented). At any rate, I’ve been trying to hunt down code duplication and clunky mechanisms based on hardcoded stuff (e.g. "Italian" and "German" chord names and note->string functions); I’d hope that this patch would allow further progress in this regard but that doesn’t appear to be the case.
https://codereview.appspot.com/568650043/diff/572560043/Documentation/notation/chords.itely File Documentation/notation/chords.itely (right): https://codereview.appspot.com/568650043/diff/572560043/Documentation/notation/chords.itely#newcode472 Documentation/notation/chords.itely:472: c:m7^1 ees I’d put the simple chord in front of the more unexpected use case. Also, why not use c:maj7^1 and e:m chords? https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-name-exceptions.ly File input/regression/chord-name-exceptions.ly (right): https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-name-exceptions.ly#newcode1 input/regression/chord-name-exceptions.ly:1: \version "2.16.0" Why not update the regtest version number? OTOH, it doesn’t actually introduces a new syntax. https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-semantics-basic.ly File input/regression/chord-semantics-basic.ly (right): https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-semantics-basic.ly#newcode10 input/regression/chord-semantics-basic.ly:10: What’s this line standing for? https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-semantics-sus.ly File input/regression/chord-semantics-sus.ly (right): https://codereview.appspot.com/568650043/diff/572560043/input/regression/chord-semantics-sus.ly#newcode11 input/regression/chord-semantics-sus.ly:11: } I feel like these are way too many regtests. Sure, having a nicely ordered list of all features looks nice, but 1/ we might be adding quite a few extra seconds to `make check’ here 2/ regtests are not the place where to be listing or documenting features and 3/ all of these could as well be included in a single regtest, duly commented and explained: if anything ever breaks in the future, we’ll catch it just as well. https://codereview.appspot.com/568650043/diff/572560043/lily/chord-name-engraver.cc File lily/chord-name-engraver.cc (right): https://codereview.appspot.com/568650043/diff/572560043/lily/chord-name-engraver.cc#newcode99 lily/chord-name-engraver.cc:99: SCM name_proc = get_property ("chordSemanticsNameFunction"); Ugh. Is there really no way of merging this with chordNameFunction (possibly with additionaloptargs)? I understand this adds many additional (and certainly useful) features, but this looks like potential for duplicate/semi-incompatible subroutines down the line. Fortunately, both approaches use chordRootNamer and therefore will be able to take advantage from the localized note names that are now available. But still, I wish we could factorize things even further. https://codereview.appspot.com/568650043/diff/572560043/scm/chord-ignatzek-names.scm File scm/chord-ignatzek-names.scm (right): https://codereview.appspot.com/568650043/diff/572560043/scm/chord-ignatzek-names.scm#newcode342 scm/chord-ignatzek-names.scm:342: (define (make-root-markup root lowercase-root?) I’m not sure "make-*-markup is the most unambiguous name for that subfunction. https://codereview.appspot.com/568650043/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
