Re: Revised autobeam settings patch -- cleaned up debug comments (issue1667044)

[Carl . D . Sorensen]

THanks for the review, Neil.

As far as the indentation on the alist goes, did you reindent the whole
list, or just the beam-settings part?  I ask this, because the sample
you gave has different indentation for beam-settings than for the time
signature.

Thanks,

Carl



http://codereview.appspot.com/1667044/diff/21001/22013
File
Documentation/snippets/new/conducting-signs,-measure-grouping-signs.ly
(right):

http://codereview.appspot.com/1667044/diff/21001/22013#newcode1
Documentation/snippets/new/conducting-signs,-measure-grouping-signs.ly:1:
%% Note: this file works from version 2.13.22
On 2010/06/22 22:58:53, Neil Puttock wrote:
> remove

Fixed, and changed version to 2.13.26.

http://codereview.appspot.com/1667044/diff/21001/22013#newcode31
Documentation/snippets/new/conducting-signs,-measure-grouping-signs.ly:31:
} % begin verbatim
On 2010/06/22 22:58:53, Neil Puttock wrote:
> remove

Done.

http://codereview.appspot.com/1667044/diff/21001/22015
File Documentation/snippets/new/reverting-default-beam-endings.ly
(right):

http://codereview.appspot.com/1667044/diff/21001/22015#newcode1
Documentation/snippets/new/reverting-default-beam-endings.ly:1: \version
"2.13.24"
Changed to 2.13.26

http://codereview.appspot.com/1667044/diff/21001/22017
File input/regression/les-nereides.ly (right):

http://codereview.appspot.com/1667044/diff/21001/22017#newcode1
input/regression/les-nereides.ly:1: \version "2.13.4"
Changed to 2.13.26

http://codereview.appspot.com/1667044/diff/21001/22023
File ly/bagpipe.ly (right):

http://codereview.appspot.com/1667044/diff/21001/22023#newcode12
ly/bagpipe.ly:12: \version "2.13.20"
Changed to 2.13.26

http://codereview.appspot.com/1667044/diff/21001/22025
File ly/music-functions-init.ly (right):

http://codereview.appspot.com/1667044/diff/21001/22025#newcode425
ly/music-functions-init.ly:425: #(override-time-signature-setting
On 2010/06/22 22:58:53, Neil Puttock wrote:
> Is override-time-signature-setting likely to be used outside this
music function
> wrapper?  If not, then it would be simpler to remove ly:export from
the function
> and remove the #{ }# block.

> (same for revert-time-signature-setting)

Done.

http://codereview.appspot.com/1667044/diff/21001/22026
File python/convertrules.py (right):

http://codereview.appspot.com/1667044/diff/21001/22026#newcode3013
python/convertrules.py:3013: stderr_write(NOT_SMART %
_("\overrideBeamSettings is obsolete; see \set BeamSettings and
\overrideTimeSignatureSettings.\n"))
On 2010/06/22 22:58:53, Neil Puttock wrote:
> NOT_SMART needs a sentence continuation

Done.

http://codereview.appspot.com/1667044/diff/21001/22028
File scm/define-context-properties.scm (right):

http://codereview.appspot.com/1667044/diff/21001/22028#newcode476
scm/define-context-properties.scm:476: time signatures.  Contains an
elements for various time signatures.  The
On 2010/06/22 22:58:53, Neil Puttock wrote:
> Contains elements

Done.

http://codereview.appspot.com/1667044/diff/21001/22028#newcode477
scm/define-context-properties.scm:477: element for each time signature
contains entries for @code{measure-length},
On 2010/06/22 22:58:53, Neil Puttock wrote:
> measureLength

Done.

http://codereview.appspot.com/1667044/diff/21001/22028#newcode478
scm/define-context-properties.scm:478: @code{measure-grouping}, and
@code{beamSettings}.")
On 2010/06/22 22:58:53, Neil Puttock wrote:
> measureGrouping

Done.

http://codereview.appspot.com/1667044/diff/21001/22031
File scm/music-functions.scm (right):

http://codereview.appspot.com/1667044/diff/21001/22031#newcode561
scm/music-functions.scm:561: (map (lambda(x) (/ (* 8 x) den))
On 2010/06/22 22:58:53, Neil Puttock wrote:
> lambda (x)

Done.

http://codereview.appspot.com/1667044/diff/21001/22032
File scm/time-signature-settings.scm (right):

http://codereview.appspot.com/1667044/diff/21001/22032#newcode197
scm/time-signature-settings.scm:197: ((beat-length . (1 . 4))
On 2010/06/22 22:58:53, Neil Puttock wrote:
> I think (3 . 4) is more likely

> e.g., Chopin's G minor Ballade


We achieve the equivalent of 3/4 beat length by setting the beat length
to 1/4 and using measureGrouping of (3 3) and a default end rule of (1 .
8) . (6 6).

This is necessary because we want to have measure grouping signs of
triangles for the first and second half of the measure, which requires a
group of 3.

I think we can make this stronger by eliminating the (1 . 8) end rule
and replacing it with (1 . 4) . (3 3) .  This has the effect of making
any beam break at 3/4, unless a finer rule is given.

At present, the rules are basically translated directly from the current
beam-settings.scm.  I was planning on overhauling this list once the
basic functionality of the patch was accepted.  But perhaps I should go
ahead and revise this list now.

http://codereview.appspot.com/1667044/diff/21001/22033
File scripts/build/output-distance.py (right):

http://codereview.appspot.com/1667044/diff/21001/22033#newcode90
scripts/build/output-distance.py:90: system ('compare -depth 8
%(dir)s/crop1.png %(dir)s/crop2.png %(dir)s/diff.png' % locals ())
On 2010/06/22 22:58:53, Neil Puttock wrote:
> not part of this patch set

It's not part of my patch set in git.  This change shows up due to my
using a version with the patch applied as my reference.  I'll try to get
the right reference for the next set.

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