lilypond-devel
[Top][All Lists]
Advanced

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

Allows optional octavation for clefs (issue 6813044)


From: janek . lilypond
Subject: Allows optional octavation for clefs (issue 6813044)
Date: Fri, 02 Nov 2012 08:58:38 +0000

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/



reply via email to

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