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/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel