On Fri, Nov 2, 2012 at 10:20 PM, Marc Hohl <[email protected]> wrote: > Am 02.11.2012 09:58, schrieb [email protected]: >> 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 ;-)
Maybe there should be a warning? >> 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. ah, ok. Missed 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... > > 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. ok. It may be a good idea to reword the comment a bit, but it's not very important. >> 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. ok, let's leave it alone for now. LGTM thanks! _______________________________________________ lilypond-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/lilypond-devel
