Re: Add support for custom ledger positions, using two new staff-symbol properties (issue 4974075)

[pierstitus]

New, simpler, patch added

On 2011/10/17 18:56:58, Keith wrote:
> Most of the complexity is due to the repeating cycle facility.  First,
you might
> try out that feature, and reconsider whether it is worth the
complexity in the
> code.

I tried it for a number of notations from http://musicnotation.org/
Otherwise it should be specified with repetitions, enough to cover a
sensible range, but now it is more universal.


> http://codereview.appspot.com/4974075/diff/28001/lily/staff-symbol.cc
> File lily/staff-symbol.cc (right):


http://codereview.appspot.com/4974075/diff/28001/lily/staff-symbol.cc#newcode184
> lily/staff-symbol.cc:184: me->warning (_ ("ledger_positions is not
sorted, may
> show unexpected behaviour"));
> Probably not necessary, because the printed ledger lines demonstrate
the
> behavior.  If somebody re-arranges the ledger lines, does the warning
print once
> per system or just once when they are set?
indeed it prints the warning once per system, not nice.


http://codereview.appspot.com/4974075/diff/28001/lily/staff-symbol.cc#newcode186
> lily/staff-symbol.cc:186: if (cycle < 0.1)
>    cycle = 2.0;
also possible, but if input is incorrect I'd prefer to not print ledger
lines at all, that makes it more clear something is wrong.


http://codereview.appspot.com/4974075/diff/28001/lily/staff-symbol.cc#newcode200
> lily/staff-symbol.cc:200: do
> We want a test here to ensure we can follow the tail of 's' in line
231 below.
> Otherwise a mysterious segfault could appear if the test above
involving 'cycle'
> is ever changed.

I made the patch much simpler, it just checks the conditions that can go
wrong in the loop, and doesn't print ledgers if those conditions are not
met. Is this what you mean?

Alternative I could rewrite the loops as for loops, which might be more
clear, but I don't think it's worth the effort, especially since for
loops such as on line 166 are not clear as water either.

Piers

http://codereview.appspot.com/4974075/