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/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel