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

[Carl . D . Sorensen]

Thanks for the feedback!  New patch set uploaded.


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
On 2019/04/02 21:20:49, Valentin Villenave wrote:
> 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?

Done.

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"
On 2019/04/02 21:20:49, Valentin Villenave wrote:
> Why not update the regtest version number? OTOH, it doesn’t actually
introduces
> a new syntax.

Done.

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: }
On 2019/04/02 21:20:49, Valentin Villenave wrote:
> 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.

Upon review, I agree with you.  This is really a set of regtests for the
chord-semantics chord namer.  I've combined them into a single regtest.

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");
On 2019/04/02 21:20:49, Valentin Villenave wrote:
> 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.

With this patch, there are two fundamental modes of getting the
semantics necessary to define a chord name.  The newly-added one is to
use the semantics entered by the user (chordSemanticsNameFunction); the
previous one was to  parse the pitches in the chord and try to infer the
semantics.  (There is also the pattern-matching provided by the
exceptions that allows overriding the automatic calculation of chord
names, replacing it with a lookup function.)

Since the difference between the two is how we get the semantics, I
could see that we replace the two fundamental chord name functions with
a single function having an optional argument which is a semantics
entry.  If we want to use the semantics entry present in the chord, we
don't pass the optional argument.  If we want to use the pitch parser
chord namer, we could create a new function whose job is to parse a set
of pitches into a semantics entry.  The resulting semantics entry could
then be passed to the semantics-based namer.

Or alternatively, we could always call the semantic namer, and have the
semantic namer call the parse-semantics function if there is no
semantics entry in the chord.  I believe this could be a way to achieve
your goal.

However, this will require more refactoring.  I don't believe we should
hold off on this patch until we can get that part of it done better.
This patch has lanquished long enough.  IMO we should just push it as-is
and get it in the code base.

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?)
On 2019/04/02 21:20:49, Valentin Villenave wrote:
> I’m not sure "make-*-markup is the most unambiguous name for that
subfunction.

Good catch.

I've changed make-*-markup to make-*-display for all of the functions
that are local to this file.  Of course, the lillypond-defined
make-*-markup functions are not changed.

https://codereview.appspot.com/568650043/