Hi Marc, the amount of comments is actually a compliment - it means that i understood almost everything! :)
Please reword the commit message along the lines of the first comment below. Also, i think it would be good to explain why the code doesn't just take (15), turn it into a markup right away and append to the clef - it uses "style" property (both in the actual code as well as in commit message). thanks! Janek http://codereview.appspot.com/6813044/diff/6001/input/regression/clef-optional-octavation.ly File input/regression/clef-optional-octavation.ly (right): http://codereview.appspot.com/6813044/diff/6001/input/regression/clef-optional-octavation.ly#newcode5 input/regression/clef-optional-octavation.ly:5: texidoc="Octavate symbols may be parenthesized or bracketed." This description is not accurate imo. It was always possible to parenthesize octavate symbols - what needs checking is that syntax "G^(8)" will automatically work for this. http://codereview.appspot.com/6813044/diff/6001/input/regression/clef-optional-octavation.ly#newcode12 input/regression/clef-optional-octavation.ly:12: \clef "C^(8)" c''1 i suggest to add at least one clef written in full name and with two-digit transposition, eg. \clef "treble^(15)" http://codereview.appspot.com/6813044/diff/6001/input/regression/cue-clef-optional-octavation.ly File input/regression/cue-clef-optional-octavation.ly (right): http://codereview.appspot.com/6813044/diff/6001/input/regression/cue-clef-optional-octavation.ly#newcode4 input/regression/cue-clef-optional-octavation.ly:4: texidoc = "Optional octavation for clefs for cue notes." Similar as before - the description needs a bit rewording. http://codereview.appspot.com/6813044/diff/6001/lily/clef-engraver.cc File lily/clef-engraver.cc (right): http://codereview.appspot.com/6813044/diff/6001/lily/clef-engraver.cc#newcode125 lily/clef-engraver.cc:125: if (ly_is_procedure (formatter)) it seems that octavation won't work at all if "clefOctavationFormatter" is not a procedure. Do we want this? http://codereview.appspot.com/6813044/diff/6001/lily/cue-clef-engraver.cc File lily/cue-clef-engraver.cc (right): http://codereview.appspot.com/6813044/diff/6001/lily/cue-clef-engraver.cc#newcode119 lily/cue-clef-engraver.cc:119: if (ly_is_procedure (formatter)) Ditto. http://codereview.appspot.com/6813044/diff/6001/scm/define-context-properties.scm File scm/define-context-properties.scm (right): http://codereview.appspot.com/6813044/diff/6001/scm/define-context-properties.scm#newcode200 scm/define-context-properties.scm:200: (cueClefOctavationFormatter ,procedure? "A procedure that takes the maybe it would be a good idea to merge this with clefOctavationFormatter? Or is it not possible? http://codereview.appspot.com/6813044/diff/6001/scm/parser-clef.scm File scm/parser-clef.scm (right): http://codereview.appspot.com/6813044/diff/6001/scm/parser-clef.scm#newcode138 scm/parser-clef.scm:138: (else style))))) i'm probably wrong, but shouldn't this be else style default or sth like that? http://codereview.appspot.com/6813044/diff/6001/scm/parser-clef.scm#newcode149 scm/parser-clef.scm:149: ;; not 'default to calm display-lily-tests.scm i don't understand this comment... http://codereview.appspot.com/6813044/diff/6001/scm/parser-clef.scm#newcode170 scm/parser-clef.scm:170: (define-public (make-cue-clef-set clef-name) Again, this piece of code looks the same as make-clef-set. Could it be merged? http://codereview.appspot.com/6813044/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel