Fix 1456

Fix 1456

Eliminate the context argument from \overrideTimeSignatureSettings
and \revertTimeSignatureSettings, since the settings that apply must
be those from the Timing context.

Update documentation to go along with this change.

Run convert-ly on the docs and snippets.

[Carl, 2010-12-16 23:10:35]

Here's a fix for issue 1456.

I've taken the context argument out of \overrideTimeSignatureSettings and
\revertTimeSignatureSettings.  They now apply to the Timing context.

I've also added an example showing how to have different time signature settings
in different staves.

Please review.

Thanks,

Carl

[Graham Percival (old account), 2010-12-17 13:53:06]

Looks fine from a quick eyeball, but I haven't tested it.


We've had a few people bragging about their computers lately, so could one of
them try applying this patch and make sure that everything compiles from
scratch?  Using the lilybuntu instructions, "from scratch" means removing the
build/ dir and then starting the "preparing the build" instructions again.

this particular patch should be applied with
  patch -p0 < blah.diff
when you're in the top source directory.

[Graham Percival (old account), 2010-12-17 13:53:37]

oops, I hit "reply to comment" intead of "publish+send".  Here's my only comment
on the patch.

http://codereview.appspot.com/3693042/diff/1/input/regression/skiptypesetting...
File input/regression/skiptypesetting-multimeasurerest.ly (right):

http://codereview.appspot.com/3693042/diff/1/input/regression/skiptypesetting...
input/regression/skiptypesetting-multimeasurerest.ly:10: \version "2.13.43"
I guess this should be .44 ?  or .36 ?  I mean, nothing's changed in this
file...?

[mail_philholmes.net, 2010-12-17 16:57:42]

----- Original Message ----- 
From: <percival.music.ca@gmail.com>
To: <Carl.D.Sorensen@gmail.com>
Cc: <reply@codereview.appspotmail.com>; <lilypond-devel@gnu.org>
Sent: Friday, December 17, 2010 1:53 PM
Subject: Re: Fix 1456 (issue3693042)


> Looks fine from a quick eyeball, but I haven't tested it.
>
>
> We've had a few people bragging about their computers lately, so could
> one of them try applying this patch and make sure that everything
> compiles from scratch?  Using the lilybuntu instructions, "from scratch"
> means removing the build/ dir and then starting the "preparing the
> build" instructions again.
>
> this particular patch should be applied with
>   patch -p0 < blah.diff
> when you're in the top source directory.
>
> http://codereview.appspot.com/3693042/
>
>

Currently testing the LB2 instructions in a new VM, but could do this with 
the old VM.  Could you be a little more explicit about how to apply the 
patch?

--
Phil Holmes

[mikesol_ufl.edu, 2010-12-17 20:49:18]

Tested and nothing broke. Let me know if you want me to send any output.

Cheers,
M "I withhold the right to brag about how fast my new computer is until it
becomes obsolete in 2 weeks" S



On Dec 17, 2010, at 11:57 AM, "Phil Holmes" <mail@philholmes.net> wrote:

> ----- Original Message ----- From: <percival.music.ca@gmail.com>
> To: <Carl.D.Sorensen@gmail.com>
> Cc: <reply@codereview.appspotmail.com>; <lilypond-devel@gnu.org>
> Sent: Friday, December 17, 2010 1:53 PM
> Subject: Re: Fix 1456 (issue3693042)
> 
> 
>> Looks fine from a quick eyeball, but I haven't tested it.
>> 
>> 
>> We've had a few people bragging about their computers lately, so could
>> one of them try applying this patch and make sure that everything
>> compiles from scratch?  Using the lilybuntu instructions, "from scratch"
>> means removing the build/ dir and then starting the "preparing the
>> build" instructions again.
>> 
>> this particular patch should be applied with
>>  patch -p0 < blah.diff
>> when you're in the top source directory.
>> 
>> http://codereview.appspot.com/3693042/
>> 
>> 
> 
> Currently testing the LB2 instructions in a new VM, but could do this with the
old VM.  Could you be a little more explicit about how to apply the patch?
> 
> --
> Phil Holmes
> 
> 
> 
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/lilypond-devel

[Neil Puttock, 2010-12-17 22:37:36]

http://codereview.appspot.com/3693042/diff/1/Documentation/notation/rhythms.i...
File Documentation/notation/rhythms.itely (left):

http://codereview.appspot.com/3693042/diff/1/Documentation/notation/rhythms.i...
Documentation/notation/rhythms.itely:1059: \relative c' {
\new Staff

http://codereview.appspot.com/3693042/diff/1/Documentation/notation/rhythms.i...
Documentation/notation/rhythms.itely:1106: \relative c' {
doesn't need \new Staff, but might add for consistency with preceding example

http://codereview.appspot.com/3693042/diff/1/input/regression/multiple-time-s...
File input/regression/multiple-time-sig-settings.ly (left):

http://codereview.appspot.com/3693042/diff/1/input/regression/multiple-time-s...
input/regression/multiple-time-sig-settings.ly:11: \relative c' {
needs \new Staff

http://codereview.appspot.com/3693042/diff/1/input/regression/skiptypesetting...
File input/regression/skiptypesetting-multimeasurerest.ly (right):

http://codereview.appspot.com/3693042/diff/1/input/regression/skiptypesetting...
input/regression/skiptypesetting-multimeasurerest.ly:10: \version "2.13.43"
On 2010/12/17 13:53:37, Graham Percival wrote:
> I guess this should be .44 ?  or .36 ?  I mean, nothing's changed in this
> file...?

It should be removed.  There's already a version string at the top of the file.

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

http://codereview.appspot.com/3693042/diff/1/ly/music-functions-init.ly#newco...
ly/music-functions-init.ly:747: (revert-time-signature-setting time-signature))
indent

http://codereview.appspot.com/3693042/diff/1/scm/time-signature-settings.scm
File scm/time-signature-settings.scm (left):

http://codereview.appspot.com/3693042/diff/1/scm/time-signature-settings.scm#...
scm/time-signature-settings.scm:294: (if (and (pair? rest) (symbol? (car rest)))
remove (+ `rest' arg)

http://codereview.appspot.com/3693042/diff/1/scm/time-signature-settings.scm#...
scm/time-signature-settings.scm:304: time-signature . rest)
remove `rest' arg

http://codereview.appspot.com/3693042/diff/1/scm/time-signature-settings.scm#...
scm/time-signature-settings.scm:312: (if (and (pair? rest) (symbol? (car rest)))
remove

[Carl, 2010-12-18 00:27:46]

Thanks for the excellent feedback.  I've fixed everything now.

A revised patch set is available.

http://codereview.appspot.com/3693042/diff/1/Documentation/notation/rhythms.i...
File Documentation/notation/rhythms.itely (left):

http://codereview.appspot.com/3693042/diff/1/Documentation/notation/rhythms.i...
Documentation/notation/rhythms.itely:1059: \relative c' {
On 2010/12/17 22:37:36, Neil Puttock wrote:
> \new Staff

Done.

http://codereview.appspot.com/3693042/diff/1/Documentation/notation/rhythms.i...
Documentation/notation/rhythms.itely:1106: \relative c' {
On 2010/12/17 22:37:36, Neil Puttock wrote:
> doesn't need \new Staff, but might add for consistency with preceding example


I've added an explanation about when contexts need to be explicitly
instantiated.  Thanks for helping to learn this.

http://codereview.appspot.com/3693042/diff/1/input/regression/multiple-time-s...
File input/regression/multiple-time-sig-settings.ly (left):

http://codereview.appspot.com/3693042/diff/1/input/regression/multiple-time-s...
input/regression/multiple-time-sig-settings.ly:11: \relative c' {
On 2010/12/17 22:37:36, Neil Puttock wrote:
> needs \new Staff

Done.

http://codereview.appspot.com/3693042/diff/1/input/regression/skiptypesetting...
File input/regression/skiptypesetting-multimeasurerest.ly (right):

http://codereview.appspot.com/3693042/diff/1/input/regression/skiptypesetting...
input/regression/skiptypesetting-multimeasurerest.ly:10: \version "2.13.43"
On 2010/12/17 22:37:36, Neil Puttock wrote:
> On 2010/12/17 13:53:37, Graham Percival wrote:
> > I guess this should be .44 ?  or .36 ?  I mean, nothing's changed in this
> > file...?
> 
> It should be removed.  There's already a version string at the top of the
file.

Done.

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

http://codereview.appspot.com/3693042/diff/1/ly/music-functions-init.ly#newco...
ly/music-functions-init.ly:747: (revert-time-signature-setting time-signature))
On 2010/12/17 22:37:36, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/3693042/diff/1/scm/time-signature-settings.scm
File scm/time-signature-settings.scm (left):

http://codereview.appspot.com/3693042/diff/1/scm/time-signature-settings.scm#...
scm/time-signature-settings.scm:294: (if (and (pair? rest) (symbol? (car rest)))
On 2010/12/17 22:37:36, Neil Puttock wrote:
> remove (+ `rest' arg)

Yeah, I was trying to decide if I should leave it in.  I made the wrong
decision,
thinking "we might use it sometime...."

Done

http://codereview.appspot.com/3693042/diff/1/scm/time-signature-settings.scm#...
scm/time-signature-settings.scm:304: time-signature . rest)
On 2010/12/17 22:37:36, Neil Puttock wrote:
> remove `rest' arg

Done.

http://codereview.appspot.com/3693042/diff/1/scm/time-signature-settings.scm#...
scm/time-signature-settings.scm:312: (if (and (pair? rest) (symbol? (car rest)))
On 2010/12/17 22:37:36, Neil Puttock wrote:
> remove

Done.

[Neil Puttock, 2010-12-19 16:54:17]

LGTM.

[pkx166h, 2010-12-20 01:20:30]

Just some Nitpicks

http://codereview.appspot.com/3693042/diff/9001/Documentation/notation/rhythm...
File Documentation/notation/rhythms.itely (right):

http://codereview.appspot.com/3693042/diff/9001/Documentation/notation/rhythm...
Documentation/notation/rhythms.itely:1111: c8^\markup "Beamed (2 2)" \repeat
unfold 7 { c8 } |
c8^ \markup { "Beamed (2 2)" } as per CG also I'd personally put the \repeat on
a new line.

http://codereview.appspot.com/3693042/diff/9001/Documentation/notation/rhythm...
Documentation/notation/rhythms.itely:1119: c8^\markup "Beamed (3 1)" \repeat
unfold 7 { c8 } |
Same as above.

http://codereview.appspot.com/3693042/diff/9001/Documentation/notation/rhythm...
Documentation/notation/rhythms.itely:1172: #'()                            %
beamExceptions
The spacing here with the comments after each #'() construct is inconsistent
with the example above.

http://codereview.appspot.com/3693042/diff/9001/Documentation/notation/rhythm...
Documentation/notation/rhythms.itely:1175: }
This brace needs one more space by the looks of it

[Carl, 2010-12-20 05:49:39]

Thanks for the review, Patrick.

I've made the changes and posted a new patch set.

http://codereview.appspot.com/3693042/diff/9001/Documentation/notation/rhythm...
File Documentation/notation/rhythms.itely (right):

http://codereview.appspot.com/3693042/diff/9001/Documentation/notation/rhythm...
Documentation/notation/rhythms.itely:1111: c8^\markup "Beamed (2 2)" \repeat
unfold 7 { c8 } |
On 2010/12/20 01:20:30, pkx166h wrote:
> c8^ \markup { "Beamed (2 2)" } as per CG also I'd personally put the \repeat
on
> a new line.

Done.

http://codereview.appspot.com/3693042/diff/9001/Documentation/notation/rhythm...
Documentation/notation/rhythms.itely:1119: c8^\markup "Beamed (3 1)" \repeat
unfold 7 { c8 } |
On 2010/12/20 01:20:30, pkx166h wrote:
> Same as above.

Done.

http://codereview.appspot.com/3693042/diff/9001/Documentation/notation/rhythm...
Documentation/notation/rhythms.itely:1172: #'()                            %
beamExceptions
On 2010/12/20 01:20:30, pkx166h wrote:
> The spacing here with the comments after each #'() construct is inconsistent
> with the example above.

Actually, I think the spacing is consistent.  They are farther to the right
because the indentation level is higher for those lines, due to the addition of
the StaffGroup and the Staff.

But I guess they don't need to be that far to the right.  They were that far to
the right because they originally had an unnecessary beamExceptions rule.  I'll
squeeze them all in.  It will look better.

http://codereview.appspot.com/3693042/diff/9001/Documentation/notation/rhythm...
Documentation/notation/rhythms.itely:1175: }
On 2010/12/20 01:20:30, pkx166h wrote:
> This brace needs one more space by the looks of it

Nice catch!

Done

[Graham Percival (old account), 2010-12-20 18:39:19]

LGTM