[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: address@hidden
Subject: Re: Fixes slope errors from incorrect X extents in Beam::print. (issue 5293060)
Date: Thu, 27 Oct 2011 10:33:58 +0200

On Oct 26, 2011, at 10:13 PM, address@hidden wrote:

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




In general, you're absolutely right about the regtests.  I've scrapped them all and replaced them with the most pertinent extracts from the old regtest.
File lily/ (right):
lily/ 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.

OK - removed.
lily/ /*
Nice comment here.  This is very helpful.

File lily/ (right):
lily/ 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".

Will change.

And I see no code here for "peters".

Check Beam::quanting
lily/ "The property @code{broken-beam-style} can be
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.

OK - removed.
File lily/ (right):
lily/ 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.

File scm/define-grobs.scm (right):
scm/define-grobs.scm:354: (broken-beam-style . individual)
This is not one of the alternatives listed in

Naming fixed.

Your comments are really helpful - I don't expect people to know how beam-quanting works (it took me a long long time to figure it out), but I think I focused so much on the technical details of quanting that I let some basic stuff (like consistency in naming) slip.  I certainly appreciate your catching it!


reply via email to

[Prev in Thread] Current Thread [Next in Thread]