[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by add

From: paulwmorris
Subject: Re: Charles Winston's GSoC code: Chord Semantics (issue 337870043 by address@hidden)
Date: Fri, 09 Nov 2018 21:44:21 -0800

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

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.

File Documentation/notation/chords.itely (right):
Documentation/notation/chords.itely:679: represent the structure of the
chord. When entered in node mode,
typo: "note mode"
File input/regression/ (right):
input/regression/ texidoc = "The property
@code{chordNameExceptions} can used
'can be used' (This was carried over, but might as well fix while we're
input/regression/ 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.
File input/regression/ (right):
Whitespace on extra unneeded line.
File input/regression/ (right):
Whitespace on unneeded empty line.
File input/regression/ (right):
Whitespace on unnecessary blank line.
File input/regression/ (right):
input/regression/ texidoc = "The
property @code{chordSemanticsNameExceptions} can used
can be used
File input/regression/ (right):
Whitespace on unneeded blank line.
File scm/chord-entry.scm (right):
Unnecessary new line.
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.
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?
scm/chord-entry.scm:243: (ly:pitch-octave (entry-pitch
Would be more clear and consistent if original-inv-pitch were renamed to
original-inv-entry or similar.
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?
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?
scm/chord-entry.scm:306: (assoc-ref semantics-list key))
This is defined twice, see line 292 above.
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?
(quality (cond ((= alteration SHARP) 'aug) ...))
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!.
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
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)))
File scm/chord-ignatzek-names.scm (right):
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.
scm/chord-ignatzek-names.scm:62: "Copy PS, but replace the step of P in
ps -> es, ces, ch-es or something here as well. Probably p -> e too.
And elsewhere as needed.
scm/chord-ignatzek-names.scm:317: lowercase-root?))))))
Indentation seems off here, compared to before this patch.
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.
Whitespace to remove.
scm/chord-ignatzek-names.scm:395: (append
Trailing whitespace nit.
scm/chord-ignatzek-names.scm:405: (ly:context-property context
Formatting, line breaking, indentation causing this code to go too far
to the right.
File scm/chord-name.scm (right):
scm/chord-name.scm:177: (filter not-root-entry? semantics-list))
Could use a blank line between these two define-publics.
scm/chord-name.scm:181: ;; chordmode->exception-entry
This comment doesn't add much.
scm/chord-name.scm:185: "
Closing " usually doesn't get its own line.
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.
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 '()

reply via email to

[Prev in Thread] Current Thread [Next in Thread]