On Fri, Nov 2, 2012 at 10:20 PM, Marc Hohl <marc@hohlart.de> wrote: > Am 02.11.2012 ...
11 years, 4 months ago
(2012-11-03 22:20:48 UTC)
#3
On Fri, Nov 2, 2012 at 10:20 PM, Marc Hohl <marc@hohlart.de> wrote:
> Am 02.11.2012 09:58, schrieb janek.lilypond@gmail.com:
>>
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/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!
Am 03.11.2012 23:20, schrieb Janek Warchoł: > On Fri, Nov 2, 2012 at 10:20 PM, ...
11 years, 4 months ago
(2012-11-05 10:10:55 UTC)
#4
Am 03.11.2012 23:20, schrieb Janek Warchoł:
> On Fri, Nov 2, 2012 at 10:20 PM, Marc Hohl <marc@hohlart.de> wrote:
>> Am 02.11.2012 09:58, schrieb janek.lilypond@gmail.com:
>>>
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/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
>
Issue 6813044: Allows optional octavation for clefs
(Closed)
Created 11 years, 5 months ago by marc
Modified 11 years, 4 months ago
Reviewers: janek
Base URL: http://git.savannah.gnu.org/gitweb/?p=lilypond.git/trunk/
Comments: 9