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

[Carl . D . Sorensen]

On 2018/11/10 05:44:23, pwm wrote:

> 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"
Done.


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.)

Done



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.

chExceptions is previously defined to be a couple of new chords
prepended to ignatzekExceptions, so it can be used this way.





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.

Done


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

Done


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.


Done


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

Done

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.

Done



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?


We could do that, but it would pollute the global namespace with all the
properties
that are part of 'chord-semantics and are only used for chord semantics.
 By putting them
in a single property, we avoid polluting the namespace.  Similar to the
way we do fret-diagram-details.


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.

Why?  Not to be argumentative, but I wonder what about this name is more
clear and consistent, in your opinion?



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?


Done


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?

Will look more later.



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.

Done



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) ...))

I think it's because for some intervals, you set a zero alteration as
perfect, while
for others, the zero alteration is 'maj.  So it doesn't drop easily into
an else, I think.



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!.

I totally agree with you; I'll try to work this out for a future patch.
For now, I want
to just get it into review.



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.

Done



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)))


Done


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.

Done



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.

Done



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.

Save for next review



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.

Should be cleared up.



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.

Done


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.


Done


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.

Done



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.
Done


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.


Done



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.

Done



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