PartCombine: Implement part-combine texts on the first real note

PartCombine: Implement part-combine texts on the first real note

-) If the context property partCombineTextsOnNote is set, the part-combine
   texts are not printed immediately if there is a rest, but on the next
	 following note. If the voice contains a note, they are printed immediately
	 This is needed if one voice has a full measure rest and the other e.g.
	 r2 r4 c4.
-) The event triggering the part-combine text is cached in that case
   and the Part_combine_engraver now also listens to note events. The
	 texts are then created at the first moment when a note is encountered.

[Valentin Villenave, 2010-11-25 11:21:58]

Hi Reinhold,
it looks quite good; I only have minor comments so hopefully someone more
experienced than me will have more to say :)

http://codereview.appspot.com/3285042/diff/1/lily/part-combine-engraver.cc
File lily/part-combine-engraver.cc (right):

http://codereview.appspot.com/3285042/diff/1/lily/part-combine-engraver.cc#ne...
lily/part-combine-engraver.cc:49: bool have_note;
Wouldn't "has_note" be more informative? Or perhaps I misunderstood the logic
here.

http://codereview.appspot.com/3285042/diff/1/lily/part-combine-engraver.cc#ne...
lily/part-combine-engraver.cc:102: if (have_note || !to_boolean (get_property
("partCombineTextsOnNote")))
Are you sure you want to use a plural in the property name? I'm not sure that's
what we do usually (other than "fretboards" etc.): see e.g. tieWaitForNote, all
singular.

[Neil Puttock, 2010-11-25 17:45:21]

http://codereview.appspot.com/3285042/diff/1/input/regression/part-combine-te...
File input/regression/part-combine-text-wait.ly (right):

http://codereview.appspot.com/3285042/diff/1/input/regression/part-combine-te...
input/regression/part-combine-text-wait.ly:10: \version "2.13.41"
move to top

http://codereview.appspot.com/3285042/diff/1/input/regression/part-combine-te...
input/regression/part-combine-text-wait.ly:13: \set Score.partCombineTextsOnNote
= ##t
indent

http://codereview.appspot.com/3285042/diff/1/input/regression/part-combine-te...
input/regression/part-combine-text-wait.ly:18: c4 r2. |
indent

http://codereview.appspot.com/3285042/diff/1/input/regression/part-combine-te...
input/regression/part-combine-text-wait.ly:24: \new Staff \partcombine \mI \mII
indent

http://codereview.appspot.com/3285042/diff/1/lily/part-combine-engraver.cc
File lily/part-combine-engraver.cc (right):

http://codereview.appspot.com/3285042/diff/1/lily/part-combine-engraver.cc#ne...
lily/part-combine-engraver.cc:91: text_ = make_item ("CombineTextScript",
/*ev?(ev->self_scm ()):*/SCM_EOL);
What's up with the event-cause setting?

http://codereview.appspot.com/3285042/diff/1/lily/part-combine-engraver.cc#ne...
lily/part-combine-engraver.cc:102: if (have_note || !to_boolean (get_property
("partCombineTextsOnNote")))
On 2010/11/25 11:21:58, Valentin Villenave wrote:
> Are you sure you want to use a plural in the property name? I'm not sure
that's
> what we do usually (other than "fretboards" etc.): see e.g. tieWaitForNote,
all
> singular.

I don't mind either way, but the plural's in keeping with printPartCombineTexts.

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

http://codereview.appspot.com/3285042/diff/1/scm/define-context-properties.sc...
scm/define-context-properties.scm:365: note rather than immediately")
could be more specific (if, as I understand it, `immediately' only applies to
rests).

[Reinhold, 2010-11-26 15:07:01]

Included all comments/suggestions and pushed to master.

http://codereview.appspot.com/3285042/diff/1/lily/part-combine-engraver.cc
File lily/part-combine-engraver.cc (right):

http://codereview.appspot.com/3285042/diff/1/lily/part-combine-engraver.cc#ne...
lily/part-combine-engraver.cc:49: bool have_note;
On 2010/11/25 11:21:58, Valentin Villenave wrote:
> Wouldn't "has_note" be more informative? Or perhaps I misunderstood the logic
> here.

I've renamed that variable to note_found_ to make it clearer. (I was thinking of
"we have a tie at that moment"

http://codereview.appspot.com/3285042/diff/1/lily/part-combine-engraver.cc#ne...
lily/part-combine-engraver.cc:91: text_ = make_item ("CombineTextScript",
/*ev?(ev->self_scm ()):*/SCM_EOL);
On 2010/11/25 17:45:21, Neil Puttock wrote:
> What's up with the event-cause setting?

Oops, commented out that part to track down a crash and missed it later on. ev
will not be 0 at that line, since lilypond will have already crashed due to the
first line, anyway. Plus, create_item is only called for ev!=0.

http://codereview.appspot.com/3285042/diff/1/lily/part-combine-engraver.cc#ne...
lily/part-combine-engraver.cc:102: if (have_note || !to_boolean (get_property
("partCombineTextsOnNote")))
On 2010/11/25 17:45:21, Neil Puttock wrote:
> I don't mind either way, but the plural's in keeping with
printPartCombineTexts.

Exactly, I wanted to stay consistent with that property. of course, we can
always rename both properties...

Other properties using plural are e.g. alignBassFigureAccidentals,
extendersOverRests, figuredBassCenterContinuations, harmonicDots,
includeGraceNotes, etc.