Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2855)

Issue 6813044: Allows optional octavation for clefs (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 5 months ago by marc
Modified:
11 years, 4 months ago
Reviewers:
janek
CC:
lilypond-devel_gnu.org
Base URL:
http://git.savannah.gnu.org/gitweb/?p=lilypond.git/trunk/
Visibility:
Public.

Description

Allows optional octavation for clefs Clef specifications like \clef "G_(8)" or \clef "bass^[15]" are supported.

Patch Set 1 #

Patch Set 2 : corrected regexp in scm/parser-clef.scm #

Patch Set 3 : remove predefined value; regexp corrections for cueClefs #

Patch Set 4 : corrected regexp for cue clefs #

Patch Set 5 : additional regtest for cue clefs; handling for 'default improved #

Total comments: 9

Patch Set 6 : Changes according to Janek's proposals #

Unified diffs Side-by-side diffs Delta from patch set Stats (+132 lines, -27 lines) Patch
A input/regression/clef-optional-octavation.ly View 1 2 3 4 5 1 chunk +15 lines, -0 lines 0 comments Download
A input/regression/cue-clef-optional-octavation.ly View 1 2 3 4 5 1 chunk +27 lines, -0 lines 0 comments Download
M lily/clef-engraver.cc View 2 chunks +7 lines, -3 lines 0 comments Download
M lily/cue-clef-engraver.cc View 2 chunks +7 lines, -3 lines 0 comments Download
M ly/engraver-init.ly View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M scm/define-context-properties.scm View 1 2 3 4 2 chunks +10 lines, -0 lines 0 comments Download
M scm/parser-clef.scm View 1 2 3 4 3 chunks +46 lines, -21 lines 0 comments Download
M scm/translation-functions.scm View 1 chunk +18 lines, -0 lines 0 comments Download

Messages

Total messages: 4
janek
Hi Marc, the amount of comments is actually a compliment - it means that i ...
11 years, 4 months ago (2012-11-02 08:58:38 UTC) #1
marc
Am 02.11.2012 09:58, schrieb janek.lilypond@gmail.com: > Hi Marc, > > the amount of comments is ...
11 years, 4 months ago (2012-11-02 21:20:07 UTC) #2
janek
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
marc
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
>

Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b