Re: Allows optional octavation for clefs (issue 6813044)

[Marc Hohl]

Am 02.11.2012 09:58, schrieb address@hidden:
> Hi Marc,
>
> the amount of comments is actually a compliment - it means that i
> understood almost everything! :)
Ok, that great then ;-)
> 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.
Ok.
> 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)"
Ok.
> 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.
Ok.
> 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?
Yes.

clefOctavationFormatter is set in ly/engraver-init.ly, so by default, it 
is a procedure.
If the user clears the default setting or defines something else 
instead, then he
has to carry the consequences ;-)
> 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.
Yes.
> 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?
In ly/engraver-init.ly, clefOctavationFormatter and 
cueClefOctavationFormatter
are initialized to use the same function, but I think the user should be 
able to
define/choose different functions for each clef type.
> 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?
No. style is predefined as 'default in line 180.
> 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...
As could be seen on

http://code.google.com/p/lilypond/issues/detail?id=2933#c4
http://code.google.com/p/lilypond/issues/detail?id=2933#c12
http://code.google.com/p/lilypond/issues/detail?id=2933#c15

display-lily-tests.scm complains about a new property set to 'default.
David proposed to either change display-lily-tests.scm or to avoid
setting an unused property (which it is, in this case) with a strong
preference to the latter case. So that's what I did.


> 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?
Well, in the original code there were two functions, so I just extended 
them.
I don't see a *simple* way of merging the two functions into one.
> http://codereview.appspot.com/6813044/