Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by address@hidden)

[paulwmorris]

Hi Charles,

I was thinking about working on MusicXML export for chord symbols, and
wondered about the status of your GSOC project.  (It would, of course,
make exporting chord symbols much simpler and more robust.)  I was
really glad to find your code up on Reitveld, and was curious, so I gave
it a review.

I haven't had a chance to try building with your patch set, but let me
offer my congratulations on what you've done here!  Quite a nice bit of
work.

I've left a number of comments, some nitpicky, but hopefully all
helpful.  Let me know if you have questions or if I can clarify
anything.  If I find time I might try building and looking into the test
failures.  Would be great to get this into LilyPond.

Cheers,
-Paul



https://codereview.appspot.com/337870043/diff/40001/Documentation/notation/chords.itely
File Documentation/notation/chords.itely (right):

https://codereview.appspot.com/337870043/diff/40001/Documentation/notation/chords.itely#newcode679
Documentation/notation/chords.itely:679: represent the structure of the
chord. When entered in node mode,
typo: "note mode"

https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-name-exceptions.ly
File input/regression/chord-name-exceptions.ly (right):

https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-name-exceptions.ly#newcode4
input/regression/chord-name-exceptions.ly:4: texidoc = "The property
@code{chordNameExceptions} can used
'can be used' (This was carried over, but might as well fix while we're
here.)

https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-name-exceptions.ly#newcode29
input/regression/chord-name-exceptions.ly:29: chExceptions = #(append
(chordmode->exception-entry chordVar markupVar) chExceptions)
Hmm, chExceptions is used in its own definition here?  Does that work?
This is not making sense to me.

https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-basic.ly
File input/regression/chord-semantics-basic.ly (right):

https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-basic.ly#newcode10
input/regression/chord-semantics-basic.ly:10:
Whitespace on extra unneeded line.

https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-bass.ly
File input/regression/chord-semantics-bass.ly (right):

https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-bass.ly#newcode10
input/regression/chord-semantics-bass.ly:10:
Whitespace on unneeded empty line.

https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-lowercase-root.ly
File input/regression/chord-semantics-lowercase-root.ly (right):

https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-lowercase-root.ly#newcode14
input/regression/chord-semantics-lowercase-root.ly:14:
Whitespace on unnecessary blank line.

https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-name-exceptions.ly
File input/regression/chord-semantics-name-exceptions.ly (right):

https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-name-exceptions.ly#newcode5
input/regression/chord-semantics-name-exceptions.ly:5: texidoc = "The
property @code{chordSemanticsNameExceptions} can used
can be used

https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-power-chord.ly
File input/regression/chord-semantics-power-chord.ly (right):

https://codereview.appspot.com/337870043/diff/40001/input/regression/chord-semantics-power-chord.ly#newcode12
input/regression/chord-semantics-power-chord.ly:12:
Whitespace on unneeded blank line.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm
File scm/chord-entry.scm (right):

https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode23
scm/chord-entry.scm:23:
Unnecessary new line.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode75
scm/chord-entry.scm:75: chord-semantics))
It's hard to read this code because of the way it's formatted.  Would be
better with more line-breaks to keep the lines from being too long and
the indentation from going so far to the right and wrapping around to
the next line (in narrow views like this code review one).  Similar
comment for other places in this file like this.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode238
scm/chord-entry.scm:238: 'chord-semantics chord-semantics))
Hmm, so there's a single 'chord-semantics property under a
'ChordSemanticsEvent ?  Seems like we could avoid that extra step of
nesting and just have the subproperties of 'chord-semantics under
'ChordSemanticsEvent ?  And that would be more like NoteEvent and other
events.  Or maybe I'm missing something?

https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode243
scm/chord-entry.scm:243: (ly:pitch-octave (entry-pitch
original-inv-pitch))))
Would be more clear and consistent if original-inv-pitch were renamed to
original-inv-entry or similar.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode267
scm/chord-entry.scm:267: (append (if bass (write-me "base3: " bass)
(list (make-note-ev bass 'bass #t)) '())
This write-me looks like a debugging statement that was left in?

https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode272
scm/chord-entry.scm:272: (bass (make-elements (cons (make-note-ev bass
'bass (cons #t #t))
Hmm, this (cons #t #t) looks like it could be related to one of the
regression test failures that James posted about?

https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode306
scm/chord-entry.scm:306: (assoc-ref semantics-list key))
This is defined twice, see line 292 above.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode399
scm/chord-entry.scm:399: ((= alteration FLAT) (set! quality 'dim)))))
Instead of all of these (set! quality ...) why not just let the result
of this cond be assigned to 'quality' and add an 'else' 'maj at the end?
Basically:
(quality (cond ((= alteration SHARP) 'aug) ...))

https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode411
scm/chord-entry.scm:411: (if (= step-number 7) (set! quality 'min))
Defining quality using cond or case would be a more idiomatic Scheme-ish
way to do it, avoiding the mutation of set!.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode414
scm/chord-entry.scm:414: (cons (list (cons 'step-number step-number)
(cons 'step-quality quality)) chord-step-list)
Why not use make-chord-step here to simplify this?  Also, I'd wrap this
to more than one line, to break it up and make it easier to read and
understand.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-entry.scm#newcode425
scm/chord-entry.scm:425: (make-chord-entry (ly:make-pitch 0 (- n 1) (nca
n)) chord-step)))
Here's a way to simplify this:

 (map (lambda (chord-step)
        (let* ((sn (assoc-ref chord-step 'step-number))
               (octave (if (>= sn 8) 1 0))
               (note (- sn (if (>= sn 8) 8 1)))
               (alter (if (= sn 7) FLAT 0))
               (pitch (ly:make-pitch octave note alter)))
          (make-chord-entry pitch chord-step)))

https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm
File scm/chord-ignatzek-names.scm (right):

https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode44
scm/chord-ignatzek-names.scm:44: "Does PS have the X step? Return that
step if it does."
Usually Scheme doc strings come after the 'define' like the one in
double quotes here.  I think it would be better to do them that way,
rather than with ';;' above the 'define'.  That will allow consolidation
of some of these that have both here.  Also, ps is short for pitches, so
it would be clearer to rename with ces, es, or ch-es (or something) for
chord entries here.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode62
scm/chord-ignatzek-names.scm:62: "Copy PS, but replace the step of P in
PS."
ps -> es, ces, ch-es or something here as well. Probably p -> e too.
And elsewhere as needed.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode317
scm/chord-ignatzek-names.scm:317: lowercase-root?))))))
Indentation seems off here, compared to before this patch.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode362
scm/chord-ignatzek-names.scm:362: empty-markup))
These make-removals-markup and make-additions-markup functions are very
similar.  If you are feeling motivated, it would be good to refactor to
remove the duplication.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode379
scm/chord-ignatzek-names.scm:379:
Whitespace to remove.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode395
scm/chord-ignatzek-names.scm:395: (append
Trailing whitespace nit.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-ignatzek-names.scm#newcode405
scm/chord-ignatzek-names.scm:405: (ly:context-property context
'chordPrefixSpacer))
Formatting, line breaking, indentation causing this code to go too far
to the right.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-name.scm
File scm/chord-name.scm (right):

https://codereview.appspot.com/337870043/diff/40001/scm/chord-name.scm#newcode177
scm/chord-name.scm:177: (filter not-root-entry? semantics-list))
Could use a blank line between these two define-publics.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-name.scm#newcode181
scm/chord-name.scm:181: ;; chordmode->exception-entry
This comment doesn't add much.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-name.scm#newcode185
scm/chord-name.scm:185: "
Closing " usually doesn't get its own line.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-name.scm#newcode187
scm/chord-name.scm:187: (define (get-semantics-event music)
Optional, but it's probably clearer to move this define up a level and
not nest it under the other defines so deeply.

https://codereview.appspot.com/337870043/diff/40001/scm/chord-name.scm#newcode204
scm/chord-name.scm:204: return))
The 'return' is not adding much here.  I'd suggest dropping it and just
having the result of the cond be the return value for the function,
without the (set! return ...) expressions, and with an else '()
fallback.

https://codereview.appspot.com/337870043/