Re: Allows optional octavation for clefs (issue 6813044)

[Janek Warchoł]

On Fri, Nov 2, 2012 at 10:20 PM, Marc Hohl <address@hidden> wrote:
> Am 02.11.2012 09:58, schrieb address@hidden:
>> 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!