[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Clef support for cue notes (issue2726043)
From: |
v . villenave |
Subject: |
Re: Clef support for cue notes (issue2726043) |
Date: |
Sun, 31 Oct 2010 19:32:50 +0000 |
Hi Reinhold,
it certainly looks good! I haven't tested your patch set though, so
these are just a couple of nitpicks off the top of my head.
http://codereview.appspot.com/2726043/diff/1/input/regression/cue-clef.ly
File input/regression/cue-clef.ly (right):
http://codereview.appspot.com/2726043/diff/1/input/regression/cue-clef.ly#newcode4
input/regression/cue-clef.ly:4: texidoc = "Clefs for cue notes: Normal
clefs should be printed, and in addition
Is it "Normal" that this word is capitalized?
http://codereview.appspot.com/2726043/diff/1/lily/pitch-scheme.cc
File lily/pitch-scheme.cc (right):
http://codereview.appspot.com/2726043/diff/1/lily/pitch-scheme.cc#newcode175
lily/pitch-scheme.cc:175: }
Just out of curiosity: have you checked that it isn't affected by #466?
http://codereview.appspot.com/2726043/diff/1/ly/engraver-init.ly
File ly/engraver-init.ly (right):
http://codereview.appspot.com/2726043/diff/1/ly/engraver-init.ly#newcode79
ly/engraver-init.ly:79:
Are you sure that we want to \consist the Cue_clef_engraver by default?
Most of the time, it won't be needed at all... (I'm searching for a way
to conditionally load it when needed, but I can't find any though.)
http://codereview.appspot.com/2726043/diff/1/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):
http://codereview.appspot.com/2726043/diff/1/ly/music-functions-init.ly#newcode219
ly/music-functions-init.ly:219: (make-cue-clef-set type))
I'm not sure this is the proper indentation for .ly code.
http://codereview.appspot.com/2726043/
Re: Clef support for cue notes (issue2726043), lemzwerg, 2010/10/31
Re: Clef support for cue notes (issue2726043), lemzwerg, 2010/10/31