Clef support for cue notes

Clef support for cue notes

-) Added \cleffedCueDuring, which allows to specify a clef for
   the cue notes. At the end of the cue section, the clef is
   automatically reset to the containing voice's clef.
-) Cue clefs are implemented as CueClef and CueEndClef grobs,
   created by a dedicated Cue_clef_engraver, which reads
   some cueClef* context properties.
-) After a line break, a cue clef does NOT override the global clef
   of the containing voice, but prints (in smaller size) after
   the containing clef.

[Carl, 2010-10-31 18:39:38]

LGTM.  Nicely done!

Carl

http://codereview.appspot.com/2726043/diff/1/input/regression/cue-clef.ly
File input/regression/cue-clef.ly (right):

http://codereview.appspot.com/2726043/diff/1/input/regression/cue-clef.ly#new...
input/regression/cue-clef.ly:1: \version "2.11.39"
2.13.39

http://codereview.appspot.com/2726043/diff/1/lily/cue-clef-engraver.cc
File lily/cue-clef-engraver.cc (right):

http://codereview.appspot.com/2726043/diff/1/lily/cue-clef-engraver.cc#newcode5
lily/cue-clef-engraver.cc:5: Mats Bengtsson <matsb@s3.kth.se>
Is this copyright statement correct?

[Valentin Villenave, 2010-10-31 19:32:49]

Hi Reinhold,

it certainly looks good! I haven't tested your patch set though, so these are
just a couple of nitpicks off the top of my head.

http://codereview.appspot.com/2726043/diff/1/input/regression/cue-clef.ly
File input/regression/cue-clef.ly (right):

http://codereview.appspot.com/2726043/diff/1/input/regression/cue-clef.ly#new...
input/regression/cue-clef.ly:4: texidoc = "Clefs for cue notes: Normal clefs
should be printed, and in addition
Is it "Normal" that this word is capitalized?

http://codereview.appspot.com/2726043/diff/1/lily/pitch-scheme.cc
File lily/pitch-scheme.cc (right):

http://codereview.appspot.com/2726043/diff/1/lily/pitch-scheme.cc#newcode175
lily/pitch-scheme.cc:175: }
Just out of curiosity: have you checked that it isn't affected by #466?

http://codereview.appspot.com/2726043/diff/1/ly/engraver-init.ly
File ly/engraver-init.ly (right):

http://codereview.appspot.com/2726043/diff/1/ly/engraver-init.ly#newcode79
ly/engraver-init.ly:79: 
Are you sure that we want to \consist the Cue_clef_engraver by default? Most of
the time, it won't be needed at all... (I'm searching for a way to conditionally
load it when needed, but I can't find any though.)

http://codereview.appspot.com/2726043/diff/1/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/2726043/diff/1/ly/music-functions-init.ly#newco...
ly/music-functions-init.ly:219: (make-cue-clef-set type))
I'm not sure this is the proper indentation for .ly code.

[Graham Percival, 2010-10-31 19:43:24]

On Sun, Oct 31, 2010 at 07:32:50PM +0000, v.villenave@gmail.com wrote:
>
http://codereview.appspot.com/2726043/diff/1/input/regression/cue-clef.ly#new...
> input/regression/cue-clef.ly:4: texidoc = "Clefs for cue notes: Normal
> clefs should be printed, and in addition
> Is it "Normal" that this word is capitalized?

In correct English, that word would be capitalized.  However, most
people don't bother to write that well.  So it's not normal to see
this capitalized correctly.  :)

Cheers,
- Graham

[lemzwerg, 2010-10-31 19:59:26]

Oops, somehow I've just created an empty comment.

http://codereview.appspot.com/2726043/diff/1/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/2726043/diff/1/ly/music-functions-init.ly#newco...
ly/music-functions-init.ly:238: cleffedCueDuring =
I very much dislike this name.  What about \cueDuringWithClef?   English
speaking people are very creative in creating verbs out of nouns, but somehow
`cleffed' really hurts to me...

[Valentin Villenave, 2010-10-31 22:12:40]

On Sun, Oct 31, 2010 at 8:43 PM, Graham Percival
<graham@percival-music.ca> wrote:
> In correct English, that word would be capitalized.  However, most
> people don't bother to write that well.  So it's not normal to see
> this capitalized correctly.  :)

Interesting. Could you elaborate?

On Sun, Oct 31, 2010 at 8:59 PM,  <lemzwerg@googlemail.com> wrote:
> I very much dislike this name.  What about \cueDuringWithClef?   English
> speaking people are very creative in creating verbs out of nouns, but
> somehow `cleffed' really hurts to me...

Ditto. I didn't want to point this out, but cleffed is plain ugly (at
least to my French ears ;)

Cheers.

[James.Lowe_datacore.com, 2010-10-31 22:26:55]

-----Original Message-----
From: lilypond-devel-bounces+james.lowe=datacore.com@gnu.org on behalf of
Valentin Villenave
Sent: Sun 31/10/2010 22:12
To: Graham Percival
Cc: reply@codereview.appspotmail.com; lilypond-devel@gnu.org
Subject: Re: Clef support for cue notes (issue2726043)
 
On Sun, Oct 31, 2010 at 8:43 PM, Graham Percival
<graham@percival-music.ca> wrote:
> In correct English, that word would be capitalized.  However, most
> people don't bother to write that well.  So it's not normal to see
> this capitalized correctly.  :)

Interesting. Could you elaborate?

--

If it's being used as a 'proper noun' it would be capitalised.

But it seems the rules are slightly different for American English (which our
Docs are in), here from wikipedia (so it must be true!)....

"In English, the word following the colon is in lower case unless it is a proper
noun or an acronym, or if it is normally capitalized for some other reason.
However, in American English a colon may be followed either by a capital letter
or by a lower-case letter, depending on usage; where direct speech follows, a
capital letter is used; where an acronym or proper noun follows, a capital is
used; otherwise, a lower-case letter is used. Some modern American style guides,
including those published by the Associated Press and the Modern Language
Association, prescribe capitalization where the colon is followed by an
independent clause (i.e. a complete sentence). However, The Chicago Manual of
Style requires capitalization only when the colon introduces two or more
complete sentences."

So either this is a proper noun or an independent clause.

James

PS It looks wrong to my eyes BTW (but I'm not American...or Canadian ;) )

[Trevor Daniels, 2010-11-01 17:22:12]

Code not checked; and I still don't understand Scheme indentation, but at least
it ought to be consistent.

Trevor

http://codereview.appspot.com/2726043/diff/1/input/regression/cue-clef.ly
File input/regression/cue-clef.ly (right):

http://codereview.appspot.com/2726043/diff/1/input/regression/cue-clef.ly#new...
input/regression/cue-clef.ly:4: texidoc = "Clefs for cue notes: Normal clefs
should be printed, and in addition
On 2010/10/31 19:32:50, Valentin Villenave wrote:
> Is it "Normal" that this word is capitalized?
Not normally, but in this case it is correct, even in English English.  The part
before the colon is a heading, and the actual sentence begins after the colon,
so has a capitalised first word.

http://codereview.appspot.com/2726043/diff/1/lily/cue-clef-engraver.cc
File lily/cue-clef-engraver.cc (right):

http://codereview.appspot.com/2726043/diff/1/lily/cue-clef-engraver.cc#newcod...
lily/cue-clef-engraver.cc:108: if (!clef_)
Indentation?

http://codereview.appspot.com/2726043/diff/1/scm/define-context-properties.scm
File scm/define-context-properties.scm (right):

http://codereview.appspot.com/2726043/diff/1/scm/define-context-properties.sc...
scm/define-context-properties.scm:250: (forceCueClef ,boolean? "Show cue  clef
symbol, even if it has not
spacing

http://codereview.appspot.com/2726043/diff/1/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/2726043/diff/1/scm/define-grobs.scm#newcode572
scm/define-grobs.scm:572: (break-align-anchor .
,ly:break-aligned-interface::calc-extent-aligned-anchor)
Indentation

http://codereview.appspot.com/2726043/diff/1/scm/define-grobs.scm#newcode594
scm/define-grobs.scm:594: (break-align-anchor .
,ly:break-aligned-interface::calc-extent-aligned-anchor)
Indentation

[Neil Puttock, 2010-11-25 22:50:40]

Hi Reinhold,

I think there's some scope for reducing code duplication in the engraver and
parser-clef.scm.

Cheers,
Neil

http://codereview.appspot.com/2726043/diff/1/input/regression/cue-clef.ly
File input/regression/cue-clef.ly (right):

http://codereview.appspot.com/2726043/diff/1/input/regression/cue-clef.ly#new...
input/regression/cue-clef.ly:4: texidoc = "Clefs for cue notes: Normal clefs
should be printed, and in addition
indent

this looks like several tests bundled together

http://codereview.appspot.com/2726043/diff/1/input/regression/cue-clef.ly#new...
input/regression/cue-clef.ly:12: \clef "bass"
indent

http://codereview.appspot.com/2726043/diff/1/lily/cue-clef-engraver.cc
File lily/cue-clef-engraver.cc (right):

http://codereview.appspot.com/2726043/diff/1/lily/cue-clef-engraver.cc#newcode41
lily/cue-clef-engraver.cc:41: Direction octave_dir_;
remove

(appears to be left over in clef-engraver.cc from old ottava code)

http://codereview.appspot.com/2726043/diff/1/lily/cue-clef-engraver.cc#newcode47
lily/cue-clef-engraver.cc:47: //   DECLARE_TRANSLATOR_LISTENER (cue_end_clef);
remove?

http://codereview.appspot.com/2726043/diff/1/lily/cue-clef-engraver.cc#newcode74
lily/cue-clef-engraver.cc:74: octave_dir_ = CENTER;
remove

http://codereview.appspot.com/2726043/diff/1/lily/cue-clef-engraver.cc#newcod...
lily/cue-clef-engraver.cc:100: Item *item = dynamic_cast<Item *> (info.grob ());
Item *item = info.item ();

http://codereview.appspot.com/2726043/diff/1/lily/cue-clef-engraver.cc#newcod...
lily/cue-clef-engraver.cc:121: Item *g = make_item ("OctavateEight", SCM_EOL);
farm out to separate function?

http://codereview.appspot.com/2726043/diff/1/lily/cue-clef-engraver.cc#newcod...
lily/cue-clef-engraver.cc:198: if (scm_is_string (glyph)) {
remove { }

http://codereview.appspot.com/2726043/diff/1/lily/cue-clef-engraver.cc#newcod...
lily/cue-clef-engraver.cc:200: } else {
remove { }

http://codereview.appspot.com/2726043/diff/1/lily/cue-clef-engraver.cc#newcod...
lily/cue-clef-engraver.cc:220: if (clef_ && octavate_) {
newline for {

http://codereview.appspot.com/2726043/diff/1/lily/pitch-scheme.cc
File lily/pitch-scheme.cc (right):

http://codereview.appspot.com/2726043/diff/1/lily/pitch-scheme.cc#newcode173
lily/pitch-scheme.cc:173: if (scm_is_number (cue_pos)) {
remove { }

http://codereview.appspot.com/2726043/diff/1/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/2726043/diff/1/ly/music-functions-init.ly#newco...
ly/music-functions-init.ly:250: 'origin location))
don't need to set 'origin (it's done automatically by the syntax constructor
`music-function')

http://codereview.appspot.com/2726043/diff/1/scm/music-functions.scm
File scm/music-functions.scm (right):

http://codereview.appspot.com/2726043/diff/1/scm/music-functions.scm#newcode802
scm/music-functions.scm:802: (make-music 'Music)
indent

http://codereview.appspot.com/2726043/diff/1/scm/music-functions.scm#newcode806
scm/music-functions.scm:806: (context-spec-music (make-voice-props-revert) 
'CueVoice "cue")
remove extra space

http://codereview.appspot.com/2726043/diff/1/scm/music-functions.scm#newcode808
scm/music-functions.scm:808: (make-music 'Music)
indent

[Reinhold, 2010-12-27 16:40:09]

On 2010/11/25 22:50:40, Neil Puttock wrote:
> I think there's some scope for reducing code duplication in the engraver and
> parser-clef.scm.

In an ideal C++ world, that would be very easy by deriving Cue_clef_engraver
from Clef_engraver. However, in lilypond the engravers should not have any
hierarchy, so I don't see how I could easily share some common functionality
between two different engravers (where the code accesses some class variables of
the engravers).

[Carl, 2010-12-27 19:27:05]

On 2010/12/27 16:40:09, Reinhold wrote:
> On 2010/11/25 22:50:40, Neil Puttock wrote:
> > I think there's some scope for reducing code duplication in the engraver and
> > parser-clef.scm.
> 
>  However, in lilypond the engravers should not have any
> hierarchy, so I don't see how I could easily share some common functionality
> between two different engravers (where the code accesses some class variables
of
> the engravers).

Can't you put functions in a new file lily/clef.cc, and pass the class variables
to those functions as arguments?

Thanks,

Carl

[Reinhold, 2010-12-28 20:26:54]

http://codereview.appspot.com/2726043/diff/1/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/2726043/diff/1/ly/music-functions-init.ly#newco...
ly/music-functions-init.ly:250: 'origin location))
On 2010/11/25 22:50:40, Neil Puttock wrote:
> don't need to set 'origin (it's done automatically by the syntax constructor
> `music-function')

Okay, I just copied this from cueDuring. I notice that lots of definitions in
music-functions-init.ly contain 'origin location. So, can we remove it from all
those definitions without breaking anything?

[Neil Puttock, 2010-12-28 23:20:45]

Hi Reinhold,

You're missing definitions for explicitCueClefVisibility and quoted-music-clef.

Cheers,
Neil

http://codereview.appspot.com/2726043/diff/21001/Documentation/changes.tely
File Documentation/changes.tely (right):

http://codereview.appspot.com/2726043/diff/21001/Documentation/changes.tely#n...
Documentation/changes.tely:71: clef, which is correctly reset at the end of the
cue notes. At the begin
notes.  At

http://codereview.appspot.com/2726043/diff/21001/Documentation/changes.tely#n...
Documentation/changes.tely:79: \clef "bass"
indent

http://codereview.appspot.com/2726043/diff/21001/Documentation/changes.tely#n...
Documentation/changes.tely:81: c4 \cueDuringWithClef #"vIQuote" #DOWN #"treble"
{ r4 r2 |
{
  r4 r2 |

http://codereview.appspot.com/2726043/diff/21001/Documentation/changes.tely#n...
Documentation/changes.tely:82: r4 } c4 c2 |
  r4
}

http://codereview.appspot.com/2726043/diff/21001/Documentation/changes.tely#n...
Documentation/changes.tely:83: \cueDuringWithClef #"vIQuote" #DOWN "soprano"
{R1*2 \break R1 } |
{ R1*2

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-be...
File input/regression/cue-clef-begin-of-score.ly (right):

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-be...
input/regression/cue-clef-begin-of-score.ly:1: \version "2.13.41"
2.13.45

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-be...
input/regression/cue-clef-begin-of-score.ly:4: texidoc = "Clefs for cue notes at
the start of a score should print the
indent

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-be...
input/regression/cue-clef-begin-of-score.ly:12: \clef "bass"
indent

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-ne...
File input/regression/cue-clef-new-line.ly (right):

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-ne...
input/regression/cue-clef-new-line.ly:1: \version "2.13.41"
2.13.45

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-ne...
input/regression/cue-clef-new-line.ly:4: texidoc = "Clefs for cue notes and line
breaks. If the cue notes start in a
breaks.  If

indent

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-ne...
input/regression/cue-clef-new-line.ly:5: new line, the cue clef should not be
printed at the end of the previuos line.
previous

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-ne...
input/regression/cue-clef-new-line.ly:9: Cue notes going over a line break
should print the standard clef in the new
on the new

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-ne...
input/regression/cue-clef-new-line.ly:17: \clef "bass"
indent

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-oc...
File input/regression/cue-clef-octavation.ly (right):

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-oc...
input/regression/cue-clef-octavation.ly:1: \version "2.13.41"
2.13.45

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-oc...
input/regression/cue-clef-octavation.ly:4: texidoc = "Octavation for clefs for
cue notes."
indent

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-oc...
input/regression/cue-clef-octavation.ly:11: \clef "treble_8" c1 |
indent

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-oc...
input/regression/cue-clef-octavation.ly:13: c1 |\break
| \break

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef-oc...
input/regression/cue-clef-octavation.ly:18: \cueDuringWithClef #"vIQuote" #UP
#"treble_8" { R1 \break R} |
R }

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef.ly
File input/regression/cue-clef.ly (right):

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef.ly...
input/regression/cue-clef.ly:1: \version "2.13.41"
2.13.45

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef.ly...
input/regression/cue-clef.ly:3: % FIXME: Split this up into several different
test cases
remove

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef.ly...
input/regression/cue-clef.ly:5: texidoc = "Clefs for cue notes: Print a cue clef
at the begin of the cue
indent

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef.ly...
input/regression/cue-clef.ly:13: \clef "bass"
indent

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef.ly...
input/regression/cue-clef.ly:14: c4 \cueDuringWithClef #"vIQuote" #DOWN
#"treble" { r4 r2 |
formatting { }

http://codereview.appspot.com/2726043/diff/21001/input/regression/cue-clef.ly...
input/regression/cue-clef.ly:16: \cueDuringWithClef #"vIQuote" #DOWN "soprano"
{R1*2 } |
{ R1*2

http://codereview.appspot.com/2726043/diff/21001/lily/cue-clef-engraver.cc
File lily/cue-clef-engraver.cc (right):

http://codereview.appspot.com/2726043/diff/21001/lily/cue-clef-engraver.cc#ne...
lily/cue-clef-engraver.cc:235: "cueClefPosition "
+ explicitCueClefVisibility

http://codereview.appspot.com/2726043/diff/21001/scm/music-functions.scm
File scm/music-functions.scm (right):

http://codereview.appspot.com/2726043/diff/21001/scm/music-functions.scm#newc...
scm/music-functions.scm:802: (make-music 'Music)
indent

http://codereview.appspot.com/2726043/diff/21001/scm/music-functions.scm#newc...
scm/music-functions.scm:806: (context-spec-music (make-voice-props-revert) 
'CueVoice "cue")
) 'CueVoice

http://codereview.appspot.com/2726043/diff/21001/scm/music-functions.scm#newc...
scm/music-functions.scm:808: (make-music 'Music)
indent

[Neil Puttock, 2010-12-28 23:32:18]

On 2010/12/28 20:26:54, Reinhold wrote:

> Okay, I just copied this from cueDuring. I notice that lots of definitions in
> music-functions-init.ly contain 'origin location. So, can we remove it from
all
> those definitions without breaking anything?

It should be safe to remove 'origin from all the music functions which are basic
`make-music' constructs (so long as they don't set 'origin on any child music).

Cheers,
Neil

[Reinhold, 2010-12-29 00:47:50]

Thanks, Neil, for all your comments (and indent nitpicks ;) ).

I have included/fixed all of them (and some other minor issues) and posted the
patch again (needs my other patch for the OctavateEight break-visibility to get
octavated cue clefs right). Any further comments?

Cheers,
Reinhold

[Neil Puttock, 2010-12-29 00:57:11]

LGTM.

[Reinhold, 2010-12-29 01:00:21]

On 2010/12/28 23:32:18, Neil Puttock wrote:
> On 2010/12/28 20:26:54, Reinhold wrote:
> 
> > Okay, I just copied this from cueDuring. I notice that lots of definitions
in
> > music-functions-init.ly contain 'origin location. So, can we remove it from
> all
> > those definitions without breaking anything?
> 
> It should be safe to remove 'origin from all the music functions which are
basic
> `make-music' constructs (so long as they don't set 'origin on any child
music).


Okay, I removed all trivial 'origin settings:
http://codereview.appspot.com/3847041

I didn't look at the more complex function, where I suspect your parenthesized
condition comes into play...

[felipeg.assis_gmail.com, 2010-12-29 08:06:03]

Hi,

I have just updated my local repository, and noticed that this
patch introduces a compilation error.

In fact, there is an undefined variable in lily/cue-clef-engraver.cc:175
(compare with lily/clef-engraver.cc). I am attaching a simple patch
that solves the issue, but you might want to review the related code.

Regards,
Felipe

On 28 December 2010 21:57,  <n.puttock@gmail.com> wrote:
> LGTM.
>
> http://codereview.appspot.com/2726043/
>
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/lilypond-devel
>

[reinhold_kainhofer.com, 2010-12-29 10:58:36]

Am Mittwoch, 29. Dezember 2010, um 09:06:01 schrieb Felipe Gonçalves Assis:
> I have just updated my local repository, and noticed that this
> patch introduces a compilation error.
> 
> In fact, there is an undefined variable in lily/cue-clef-engraver.cc:175
> (compare with lily/clef-engraver.cc). I am attaching a simple patch
> that solves the issue, but you might want to review the related code.

Thanks, it seems I missed this occurrence of forceClef, when I ripped out that 
feature (I don't see any need for it with cue clefs). I have no idea why the 
compilation didn't fail here, though...

Cheers,
Reinhold

-- 
------------------------------------------------------------------
Reinhold Kainhofer, reinhold@kainhofer.com, http://reinhold.kainhofer.com/
 * Financial & Actuarial Math., Vienna Univ. of Technology, Austria
 * http://www.fam.tuwien.ac.at/, DVR: 0005886
 * LilyPond, Music typesetting, http://www.lilypond.org