Re: Allows optional octavation for clefs (issue 6813044)

[Marc Hohl]

Am 03.11.2012 23:20, schrieb 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?
Hmmm – if you do a 'grep ly_is_procedure' on the sources, there are
a lot of hits. I randomly picked about ten and never found that an
error or a warning message is raised, so I just follow the style 
guidelines ;-)
> > > 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.
Perhaps I come up with a better wording.
> > > 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!
Thanks for your time to review!

Marc
>