Re: Revised autobeam settings patch (issue1682049)

[Carl . D . Sorensen]

I've responded to each of Neil's comments below.  I'll post another
patch set once I've proven that the regtests and the docs build
properly.

THanks,

Carl



http://codereview.appspot.com/1682049/diff/3035/9004
File Documentation/notation/rhythms.itely (right):

http://codereview.appspot.com/1682049/diff/3035/9004#newcode1052
Documentation/notation/rhythms.itely:1052: In addition to setting the
printed time signature, the @code{time}
On 2010/07/07 00:52:10, Neil Puttock wrote:
> \time

Done.

http://codereview.appspot.com/1682049/diff/3035/9004#newcode1055
Documentation/notation/rhythms.itely:1055: @code{baseUnit},
@code{beatStructure}, and @code{beamExceptions}.  The
On 2010/07/07 00:52:10, Neil Puttock wrote:
> baseMoment

Done.

http://codereview.appspot.com/1682049/diff/3035/9004#newcode1069
Documentation/notation/rhythms.itely:1069:
\overrideTimeSignatureSettings
On 2010/07/07 00:52:10, Neil Puttock wrote:
> Can you think of a better name which doesn't cause confusion with
\override and
> \revert


We had a big discussion about this on -devel, and nobody came up with
anything that was better than the chosen names.  You can see the whole
messy string here:

http://thread.gmane.org/gmane.comp.gnu.lilypond.devel/26649

If you have any better suggestions, I'd welcome them.


> (seeing as this naming convention will have to be changed)?

What does this mean?

http://codereview.appspot.com/1682049/diff/3035/9004#newcode1079
Documentation/notation/rhythms.itely:1079: For convenience, the Scheme
function @code{make-setting} is
On 2010/07/07 00:52:10, Neil Puttock wrote:
> `make-setting' is a bit vague

Yes, it is.  Thanks for the catch.


> why not simply add these settings as extra arguments to
> \overrideTimeSignatureSettings?

I think the reason is that I wanted to separate out the constructor and
accessor functions for the settings.  But I can do that internally to
the function call.  No need to make the user do the extra call.

Oh, as I was editing the changes I remembered why I did it.  It was to
be similar to \override and \revert.

\override 'mygrob #'Property = Setting
\revert  'mygrob #'Property

Similarly

\overrideTimeSignatureSettings 'Voice #'(4 . 4) my-setting
\revertTimeSignatureSettings 'Voice #'(4 . 4)

But I've gone ahead and made the changes so that we can see it both ways
(and it will be easy to revert if desired).

http://codereview.appspot.com/1682049/diff/3035/9004#newcode1817
Documentation/notation/rhythms.itely:1817: of the beam, e.g. @code{(1 .
16)}.
On 2010/07/07 00:52:10, Neil Puttock wrote:
> e.g.,

Done.

http://codereview.appspot.com/1682049/diff/3035/9004#newcode1860
Documentation/notation/rhythms.itely:1860: \new Voice  = two {
On 2010/07/07 00:52:10, Neil Puttock wrote:
> Voice =

Done.

http://codereview.appspot.com/1682049/diff/3035/9004#newcode1895
Documentation/notation/rhythms.itely:1895: % eliminate beamException
that groups beats 1,2 and 3,4
On 2010/07/07 00:52:10, Neil Puttock wrote:
> beam exception

> 1, 2
> 3, 4

Done.

http://codereview.appspot.com/1682049/diff/3035/9005
File Documentation/snippets/new/automatic-beam-subdivisions.ly (right):

http://codereview.appspot.com/1682049/diff/3035/9005#newcode13
Documentation/snippets/new/automatic-beam-subdivisions.ly:13: } % begin
verbatim
On 2010/07/07 00:52:10, Neil Puttock wrote:
> remove

Done.

http://codereview.appspot.com/1682049/diff/3035/9009
File
Documentation/snippets/new/conducting-signs,-measure-grouping-signs.ly
(right):

http://codereview.appspot.com/1682049/diff/3035/9009#newcode6
Documentation/snippets/new/conducting-signs,-measure-grouping-signs.ly:6:
Beat grouping within a bar is controlled by the context property
On 2010/07/07 00:52:10, Neil Puttock wrote:
> within a measure

Done.

http://codereview.appspot.com/1682049/diff/3035/9009#newcode29
Documentation/snippets/new/conducting-signs,-measure-grouping-signs.ly:29:
doctitle = "Conducting signs measure grouping signs"
On 2010/07/07 00:52:10, Neil Puttock wrote:
> rename file too (and references in selected snippets)

This was actually a mistake; I didn't get rid of the comma on purpose.

I'm not a fan of the title of this snippet.  Read calls them "conducting
signs";
Stone calls them "beating signs", and indicates that they are a subset
of "Conductor's signs".  Ross makes no mention that I can see.

Do you really want me to rename the file, or was that just for
consistency?  My current fix is to add the comma back in.

http://codereview.appspot.com/1682049/diff/3035/9012
File Documentation/snippets/new/sub-dividing-beams.ly (right):

http://codereview.appspot.com/1682049/diff/3035/9012#newcode8
Documentation/snippets/new/sub-dividing-beams.ly:8: sub-divided.  That
is, the three (or more) beams stretch unbroken over
On 2010/07/07 00:52:10, Neil Puttock wrote:
> subdivided

> etc.

I wanted to rename this file, so I did
(Documentation/snippets/new/subdividing-beams.ly).  I al

What do I do to get rid of the old name in Documentation/snippets?  Do
I make Documentation/snippets/new/sub-dividing-beams.ly with a markup
that says "THis snippet has been renamed"?

http://codereview.appspot.com/1682049/diff/3035/9012#newcode25
Documentation/snippets/new/sub-dividing-beams.ly:25: } % begin verbatim
On 2010/07/07 00:52:10, Neil Puttock wrote:
> remove

Done.

http://codereview.appspot.com/1682049/diff/3035/9016
File lily/auto-beam-engraver.cc (right):

http://codereview.appspot.com/1682049/diff/3035/9016#newcode545
lily/auto-beam-engraver.cc:545: "beamExceptions "
On 2010/07/07 00:52:10, Neil Puttock wrote:
> mixing tabs and spaces

Done.

http://codereview.appspot.com/1682049/diff/3035/9019
File lily/beaming-pattern.cc (right):

http://codereview.appspot.com/1682049/diff/3035/9019#newcode298
lily/beaming-pattern.cc:298: Moment (1, 4));
On 2010/07/07 00:52:10, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/1682049/diff/3035/9027
File python/convertrules.py (right):

http://codereview.appspot.com/1682049/diff/3035/9027#newcode3008
python/convertrules.py:3008: @rule ((2, 13, 24),
On 2010/07/07 00:52:10, Neil Puttock wrote:
> beamSettings and beatLength need convert rules

Yes, and we need to change the version to which the rules will apply.  I
guess it will be 2.13.28, now that .27 is out and Graham will be gone
for a month.

http://codereview.appspot.com/1682049/diff/3035/9030
File scm/define-context-properties.scm (right):

http://codereview.appspot.com/1682049/diff/3035/9030#newcode133
scm/define-context-properties.scm:133: (beamExceptions ,list? "alist of
exceptions to autobeam rules
On 2010/07/07 00:52:10, Neil Puttock wrote:
> Alist

Changed to "An alist"

http://codereview.appspot.com/1682049/diff/3035/9030#newcode135
scm/define-context-properties.scm:135: (beatStructure ,list? "List of
baseMoments that are combined
On 2010/07/07 00:52:10, Neil Puttock wrote:
> @code{baseMoment}s

Done.

http://codereview.appspot.com/1682049/diff/3035/9030#newcode444
scm/define-context-properties.scm:444: subdivided at baseMoment
positions by only drawing one beam over the beat.")
On 2010/07/07 00:52:10, Neil Puttock wrote:
> @code{baseMoment}


Done.

http://codereview.appspot.com/1682049/diff/3035/9030#newcode472
scm/define-context-properties.scm:472: (timeSignatureSettings ,pair? "A
nested alist of settings for
On 2010/07/07 00:52:10, Neil Puttock wrote:
> ,cheap-list?

Yep, done.

http://codereview.appspot.com/1682049/diff/3035/9032
File scm/lily-library.scm (right):

http://codereview.appspot.com/1682049/diff/3035/9032#newcode70
scm/lily-library.scm:70: (if (null? fraction)
On 2010/07/07 00:52:10, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/1682049/diff/3035/9032#newcode75
scm/lily-library.scm:75: (cons (ly:moment-main-numerator moment)
On 2010/07/07 00:52:10, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/1682049/diff/3035/9034
File scm/music-functions.scm (right):

http://codereview.appspot.com/1682049/diff/3035/9034#newcode536
scm/music-functions.scm:536: (define (make-time-settings context)
On 2010/07/07 00:52:10, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/1682049/diff/3035/9034#newcode546
scm/music-functions.scm:546: (beam-exceptions fraction
time-signature-settings))
On 2010/07/07 00:52:10, Neil Puttock wrote:
> indent (or move back up to previous line)

Done.

http://codereview.appspot.com/1682049/diff/3035/9034#newcode547
scm/music-functions.scm:547: (new-measure-length  (ly:make-moment num
den)))
On 2010/07/07 00:52:10, Neil Puttock wrote:
> (new-measure-length (ly:make-moment num den)))

Done.

http://codereview.appspot.com/1682049/diff/3035/9035
File scm/time-signature-settings.scm (right):

http://codereview.appspot.com/1682049/diff/3035/9035#newcode72
scm/time-signature-settings.scm:72: '(
On 2010/07/07 00:52:10, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/1682049/diff/3035/9035#newcode107
scm/time-signature-settings.scm:107: ((beamExceptions . (((end . (((1 .
16) . (4 4 4 4 4 4 4 4)))))))))
On 2010/07/07 00:52:10, Neil Puttock wrote:
> ((end

Done.

http://codereview.appspot.com/1682049/diff/3035/9035#newcode167
scm/time-signature-settings.scm:167: ((baseUnit . (1 . 8))
On 2010/07/07 00:52:10, Neil Puttock wrote:
> baseMoment

Done.

http://codereview.appspot.com/1682049/diff/3035/9035#newcode173
scm/time-signature-settings.scm:173: ((baseUnit . (1 . 8))
On 2010/07/07 00:52:10, Neil Puttock wrote:
> baseMoment

Done.

http://codereview.appspot.com/1682049/diff/3035/9035#newcode183
scm/time-signature-settings.scm:183: "Get setting @code{my-symbol} for
@code{time-signature} from
On 2010/07/07 00:52:10, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/1682049/diff/3035/9035#newcode199
scm/time-signature-settings.scm:199: "Get baseUnit value for
@code{time-signature} from
On 2010/07/07 00:52:10, Neil Puttock wrote:
> @code{baseMoment}

Done.

http://codereview.appspot.com/1682049/diff/3035/9035#newcode213
scm/time-signature-settings.scm:213: (/ (* (car numerator) (cdr
denominator))
On 2010/07/07 00:52:10, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/1682049/show