Allows for automatic renumbering of measure numbers at volta repeats.

Allows for automatic renumbering of measure numbers at volta repeats.

[MikeSol, 2011-11-28 21:59:42]

I'm not sure if this is the best way to go about it, but it seems to get the job
done!

Cheers,
MS

[Carl, 2011-11-29 00:04:52]

Thanks for taking this on.  I sense that this is a step in the direction of
helping LilyPond really understand codas and segnos.

But the terminology is unclear for this musically unsophisticated user.  And I'm
not sure I really understand how it all works.

Thanks,

Carl

http://codereview.appspot.com/5440049/diff/1/lily/timing-translator.cc
File lily/timing-translator.cc (right):

http://codereview.appspot.com/5440049/diff/1/lily/timing-translator.cc#newcod...
lily/timing-translator.cc:172: "renvoiDir "
I'm not comfortable at this point with the names renvoiDir and renvoiStyle. 
What do you mean by renvoi?  

The Virginia Tech online music dictionary has no entry for renvoi.  The
Dolmetsch online dictionary lists renvoi as the repeat sign to which dal segno
refers.  renvoi to me appears to be a french word that carries very little, if
any, english meaning.  But I'm not a music professional, so perhaps I'm all wet
on this one.

I would expect something more like alternativeBarNumberStyle.  And I don't know
what alternative I would propose for renvoiDir, since I don't really grok its
meaning right now.

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

http://codereview.appspot.com/5440049/diff/1/scm/define-context-properties.sc...
scm/define-context-properties.scm:416: (renvoiDir ,ly:dir? "The direction of a
volta send-back.")
What are the possibilities of directions?  Can it be either backward or forward
in time?  Is that why we need renvoiDir?

What is a "volta send-back"?

http://codereview.appspot.com/5440049/diff/1/scm/define-context-properties.sc...
scm/define-context-properties.scm:417: (renvoiStyle ,symbol? "The style of a
volta send-back.")
What are the possible styles, and where are they documented?

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

http://codereview.appspot.com/5440049/diff/1/scm/define-music-properties.scm#...
scm/define-music-properties.scm:110: (last-renvoi ,ly:dir? "Is this the last
send-back for a volta.")
The property description indicates a boolean value.  But the type indicates a
direction (as in renvoi-dir.  Is this supposed to be a boolean, or is it
supposed to be a direction?

[Keith, 2011-11-29 03:54:42]

Works nicely.

I'm inspired to name some variables in Irish now!  And, I'll be sure the names
are misleadingly related, but critically distinct, from the conceptual contents
of the variable.

http://codereview.appspot.com/5440049/diff/1/lily/bar-number-engraver.cc
File lily/bar-number-engraver.cc (right):

http://codereview.appspot.com/5440049/diff/1/lily/bar-number-engraver.cc#newc...
lily/bar-number-engraver.cc:38: int renvoi_number_;
maybe "alternative_number_"
Isn't there already a counter for that somewhere?
Nested \alternatives fool it, but I couldn't make it crash so that should be
fine.

http://codereview.appspot.com/5440049/diff/1/lily/bar-number-engraver.cc#newc...
lily/bar-number-engraver.cc:64: SCM renvoi_dir_scm = context ()->get_property
("renvoiDir");
maybe "FirstOrLastAlternative"
I guess you made it a direction for type-checking purposes?

http://codereview.appspot.com/5440049/diff/1/lily/bar-number-engraver.cc#newc...
lily/bar-number-engraver.cc:65: SCM renvoi_style = context ()->get_property
("renvoiStyle");
"AlternateNumberingStyle"

http://codereview.appspot.com/5440049/diff/1/lily/include/timing-translator.hh
File lily/include/timing-translator.hh (right):

http://codereview.appspot.com/5440049/diff/1/lily/include/timing-translator.h...
lily/include/timing-translator.hh:28: int renvoi_;
maybe "alternative_starting_bar_number"

http://codereview.appspot.com/5440049/diff/1/lily/timing-translator.cc
File lily/timing-translator.cc (right):

http://codereview.appspot.com/5440049/diff/1/lily/timing-translator.cc#newcode85
lily/timing-translator.cc:85: bool make_renvoi = context ()->get_property
("renvoiStyle") == ly_symbol2scm ("repeat-measure-numbers")
"numbers"

http://codereview.appspot.com/5440049/diff/1/lily/timing-translator.cc#newcode86
lily/timing-translator.cc:86: || context ()->get_property ("renvoiStyle") ==
ly_symbol2scm ("repeat-measure-numbers-with-letter-tags");
"numbers-with-letters"

http://codereview.appspot.com/5440049/diff/1/lily/timing-translator.cc#newcode90
lily/timing-translator.cc:90: if (renvoi_dir == -1)
Desperately seeking comment.

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

http://codereview.appspot.com/5440049/diff/1/scm/define-music-properties.scm#...
scm/define-music-properties.scm:174: (renvoi-dir ,ly:dir? "The direction of a
volta send-back.")
Seems more like: indicates if an AlternativeStart event is the 
First (-1), Middle (0), Last (1)
of group of alternate endings.

http://codereview.appspot.com/5440049/diff/1/scm/define-music-types.scm
File scm/define-music-types.scm (right):

http://codereview.appspot.com/5440049/diff/1/scm/define-music-types.scm#newco...
scm/define-music-types.scm:453: (RenvoiMusic
maybe "AlternativeStart"
Does it need an iterator? does it contain the alternative or is it merely an
event marking the start?

[pkx166h, 2011-11-29 07:46:24]

Passes Make and make check just gives me

--snip--

/home/jlowe/lilypond-git/input/regression/display-lily-tests.log

@@ -13,6 +13,9 @@
 Test 106 unequal: NOT A BUG. 
 in  = \transpose c d { c d }
 out = { d e }
+Test 111 unequal: . 
+in  = \repeat volta 2 { c4 d } \alternative { { c d } { e f } }
+out = \repeat volta 2 { c4 d } \alternative { { %{ Print method not implemented
for music type RenvoiMusic %} { c d } } { %{ Print method not implemented for
music type RenvoiMusic %} { e f } %{ Print method not implemented for music type
RenvoiMusic %} } }
 Test 125 unequal: NOT A BUG. 
 in  = \oneVoice
 out = {

--snip--

So I think this is ok.


James

[MikeSol, 2011-11-29 12:08:52]

I'll just say to all you haters of `renvoi' that:

--) There is no English term that I know of that succinctly expresses the idea
of "being sent back to something."  And, as new terms are invented out of
necessity, I took the liberty to invent one here with `renvoi,' which
Jankélévitch (musicologist) uses all the time to evoke a notion of "going back
to" with cryptic phrases like "La mort propre est un renvoi de l'au-delà à l'en
deçà." (One's own death is a send-back from the great beyond to corporeal
reality,) (NB this is not a real Jankélévitch quote...but it may as well have
been...).
--) Renvoi also means `belch'.  For this reason alone it deserves to be part of
the source.

But, deferring to the clamors of my detractors, I have changed it.

Regtest changed a bit too and all other comments summarily responded to.

The only problem that remains is that the display-lily-tests shows a one-space
increase in the new output.  I've made the string rendered for AlternativeEvent
"", but is there any way just to get the display method to ignore it (meaning
not add the extra whitespace)?  If not, this extra whitespace doesn't bother me
and I'll add it as "NOT A BUG", but I'd rather scrub it if possible.

Cheers,
MS

http://codereview.appspot.com/5440049/diff/1/lily/bar-number-engraver.cc
File lily/bar-number-engraver.cc (right):

http://codereview.appspot.com/5440049/diff/1/lily/bar-number-engraver.cc#newc...
lily/bar-number-engraver.cc:38: int renvoi_number_;
On 2011/11/29 03:54:42, Keith wrote:
> maybe "alternative_number_"
> Isn't there already a counter for that somewhere?
> Nested \alternatives fool it, but I couldn't make it crash so that should be
> fine.

Done.

http://codereview.appspot.com/5440049/diff/1/lily/bar-number-engraver.cc#newc...
lily/bar-number-engraver.cc:64: SCM renvoi_dir_scm = context ()->get_property
("renvoiDir");
On 2011/11/29 03:54:42, Keith wrote:
> maybe "FirstOrLastAlternative"
> I guess you made it a direction for type-checking purposes?

Done.

http://codereview.appspot.com/5440049/diff/1/lily/bar-number-engraver.cc#newc...
lily/bar-number-engraver.cc:65: SCM renvoi_style = context ()->get_property
("renvoiStyle");
On 2011/11/29 03:54:42, Keith wrote:
> "AlternateNumberingStyle"

Done.

http://codereview.appspot.com/5440049/diff/1/lily/include/timing-translator.hh
File lily/include/timing-translator.hh (right):

http://codereview.appspot.com/5440049/diff/1/lily/include/timing-translator.h...
lily/include/timing-translator.hh:28: int renvoi_;
On 2011/11/29 03:54:42, Keith wrote:
> maybe "alternative_starting_bar_number"

Done.

http://codereview.appspot.com/5440049/diff/1/lily/timing-translator.cc
File lily/timing-translator.cc (right):

http://codereview.appspot.com/5440049/diff/1/lily/timing-translator.cc#newcode85
lily/timing-translator.cc:85: bool make_renvoi = context ()->get_property
("renvoiStyle") == ly_symbol2scm ("repeat-measure-numbers")
On 2011/11/29 03:54:42, Keith wrote:
> "numbers"

Done.

http://codereview.appspot.com/5440049/diff/1/lily/timing-translator.cc#newcode86
lily/timing-translator.cc:86: || context ()->get_property ("renvoiStyle") ==
ly_symbol2scm ("repeat-measure-numbers-with-letter-tags");
On 2011/11/29 03:54:42, Keith wrote:
> "numbers-with-letters"

Done.

http://codereview.appspot.com/5440049/diff/1/lily/timing-translator.cc#newcode90
lily/timing-translator.cc:90: if (renvoi_dir == -1)
On 2011/11/29 03:54:42, Keith wrote:
> Desperately seeking comment.

Done.

http://codereview.appspot.com/5440049/diff/1/lily/timing-translator.cc#newcod...
lily/timing-translator.cc:172: "renvoiDir "
On 2011/11/29 00:04:52, Carl wrote:
> I'm not comfortable at this point with the names renvoiDir and renvoiStyle. 
> What do you mean by renvoi?  
> 
> The Virginia Tech online music dictionary has no entry for renvoi.  The
> Dolmetsch online dictionary lists renvoi as the repeat sign to which dal segno
> refers.  renvoi to me appears to be a french word that carries very little, if
> any, english meaning.  But I'm not a music professional, so perhaps I'm all
wet
> on this one.
> 
> I would expect something more like alternativeBarNumberStyle.  And I don't
know
> what alternative I would propose for renvoiDir, since I don't really grok its
> meaning right now.

Done.

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

http://codereview.appspot.com/5440049/diff/1/scm/define-context-properties.sc...
scm/define-context-properties.scm:416: (renvoiDir ,ly:dir? "The direction of a
volta send-back.")
On 2011/11/29 00:04:52, Carl wrote:
> What are the possibilities of directions?  Can it be either backward or
forward
> in time?  Is that why we need renvoiDir?
> 
> What is a "volta send-back"?

LEFT is the beginning of an alternative
CENTER is each time the alternative repeats
RIGHT is the end of an alternative

It is true that Direction is a bit of a stretch to use directions in this case,
but I'm using them because they correspond to left->right in score space and are
easier than defining a new enum (or enum-like convention of naming scheme
variables).  I may have to fiddle with this to address Keith's concern about
combined volta repeats and skipping letters.

http://codereview.appspot.com/5440049/diff/1/scm/define-context-properties.sc...
scm/define-context-properties.scm:417: (renvoiStyle ,symbol? "The style of a
volta send-back.")
On 2011/11/29 00:04:52, Carl wrote:
> What are the possible styles, and where are they documented?

Thanks for spotting this - I'll document them here.

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

http://codereview.appspot.com/5440049/diff/1/scm/define-music-properties.scm#...
scm/define-music-properties.scm:110: (last-renvoi ,ly:dir? "Is this the last
send-back for a volta.")
On 2011/11/29 00:04:52, Carl wrote:
> The property description indicates a boolean value.  But the type indicates a
> direction (as in renvoi-dir.  Is this supposed to be a boolean, or is it
> supposed to be a direction?

Vestigial, removing.

http://codereview.appspot.com/5440049/diff/1/scm/define-music-properties.scm#...
scm/define-music-properties.scm:174: (renvoi-dir ,ly:dir? "The direction of a
volta send-back.")
On 2011/11/29 03:54:42, Keith wrote:
> Seems more like: indicates if an AlternativeStart event is the 
> First (-1), Middle (0), Last (1)
> of group of alternate endings.

Done.

http://codereview.appspot.com/5440049/diff/1/scm/define-music-types.scm
File scm/define-music-types.scm (right):

http://codereview.appspot.com/5440049/diff/1/scm/define-music-types.scm#newco...
scm/define-music-types.scm:453: (RenvoiMusic
On 2011/11/29 03:54:42, Keith wrote:
> maybe "AlternativeStart"
> Does it need an iterator? does it contain the alternative or is it merely an
> event marking the start?

You're right - it's an event now.

[mike_apollinemike.com, 2011-11-29 12:20:29]

On Nov 29, 2011, at 1:08 PM, mtsolo@gmail.com wrote:

> 
> The only problem that remains is that the display-lily-tests shows a
> one-space increase in the new output.  I've made the string rendered for
> AlternativeEvent "", but is there any way just to get the display method
> to ignore it (meaning not add the extra whitespace)?  If not, this extra
> whitespace doesn't bother me and I'll add it as "NOT A BUG", but I'd
> rather scrub it if possible.
> 

Also, one gotchya is that for this to kick in, alternatives must be
contextualized in curly brackets.  So,

\repeat volta 4 { a1 } \alternative { a b }

Will never work with the current AlternativeEvent implementation, whereas

\repeat volta 4 { a1 } \alternative { { a } { b } }

will.

It is not impossible to add the AlternativeEvent in
Volta_repeat_iterator::get_music_list (), which would get around this problem. 
But it'd be doing a lot of event-management at the iterator level, which is so
far as I know hitherto unprecedented.  Any thoughts?

Cheers,
MS

[mike_apollinemike.com, 2011-11-29 14:43:48]

On Nov 29, 2011, at 1:20 PM, mike@apollinemike.com wrote:

> On Nov 29, 2011, at 1:08 PM, mtsolo@gmail.com wrote:
> 
>> 
>> The only problem that remains is that the display-lily-tests shows a
>> one-space increase in the new output.  I've made the string rendered for
>> AlternativeEvent "", but is there any way just to get the display method
>> to ignore it (meaning not add the extra whitespace)?  If not, this extra
>> whitespace doesn't bother me and I'll add it as "NOT A BUG", but I'd
>> rather scrub it if possible.
>> 
> 
> Also, one gotchya is that for this to kick in, alternatives must be
contextualized in curly brackets.  So,
> 
> \repeat volta 4 { a1 } \alternative { a b }
> 
> Will never work with the current AlternativeEvent implementation, whereas
> 
> \repeat volta 4 { a1 } \alternative { { a } { b } }
> 
> will.
> 
> It is not impossible to add the AlternativeEvent in
Volta_repeat_iterator::get_music_list (), which would get around this problem.  

Fixed, new patch set up.

Cheers,
MS

[pkx166h, 2011-11-29 22:02:00]

Passes Make and Make check

James

[Keith, 2011-11-30 05:29:04]

Now it looks quite good, and still works great.
C'est si bon.
Is maith é sin.
Smashing, old chap.

http://codereview.appspot.com/5440049/diff/11010/lily/bar-number-engraver.cc
File lily/bar-number-engraver.cc (right):

http://codereview.appspot.com/5440049/diff/11010/lily/bar-number-engraver.cc#...
lily/bar-number-engraver.cc:68: if we're starting an alternative, we set the
starting
if we're starting the first alternative,

http://codereview.appspot.com/5440049/diff/11010/lily/bar-number-engraver.cc#...
lily/bar-number-engraver.cc:128: for (int i = 0; i < alternative_number_ / 26;
i++)
I don't think you want to loop.  It gives
 a b c ... z aa ab .. az ba bb .. bz cca ccb ... fffffa fffffb

http://codereview.appspot.com/5440049/diff/11010/lily/bar-number-engraver.cc#...
lily/bar-number-engraver.cc:129: text_tag += alphabet.at (alternative_number_ /
26);
best to do a modulo 26, so that a sticky key typo does not cause a segfault.
\repeat volta 888

http://codereview.appspot.com/5440049/diff/11010/scm/define-music-callbacks.scm
File scm/define-music-callbacks.scm (right):

http://codereview.appspot.com/5440049/diff/11010/scm/define-music-callbacks.s...
scm/define-music-callbacks.scm:45: (append
;; set properties for proper bar numbering

http://codereview.appspot.com/5440049/diff/11010/scm/define-music-callbacks.s...
scm/define-music-callbacks.scm:50: 'alternative-increment
Alternatively, you could ask Volta_repeat_iterator::next_element() to set this
for you when it sets the text for the little number under the volta bracket.

[mike_apollinemike.com, 2011-11-30 09:14:12]

On Nov 30, 2011, at 6:29 AM, k-ohara5a5a@oco.net wrote:

> Now it looks quite good, and still works great.
> C'est si bon.
> Is maith é sin.
> Smashing, old chap.
> 
> 

Thanks!

> 
> http://codereview.appspot.com/5440049/diff/11010/lily/bar-number-engraver.cc
> File lily/bar-number-engraver.cc (right):
> 
>
http://codereview.appspot.com/5440049/diff/11010/lily/bar-number-engraver.cc#...
> lily/bar-number-engraver.cc:68: if we're starting an alternative, we set
> the starting
> if we're starting the first alternative,

Fixed.

> 
>
http://codereview.appspot.com/5440049/diff/11010/lily/bar-number-engraver.cc#...
> lily/bar-number-engraver.cc:128: for (int i = 0; i < alternative_number_
> / 26; i++)
> I don't think you want to loop.  It gives
> a b c ... z aa ab .. az ba bb .. bz cca ccb ... fffffa fffffb
> 

Fixed.

>
http://codereview.appspot.com/5440049/diff/11010/lily/bar-number-engraver.cc#...
> lily/bar-number-engraver.cc:129: text_tag += alphabet.at
> (alternative_number_ / 26);
> best to do a modulo 26, so that a sticky key typo does not cause a
> segfault.
> \repeat volta 888
> 

Fixed.

>
http://codereview.appspot.com/5440049/diff/11010/scm/define-music-callbacks.scm
> File scm/define-music-callbacks.scm (right):
> 
>
http://codereview.appspot.com/5440049/diff/11010/scm/define-music-callbacks.s...
> scm/define-music-callbacks.scm:45: (append
> ;; set properties for proper bar numbering
> 

Fixed.

>
http://codereview.appspot.com/5440049/diff/11010/scm/define-music-callbacks.s...
> scm/define-music-callbacks.scm:50: 'alternative-increment
> Alternatively, you could ask Volta_repeat_iterator::next_element() to
> set this for you when it sets the text for the little number under the
> volta bracket.
> 

Mm...I'm tempted to keep it the way it is, just cuz it's easier to read all of
the properties having to do with the creation of the event in the same place (in
this case, the elements callback).

Cheers,
MS

> http://codereview.appspot.com/5440049/

[pkx166h, 2011-12-01 17:38:24]

Passes make and make check

James

[Neil Puttock, 2011-12-06 12:04:54]

http://codereview.appspot.com/5440049/diff/9003/lily/bar-number-engraver.cc
File lily/bar-number-engraver.cc (right):

http://codereview.appspot.com/5440049/diff/9003/lily/bar-number-engraver.cc#n...
lily/bar-number-engraver.cc:156: scm_string_concatenate (scm_list_2
(scm_number_to_string (bn, scm_from_int (10)),
Would be better to have a barNumberFormatter function to return the markup.

[mike_apollinemike.com, 2011-12-06 12:07:03]

Le Dec 6, 2011 à 1:04 PM, n.puttock@gmail.com a écrit :

> 
> http://codereview.appspot.com/5440049/diff/9003/lily/bar-number-engraver.cc
> File lily/bar-number-engraver.cc (right):
> 
>
http://codereview.appspot.com/5440049/diff/9003/lily/bar-number-engraver.cc#n...
> lily/bar-number-engraver.cc:156: scm_string_concatenate (scm_list_2
> (scm_number_to_string (bn, scm_from_int (10)),
> Would be better to have a barNumberFormatter function to return the
> markup.
> 

Agreed...I'll roll it into the parenthesized patch.  It fits w/ Reinhold's
comments.

Good to see you back on the list!

Cheers,
MS

[Neil Puttock, 2011-12-12 12:07:41]

On 2011/12/06 12:07:03, mike_apollinemike.com wrote:

> Good to see you back on the list!

Cheers mate. :)  I'm afraid I'm only passing through temporarily though. 
Hopefully will get back into the swing of things after Christmas.

Cheers,
Neil