Re: Fix critical regressions to lyric spacing (issue4061043)

[Carl . D . Sorensen]

I've responded to the comments in detail below.

Graham, relative to your comments on issues 1483 and 1486, both are
addressed in this patch.

The templates now have the lyrics attached properly to the staves, as do
the examples in the NR.  This takes care of 1483.

The templates also have sufficient space for the lyrics at the top and
bottom of the score.  This takes care of 1486.

For good measure, there is a snippet in the docs that demonstrates the
way to get the same spacing behavior as in 2.12.

As far as I can see, all of the problems associated with both issues are
resolved by this patch.



http://codereview.appspot.com/4061043/diff/1/Documentation/changes.tely
File Documentation/changes.tely (right):

http://codereview.appspot.com/4061043/diff/1/Documentation/changes.tely#newcode70
Documentation/changes.tely:70: Lyrics above a staff must have their
@code{staff-affinity} set to
I think it is appropriate, because people who used the old vocal
templates will suddenly find that the soprano lyrics will be attached to
the previous system, and the tenor lyrics will be attached to the
womens' staff.  One rational place for them to go to investigate is to
look at CHANGES.

What if, instead of a snippet, it referenced the appropriate section in
the Notation Reference?

I've gone ahead and added the reference; I'll change if you think it
necessary.

http://codereview.appspot.com/4061043/diff/1/Documentation/notation/vocal.itely
File Documentation/notation/vocal.itely (right):

http://codereview.appspot.com/4061043/diff/1/Documentation/notation/vocal.itely#newcode1021
Documentation/notation/vocal.itely:1021: % lyrics above a stave should
have this override
Changed stave to staff

http://codereview.appspot.com/4061043/diff/1/Documentation/snippets/new/lyrics-old-spacing-settings.ly
File Documentation/snippets/new/lyrics-old-spacing-settings.ly (right):

http://codereview.appspot.com/4061043/diff/1/Documentation/snippets/new/lyrics-old-spacing-settings.ly#newcode19
Documentation/snippets/new/lyrics-old-spacing-settings.ly:19: % VERSE
ONE
On 2011/01/23 14:20:35, Graham Percival wrote:
> This differs from our normal indentation style... and in particular,
it
> conflicts with the two-space indentation style that you used above in
the global
> section... but meh, go ahead.


No, I'll fix it.  This is what happens when I copy somebody else's code
and don't review it carefully.  I made sure it worked, and that we had
the right header stuff, but didn't pay a lot of attention to the coding
style.

Thanks for the catch.

I've fixed the indentation, along with removing the unnecessary { s1 }
from each \new Lyrics construct.

http://codereview.appspot.com/4061043/diff/1/Documentation/snippets/new/vocal-ensemble-template-with-automatic-piano-reduction.ly
File
Documentation/snippets/new/vocal-ensemble-template-with-automatic-piano-reduction.ly
(right):

http://codereview.appspot.com/4061043/diff/1/Documentation/snippets/new/vocal-ensemble-template-with-automatic-piano-reduction.ly#newcode6
Documentation/snippets/new/vocal-ensemble-template-with-automatic-piano-reduction.ly:6:
%% Translation of GIT committish:
fa19277d20f8ab0397c560eb0e7b814bd804ecec
On 2011/01/23 22:55:54, Neil Puttock wrote:
> remove all translation stuff

Done.

http://codereview.appspot.com/4061043/diff/1/Documentation/snippets/new/vocal-ensemble-template.ly
File Documentation/snippets/new/vocal-ensemble-template.ly (right):

http://codereview.appspot.com/4061043/diff/1/Documentation/snippets/new/vocal-ensemble-template.ly#newcode6
Documentation/snippets/new/vocal-ensemble-template.ly:6: %% Translation
of GIT committish: fa19277d20f8ab0397c560eb0e7b814bd804ecec
On 2011/01/23 22:55:54, Neil Puttock wrote:
> remove

Done.

http://codereview.appspot.com/4061043/diff/1/input/regression/baerenreiter-sarabande.ly
File input/regression/baerenreiter-sarabande.ly (right):

http://codereview.appspot.com/4061043/diff/1/input/regression/baerenreiter-sarabande.ly#newcode174
input/regression/baerenreiter-sarabande.ly:174:
obsolete-between-system-space = 25\mm  system-system-spacing
#'basic-distance = #(/ obsolete-between-system-space staff-space)
score-system-spacing #'basic-distance = #(/
obsolete-between-system-space staff-space)
On 2011/01/23 14:20:35, Graham Percival wrote:
> woah, what happened here?  Could we get some linebreaks? (is this some
weird
> osx-linebreaks+git malfunction?)

No, this is exactly the way convert-ly was intended to work for this
change, because in some cases the lines were commented.

This was discussed before the convert-ly rule was pushed.

The convert-ly rule was pushed in commit
4921d3518f4961abcfaf9ea243bec33efc943574  IIUC.  The author was Keith
O'Hara and the committer was Graham Percival ;-)

But I will do a manual fix and make it the way it would be if we were
writing it from scratch.

Done.

http://codereview.appspot.com/4061043/diff/1/input/regression/mozart-hrn-3.ly
File input/regression/mozart-hrn-3.ly (right):

http://codereview.appspot.com/4061043/diff/1/input/regression/mozart-hrn-3.ly#newcode51
input/regression/mozart-hrn-3.ly:51: obsolete-between-system-space = 20
\mm  system-system-spacing #'basic-distance = #(/
obsolete-between-system-space staff-space)  score-system-spacing
#'basic-distance = #(/ obsolete-between-system-space staff-space)
On 2011/01/23 14:20:35, Graham Percival wrote:
> ditto.  hmm, maybe Keith's recent convert-ly patch has a problem?

Done.

http://codereview.appspot.com/4061043/diff/1/input/regression/page-spacing.ly
File input/regression/page-spacing.ly (right):

http://codereview.appspot.com/4061043/diff/1/input/regression/page-spacing.ly#newcode71
input/regression/page-spacing.ly:71: obsolete-between-system-space = 1.0
 system-system-spacing #'basic-distance = #(/
obsolete-between-system-space staff-space)  score-system-spacing
#'basic-distance = #(/ obsolete-between-system-space staff-space)
On 2011/01/23 14:20:35, Graham Percival wrote:
> ditto.

Done.

http://codereview.appspot.com/4061043/diff/1/input/regression/page-top-space.ly
File input/regression/page-top-space.ly (right):

http://codereview.appspot.com/4061043/diff/1/input/regression/page-top-space.ly#newcode25
input/regression/page-top-space.ly:25: obsolete-page-top-space = 3 \cm
top-system-spacing #'basic-distance = #(/ obsolete-page-top-space
staff-space)
On 2011/01/23 14:20:35, Graham Percival wrote:
> ditto.

Done.

http://codereview.appspot.com/4061043/