Re: Fixes slope errors from incorrect X extents in Beam::print. (issue 5293060)

[Carl . D . Sorensen]

I don't understand beam-quanting well enough to evaluate most of the
code, but I've seen some concerns in properties and regtests.

Thanks,

Carl



http://codereview.appspot.com/5293060/diff/2004/input/regression/beam-broken-scriabin-individual.ly
File input/regression/beam-broken-scriabin-individual.ly (right):

http://codereview.appspot.com/5293060/diff/2004/input/regression/beam-broken-scriabin-individual.ly#newcode6
input/regression/beam-broken-scriabin-individual.ly:6: texidoc =
"Scriabin's Op. 11 No. 1 with the @cod{broken-beam-style}
@code

http://codereview.appspot.com/5293060/diff/2004/input/regression/beam-broken-scriabin-peters-prolongation.ly
File input/regression/beam-broken-scriabin-peters-prolongation.ly
(right):

http://codereview.appspot.com/5293060/diff/2004/input/regression/beam-broken-scriabin-peters-prolongation.ly#newcode6
input/regression/beam-broken-scriabin-peters-prolongation.ly:6: texidoc
= "Scriabin's Op. 11 No. 1 with the @cod{broken-beam-style}
@code

http://codereview.appspot.com/5293060/diff/2004/input/regression/beam-broken-scriabin-strict-prolongation.ly
File input/regression/beam-broken-scriabin-strict-prolongation.ly
(right):

http://codereview.appspot.com/5293060/diff/2004/input/regression/beam-broken-scriabin-strict-prolongation.ly#newcode6
input/regression/beam-broken-scriabin-strict-prolongation.ly:6: texidoc
= "Scriabin's Op. 11 No. 1 with the @cod{broken-beam-style}
@code

http://codereview.appspot.com/5293060/diff/2004/input/regression/scriabin-defs.ly
File input/regression/scriabin-defs.ly (right):

http://codereview.appspot.com/5293060/diff/2004/input/regression/scriabin-defs.ly#newcode1
input/regression/scriabin-defs.ly:1: \version "2.15.10"
AFAICT, we don't have a method to ignore this file, so when we run
regtests it will be run separately.  And it doesn't have an appropriate
header for  a regtest.

Perhaps it should be put in a separate directory, e.g.
input/regression/includes, so that it won't be part of the regtests.
But I haven't checked the makefiles to verify that this would work.

http://codereview.appspot.com/5293060/diff/2004/input/regression/scriabin-defs.ly#newcode2
input/regression/scriabin-defs.ly:2:
Why do we need to do the Scriabin as a regtest, instead of just doing a
simple two-bar file with a \break (hence a two-staff example)?

In general, the smaller the regtests, the easier it is to spot changes
and to avoid unintended and unimportant differences.

http://codereview.appspot.com/5293060/diff/2004/input/regression/scriabin-defs.ly#newcode219
input/regression/scriabin-defs.ly:219: \override
Staff.DynamicLineSpanner #'staff-padding = #2.5
I'm really nervous about a regtest with lots of \overrides in it.

According to our rules, we shouldn't have \set and \override in a
regtest unless it is strictly necessary to create the behavior we are
testing.

http://codereview.appspot.com/5293060/diff/2004/lily/beam-quanting.cc
File lily/beam-quanting.cc (right):

http://codereview.appspot.com/5293060/diff/2004/lily/beam-quanting.cc#newcode411
lily/beam-quanting.cc:411: init_instance_variables ();//if
(!consistent_broken_slope) printf ("STATS0 EH %4.4f %4.4f X%4.4f Y %4.4f
%4.4f\n", extremal_hangover_[LEFT], extremal_hangover_[RIGHT], x_span_,
unquanted_y_[LEFT], unquanted_y_[RIGHT]);
IMO, you should have a separate branch without the printf statements.
We should not be asked to approve code containing commented out
statements with the assurance that they will be removed before pushing.
We should only be reviewing code intended for pushing (at least if this
patch is intended for pushing as opposed to a design sketch).

The printf statements, particularly in this format, make it very hard to
read the code.

http://codereview.appspot.com/5293060/diff/2004/lily/beam-quanting.cc#newcode1058
lily/beam-quanting.cc:1058: /*
Nice comment here.  This is very helpful.

http://codereview.appspot.com/5293060/diff/2004/lily/beam.cc
File lily/beam.cc (right):

http://codereview.appspot.com/5293060/diff/2004/lily/beam.cc#newcode979
lily/beam.cc:979: bool consistent_broken_slope = broken_beam_style !=
ly_symbol2scm ("individual");
Here (and in scm/define-grobs.scm) you use "individual"; in
scm/define-grob-properties.scm you use "separate".

And I see no code here for "peters".

http://codereview.appspot.com/5293060/diff/2004/lily/beam.cc#newcode1459
lily/beam.cc:1459: "The property @code{broken-beam-style} can be
@code{separate},"
Values should not be listed here.  They are listed in
scm/define-grob-properties.scm.  If they are listed in two places, they
can get out of step and nobody will know which one is correct.  IMO,
these two lines should be completely removed.  There is no need to list
the property that is part of the interface here; it will be
automatically generated by the docs.

http://codereview.appspot.com/5293060/diff/2004/lily/spanner.cc
File lily/spanner.cc (right):

http://codereview.appspot.com/5293060/diff/2004/lily/spanner.cc#newcode240
lily/spanner.cc:240: Interval (1,-1));
Some comments in here about the strategy to get the spanner length would
probably be helpful.  You have three different methods, IIUC, that are
called depending on the situation.

http://codereview.appspot.com/5293060/diff/2004/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/5293060/diff/2004/scm/define-grobs.scm#newcode354
scm/define-grobs.scm:354: (broken-beam-style . individual)
This is not one of the alternatives listed in
scm/define-grob-properties.scm

http://codereview.appspot.com/5293060/