Fix Tracker 918, Add extra RemoveEmpty*StaffContext functions to support "Frenched" scores

Fix Tracker 918, Add extra RemoveEmpty*StaffContext functions to support "Frenched" scores 
with DrumStaff, TabStaff and RhythmicStaff staves.

Add new definitions in ly/engraver-int.ly and new regression tests.

[Ian Hulin, 2009-12-08 23:14:54]

Patch for Tracker 918 available on Rietveld - Issue 165096. This includes
updated/enhanced regression tests.

[Neil Puttock, 2009-12-10 23:43:35]

Hi Ian,

LGTM, apart from some formatting issues and a few incorrect \version numbers.

Can you sort out the naming of the new regression tests?  For consistency with
the existing test, I'd advise amending them as follows:

hara-kiri-drumstaff.ly
hara-kiri-rhythmicstaff.ly
hara-kiri-tabstaff.ly

Cheers,
Neil

http://codereview.appspot.com/165096/diff/1/2
File input/regression/hara-kiri-drums.ly (right):

http://codereview.appspot.com/165096/diff/1/2#newcode1
input/regression/hara-kiri-drums.ly:1: \version "2.13.8"
2.13.9

http://codereview.appspot.com/165096/diff/1/2#newcode2
input/regression/hara-kiri-drums.ly:2: \header { texidoc =
I know you've just copied the existing example, but it is a bit messy; it would
be preferable to tidy things up here:

\header {
  texidoc = "Hara-kiri ...

http://codereview.appspot.com/165096/diff/1/2#newcode14
input/regression/hara-kiri-drums.ly:14: 
trailing whitespace

http://codereview.appspot.com/165096/diff/1/2#newcode18
input/regression/hara-kiri-drums.ly:18: ragged-right= ##t
ragged-right =

http://codereview.appspot.com/165096/diff/1/2#newcode26
input/regression/hara-kiri-drums.ly:26: \new DrumStaff \drummode {  sn4 sn sn sn
\break s1 \break sn4 sn sn sn \break sn sn sn sn}
\drummode {
  sn4 sn sn sn \break
  s1 break
  sn4 sn sn sn \break
  sn4 sn sn sn
}

etc.

http://codereview.appspot.com/165096/diff/1/3
File input/regression/hara-kiri-percent-repeat.ly (left):

http://codereview.appspot.com/165096/diff/1/3#oldcode8
input/regression/hara-kiri-percent-repeat.ly:8: \new Staff { c''1 c'' \break c''
c'' }
<<
  \new Staff

etc.

http://codereview.appspot.com/165096/diff/1/3
File input/regression/hara-kiri-percent-repeat.ly (right):

http://codereview.appspot.com/165096/diff/1/3#newcode1
input/regression/hara-kiri-percent-repeat.ly:1: \version "2.13.8"
2.13.9

http://codereview.appspot.com/165096/diff/1/3#newcode4
input/regression/hara-kiri-percent-repeat.ly:4: texidoc = "Staves,
RhythmicStaves, TabStaves and DrumStaves with percent repeats are not
suppressed."
line too long

http://codereview.appspot.com/165096/diff/1/3#newcode10
input/regression/hara-kiri-percent-repeat.ly:10: \new TabStaff \repeat  percent
4 {c1}
\repeat percent 4 { c1 }

http://codereview.appspot.com/165096/diff/1/3#newcode11
input/regression/hara-kiri-percent-repeat.ly:11: \new DrumStaff \drummode {
\repeat percent 4 {hh1} }
{ hh1 }

http://codereview.appspot.com/165096/diff/1/3#newcode16
input/regression/hara-kiri-percent-repeat.ly:16: \context {
\RemoveEmptyStaffContext }
indent two spaces only:

\layout {
  \context { \RemoveEmptyStaffContext }

http://codereview.appspot.com/165096/diff/1/4
File input/regression/hara-kiri-rhythmicstaves.ly (right):

http://codereview.appspot.com/165096/diff/1/4#newcode1
input/regression/hara-kiri-rhythmicstaves.ly:1: \version "2.13.5"
2.13.9

http://codereview.appspot.com/165096/diff/1/4#newcode2
input/regression/hara-kiri-rhythmicstaves.ly:2: \header { texidoc =
same formatting nitpicks as hara-kiri-percent-repeat.ly

http://codereview.appspot.com/165096/diff/1/4#newcode14
input/regression/hara-kiri-rhythmicstaves.ly:14: 
trailing whitespace

http://codereview.appspot.com/165096/diff/1/5
File input/regression/hara-kiri-tabs.ly (right):

http://codereview.appspot.com/165096/diff/1/5#newcode1
input/regression/hara-kiri-tabs.ly:1: \version "2.13.5"
2.13.9

http://codereview.appspot.com/165096/diff/1/5#newcode3
input/regression/hara-kiri-tabs.ly:3: \header { texidoc =
same formatting nitpicks as hara-kiri-percent-repeat.ly

http://codereview.appspot.com/165096/diff/1/5#newcode15
input/regression/hara-kiri-tabs.ly:15: This example was done with a pianostaff,
which has fixed distance
This can be removed, since it's not true (and hasn't been for a long time)

http://codereview.appspot.com/165096/diff/1/5#newcode18
input/regression/hara-kiri-tabs.ly:18: 
trailing whitespace

http://codereview.appspot.com/165096/diff/1/6
File ly/engraver-init.ly (left):

http://codereview.appspot.com/165096/diff/1/6#oldcode1013
ly/engraver-init.ly:1013: RemoveEmptyRhythmicStaffContext= \context {
RemoveEmptyRhythmicStaffContext = \context {

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

http://codereview.appspot.com/165096/diff/1/6#newcode1012
ly/engraver-init.ly:1012: % Add RemoveEmpty*StaffContext function definitions
here
Remove this.

http://codereview.appspot.com/165096/diff/1/6#newcode1014
ly/engraver-init.ly:1014: RemoveEmptyDrumStaffContext= \context {
RemoveEmptyDrumStaffContext = \context {

http://codereview.appspot.com/165096/diff/1/6#newcode1028
ly/engraver-init.ly:1028: RemoveEmptyTabStaffContext= \context {
RemoveEmptyTabStaffContext = \context {

[Ian Hulin, 2009-12-11 14:52:57]

Hi Neil,

n.puttock@gmail.com wrote:
> Hi Ian,
>
> LGTM, apart from some formatting issues and a few incorrect \version
> numbers.
>
> Can you sort out the naming of the new regression tests?  For
> consistency with the existing test, I'd advise amending them as follows:
>
> hara-kiri-drumstaff.ly
> hara-kiri-rhythmicstaff.ly
> hara-kiri-tabstaff.ly
>
> Cheers,
> Neil
>
>
Just info for fellow Frogs. 

You can do this by using (for example)
$ cd <your git repo directory>
$ git mv input/regression/hara-kiri-drums.ly 
input/regression/hara-kiri-drumstaff.ly
$ git mv input/regression/hara-kiri-tabs.ly 
input/regression/hara-kiri-tabstaff.ly
$ git mv input/regression/hara-kiri-rhythmicstaves.ly 
input/regression/hara-kiri-rhythmicstaff.ly

Using git mv updates your file system and the git info at the same time.

You can use
$ git mv -n <source> <dest>
 to test what you are about to do before actually doing the rename.

I'll do the other changes once I've renamed files and put up an amended 
patch set on Rietveld.

Cheers,

Ian
>
>
>
>
> http://codereview.appspot.com/165096/diff/1/2
> File input/regression/hara-kiri-drums.ly (right):
>
> http://codereview.appspot.com/165096/diff/1/2#newcode1
> input/regression/hara-kiri-drums.ly:1: \version "2.13.8"
> 2.13.9
>
> http://codereview.appspot.com/165096/diff/1/2#newcode2
> input/regression/hara-kiri-drums.ly:2: \header { texidoc =
> I know you've just copied the existing example, but it is a bit messy;
> it would be preferable to tidy things up here:
>
> \header {
>   texidoc = "Hara-kiri ...
>
> http://codereview.appspot.com/165096/diff/1/2#newcode14
> input/regression/hara-kiri-drums.ly:14:
> trailing whitespace
>
> http://codereview.appspot.com/165096/diff/1/2#newcode18
> input/regression/hara-kiri-drums.ly:18: ragged-right= ##t
> ragged-right =
>
> http://codereview.appspot.com/165096/diff/1/2#newcode26
> input/regression/hara-kiri-drums.ly:26: \new DrumStaff \drummode {  sn4
> sn sn sn \break s1 \break sn4 sn sn sn \break sn sn sn sn}
> \drummode {
>   sn4 sn sn sn \break
>   s1 break
>   sn4 sn sn sn \break
>   sn4 sn sn sn
> }
>
> etc.
>
> http://codereview.appspot.com/165096/diff/1/3
> File input/regression/hara-kiri-percent-repeat.ly (left):
>
> http://codereview.appspot.com/165096/diff/1/3#oldcode8
> input/regression/hara-kiri-percent-repeat.ly:8: \new Staff { c''1 c''
> \break c'' c'' }
> <<
>   \new Staff
>
> etc.
>
> http://codereview.appspot.com/165096/diff/1/3
> File input/regression/hara-kiri-percent-repeat.ly (right):
>
> http://codereview.appspot.com/165096/diff/1/3#newcode1
> input/regression/hara-kiri-percent-repeat.ly:1: \version "2.13.8"
> 2.13.9
>
> http://codereview.appspot.com/165096/diff/1/3#newcode4
> input/regression/hara-kiri-percent-repeat.ly:4: texidoc = "Staves,
> RhythmicStaves, TabStaves and DrumStaves with percent repeats are not
> suppressed."
> line too long
>
> http://codereview.appspot.com/165096/diff/1/3#newcode10
> input/regression/hara-kiri-percent-repeat.ly:10: \new TabStaff \repeat
> percent 4 {c1}
> \repeat percent 4 { c1 }
>
> http://codereview.appspot.com/165096/diff/1/3#newcode11
> input/regression/hara-kiri-percent-repeat.ly:11: \new DrumStaff
> \drummode { \repeat percent 4 {hh1} }
> { hh1 }
>
> http://codereview.appspot.com/165096/diff/1/3#newcode16
> input/regression/hara-kiri-percent-repeat.ly:16: \context {
> \RemoveEmptyStaffContext }
> indent two spaces only:
>
> \layout {
>   \context { \RemoveEmptyStaffContext }
>
> http://codereview.appspot.com/165096/diff/1/4
> File input/regression/hara-kiri-rhythmicstaves.ly (right):
>
> http://codereview.appspot.com/165096/diff/1/4#newcode1
> input/regression/hara-kiri-rhythmicstaves.ly:1: \version "2.13.5"
> 2.13.9
>
> http://codereview.appspot.com/165096/diff/1/4#newcode2
> input/regression/hara-kiri-rhythmicstaves.ly:2: \header { texidoc =
> same formatting nitpicks as hara-kiri-percent-repeat.ly
>
> http://codereview.appspot.com/165096/diff/1/4#newcode14
> input/regression/hara-kiri-rhythmicstaves.ly:14:
> trailing whitespace
>
> http://codereview.appspot.com/165096/diff/1/5
> File input/regression/hara-kiri-tabs.ly (right):
>
> http://codereview.appspot.com/165096/diff/1/5#newcode1
> input/regression/hara-kiri-tabs.ly:1: \version "2.13.5"
> 2.13.9
>
> http://codereview.appspot.com/165096/diff/1/5#newcode3
> input/regression/hara-kiri-tabs.ly:3: \header { texidoc =
> same formatting nitpicks as hara-kiri-percent-repeat.ly
>
> http://codereview.appspot.com/165096/diff/1/5#newcode15
> input/regression/hara-kiri-tabs.ly:15: This example was done with a
> pianostaff, which has fixed distance
> This can be removed, since it's not true (and hasn't been for a long
> time)
>
> http://codereview.appspot.com/165096/diff/1/5#newcode18
> input/regression/hara-kiri-tabs.ly:18:
> trailing whitespace
>
> http://codereview.appspot.com/165096/diff/1/6
> File ly/engraver-init.ly (left):
>
> http://codereview.appspot.com/165096/diff/1/6#oldcode1013
> ly/engraver-init.ly:1013: RemoveEmptyRhythmicStaffContext= \context {
> RemoveEmptyRhythmicStaffContext = \context {
>
> http://codereview.appspot.com/165096/diff/1/6
> File ly/engraver-init.ly (right):
>
> http://codereview.appspot.com/165096/diff/1/6#newcode1012
> ly/engraver-init.ly:1012: % Add RemoveEmpty*StaffContext function
> definitions here
> Remove this.
>
> http://codereview.appspot.com/165096/diff/1/6#newcode1014
> ly/engraver-init.ly:1014: RemoveEmptyDrumStaffContext= \context {
> RemoveEmptyDrumStaffContext = \context {
>
> http://codereview.appspot.com/165096/diff/1/6#newcode1028
> ly/engraver-init.ly:1028: RemoveEmptyTabStaffContext= \context {
> RemoveEmptyTabStaffContext = \context {
>
> http://codereview.appspot.com/165096
>
> ______________________________________________       This email has 
> been scanned by Netintelligence       
> http://www.netintelligence.com/email
>
>

[Ian Hulin, 2009-12-12 00:12:16]

Hi,
I've implemented Neil's comments, re-run regression tests locally and uploaded
amended patches to Rietveld.

I think this should be ready to push now.

Cheers,

Ian

http://codereview.appspot.com/165096/diff/1/2
File input/regression/hara-kiri-drums.ly (right):

http://codereview.appspot.com/165096/diff/1/2#newcode1
input/regression/hara-kiri-drums.ly:1: \version "2.13.8"
On 2009/12/10 23:43:35, Neil Puttock wrote:
> 2.13.9

Done.

http://codereview.appspot.com/165096/diff/1/2#newcode14
input/regression/hara-kiri-drums.ly:14: 
On 2009/12/10 23:43:35, Neil Puttock wrote:
> trailing whitespace

Done.

http://codereview.appspot.com/165096/diff/1/2#newcode18
input/regression/hara-kiri-drums.ly:18: ragged-right= ##t
On 2009/12/10 23:43:35, Neil Puttock wrote:
> ragged-right =

Done.

http://codereview.appspot.com/165096/diff/1/2#newcode26
input/regression/hara-kiri-drums.ly:26: \new DrumStaff \drummode {  sn4 sn sn sn
\break s1 \break sn4 sn sn sn \break sn sn sn sn}
On 2009/12/10 23:43:35, Neil Puttock wrote:
> \drummode {
>   sn4 sn sn sn \break
>   s1 break
>   sn4 sn sn sn \break
>   sn4 sn sn sn
> }
> 
> etc.
> 

Done.

http://codereview.appspot.com/165096/diff/1/3
File input/regression/hara-kiri-percent-repeat.ly (left):

http://codereview.appspot.com/165096/diff/1/3#oldcode8
input/regression/hara-kiri-percent-repeat.ly:8: \new Staff { c''1 c'' \break c''
c'' }
On 2009/12/10 23:43:35, Neil Puttock wrote:
> <<
>   \new Staff
> 
> etc.

Done.

http://codereview.appspot.com/165096/diff/1/3
File input/regression/hara-kiri-percent-repeat.ly (right):

http://codereview.appspot.com/165096/diff/1/3#newcode4
input/regression/hara-kiri-percent-repeat.ly:4: texidoc = "Staves,
RhythmicStaves, TabStaves and DrumStaves with percent repeats are not
suppressed."
On 2009/12/10 23:43:35, Neil Puttock wrote:
> line too long
Doc comment split into two shorter lines.
Done.

http://codereview.appspot.com/165096/diff/1/3#newcode11
input/regression/hara-kiri-percent-repeat.ly:11: \new DrumStaff \drummode {
\repeat percent 4 {hh1} }
On 2009/12/10 23:43:35, Neil Puttock wrote:
> { hh1 }

Done.

http://codereview.appspot.com/165096/diff/1/3#newcode16
input/regression/hara-kiri-percent-repeat.ly:16: \context {
\RemoveEmptyStaffContext }
On 2009/12/10 23:43:35, Neil Puttock wrote:
> indent two spaces only:
> 
> \layout {
>   \context { \RemoveEmptyStaffContext }
> 

Done.

http://codereview.appspot.com/165096/diff/1/4
File input/regression/hara-kiri-rhythmicstaves.ly (right):

http://codereview.appspot.com/165096/diff/1/4#newcode2
input/regression/hara-kiri-rhythmicstaves.ly:2: \header { texidoc =
On 2009/12/10 23:43:35, Neil Puttock wrote:
> same formatting nitpicks as hara-kiri-percent-repeat
Done.

http://codereview.appspot.com/165096/diff/1/4#newcode4
input/regression/hara-kiri-rhythmicstaves.ly:4: " Hara-kiri staves kill
themselves if they are empty.  This
"kill themselves" > "are suppressed" (fewer characters, better style)

http://codereview.appspot.com/165096/diff/1/4#newcode5
input/regression/hara-kiri-rhythmicstaves.ly:5: example really contains three
rhythmic staves, but as they progress, empty ones
"they progress" > "it progresses" (fewer characters, clearer meaning)

http://codereview.appspot.com/165096/diff/1/4#newcode14
input/regression/hara-kiri-rhythmicstaves.ly:14: 
On 2009/12/10 23:43:35, Neil Puttock wrote:
> trailing whitespace

Done.

http://codereview.appspot.com/165096/diff/1/5
File input/regression/hara-kiri-tabs.ly (right):

http://codereview.appspot.com/165096/diff/1/5#newcode1
input/regression/hara-kiri-tabs.ly:1: \version "2.13.5"
On 2009/12/10 23:43:35, Neil Puttock wrote:
> 2.13.9

Done.

http://codereview.appspot.com/165096/diff/1/5#newcode1
input/regression/hara-kiri-tabs.ly:1: \version "2.13.5"
File now renamed hara-kiri-tabstaff.ly

http://codereview.appspot.com/165096/diff/1/5#newcode3
input/regression/hara-kiri-tabs.ly:3: \header { texidoc =
On 2009/12/10 23:43:35, Neil Puttock wrote:
> same formatting nitpicks as hara-kiri-percent-repeat.ly

Done.

http://codereview.appspot.com/165096/diff/1/5#newcode5
input/regression/hara-kiri-tabs.ly:5: " Hara-kiri staves kill themselves if they
are empty.  This
"kill themselves" > "are suppressed"

http://codereview.appspot.com/165096/diff/1/5#newcode6
input/regression/hara-kiri-tabs.ly:6: example really contains three tab staves,
but as they progress, empty ones
"they progress" > "it progresses"

http://codereview.appspot.com/165096/diff/1/5#newcode15
input/regression/hara-kiri-tabs.ly:15: This example was done with a pianostaff,
which has fixed distance
On 2009/12/10 23:43:35, Neil Puttock wrote:
> This can be removed, since it's not true (and hasn't been for a long time)

Done.

http://codereview.appspot.com/165096/diff/1/5#newcode18
input/regression/hara-kiri-tabs.ly:18: 
On 2009/12/10 23:43:35, Neil Puttock wrote:
> trailing whitespace

Done.

http://codereview.appspot.com/165096/diff/1/6
File ly/engraver-init.ly (left):

http://codereview.appspot.com/165096/diff/1/6#oldcode1013
ly/engraver-init.ly:1013: RemoveEmptyRhythmicStaffContext= \context {
On 2009/12/10 23:43:35, Neil Puttock wrote:
> RemoveEmptyRhythmicStaffContext = \context {

Done.

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

http://codereview.appspot.com/165096/diff/1/6#newcode1012
ly/engraver-init.ly:1012: % Add RemoveEmpty*StaffContext function definitions
here
On 2009/12/10 23:43:35, Neil Puttock wrote:
> Remove this.
Old habits.  I worked on a project for a long time where we were required to put
a comment in code to link the bug-tracking system to a particular change in a
source file.

Done.

http://codereview.appspot.com/165096/diff/1/6#newcode1014
ly/engraver-init.ly:1014: RemoveEmptyDrumStaffContext= \context {
On 2009/12/10 23:43:35, Neil Puttock wrote:
> RemoveEmptyDrumStaffContext = \context {

Done.

http://codereview.appspot.com/165096/diff/1/6#newcode1028
ly/engraver-init.ly:1028: RemoveEmptyTabStaffContext= \context {
On 2009/12/10 23:43:35, Neil Puttock wrote:
> RemoveEmptyTabStaffContext = \context {

Done.

[Neil Puttock, 2009-12-13 17:00:39]

On 2009/12/12 00:12:16, Ian Hulin wrote:

> I've implemented Neil's comments, re-run regression tests locally and uploaded
> amended patches to Rietveld.

Thanks!

> I think this should be ready to push now.

Nearly there:

You're playing catch-up with the \version strings. :)

Both hara-kiri-rhythmicstaff.ly and hara-kiri-tabstaff.ly still need
reformatting in the same way as hara-kiri-drumstaff.ly

Cheers,
Neil

[Ian Hulin, 2009-12-14 17:15:13]

Version numbers sorted, also formatting of music in hara-kiri-drumstaff and
hara-kiri-tabstaff.

Regression tests re-run locally.

Cheers,
Ian

http://codereview.appspot.com/165096/diff/3015/1022
File ly/engraver-init.ly (left):

http://codereview.appspot.com/165096/diff/3015/1022#oldcode144
ly/engraver-init.ly:144: \accepts "FiguredBass"
This line not removed by this patch:  appeared after git rebase.

http://codereview.appspot.com/165096/diff/3015/1022#oldcode378
ly/engraver-init.ly:378: \consists "Text_spanner_engraver"
This line also not removed by this patch.

http://codereview.appspot.com/165096/diff/1023/3021#oldcode144
ly/engraver-init.ly:144: \accepts "FiguredBass"
line removed during git merge/rebase, not explicitly by this patch.

http://codereview.appspot.com/165096/diff/1023/3021#oldcode378
ly/engraver-init.ly:378: \consists "Text_spanner_engraver"
line removed during git merge/rebase, not explicitly by this patch.

[Neil Puttock, 2009-12-20 21:15:53]

On 2009/12/14 17:15:13, Ian Hulin wrote:
> Version numbers sorted, also formatting of music in hara-kiri-drumstaff and
> hara-kiri-tabstaff.
> 
> Regression tests re-run locally.

LGTM.

You just need to rebase against master to get rid of the reverts in
engraver-init.ly, then it'll be ready for pushing.

Cheers,
Neil