Charles Winston's chord-semantics GSOC work (issue 568650043 by address@hidden)

[v . villenave]

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/