On Sun, Jan 2, 2011 at 6:33 PM, Benkő Pál
<address@hidden> wrote:
hi Joe,
need to be careful about what happens when bar-size is set.
> Currently, your patch will break (for example) input/regression/
drums.ly
> because it ignores bar-size.
as it loses information, but the other way round.
That's true, but we currently allow users to override bar-size (for example in percussion parts). With your current patch, if the user overrides bar-size, then Bar_line::calc_bar_size and Bar_line::calc_bar_extent will give conflicting results (I was wrong earlier when I said that this would affect the printed bar line, but I still think it's bad for the functions to conflict).
AFAICS the intent of the span bar code is to draw lines
not between staves but from line to line, and my patch
reverts this.
of course there's no difference in general, but there is
if bar lines are to have different extent than that of
their staff; and there is a further difference whether
the bar is shorter or longer than the staff: if longer,
there may be no point in using span bars at all; if
shorter, then perhaps span bars are better placed just
between staves (see spantest5.ly).
I don't think the current intention is documented anywhere. If you want to fix a policy now, that's ok with me.
I still don't know exactly how should spantest3 behave
(spantest5 makes me feel it works acceptably), neither
what should happen if different bar types are connected
(see spantest4.ly - I hope this is farthest from a real
world case of all my tests).
>>> I removed the center setting code and that
>>> (with my patch still active) made my example perfect;
>>> however, the attached complementary test (with bar
>>> lines only within staff, not between them) failed,
>>> but it's perfect with current center setting
>>> (independently whether my original patch is active or not).
>> Since Bar_line::compound_barline is used in both BarLine and SpanBar, you
>> will need to find some solution that works for both cases. It won't be as
>> simple as just enabling or disabling the centering code.
I moved the centering code from compound_barline to print,
and this way all regtests are OK and all my spantests are
good or at least acceptable.
Thanks, this patch looks good. Just a couple of things I'd like to see:
- a comment explaining the positioning of span-bars (ie. between bar-lines or between staves?)
- agreement between bar-size and bar-extent. There are two solutions I can see, but feel free to invent your own:
- deprecate the bar-size property and use bar-extent from now on. This will require a convert-ly rule.
- unset bar-size by default (in scm/define-grob-properties.scm). In calc_bar_extent, check to see if bar-size is set. If it is, use it. Otherwise, use the staff extent
Here are a couple other suggestions that I think would improve the code. I'd be happy to commit your patch without them because they reflect problems with the existing code rather than problems with your patch. But if you have time, it would be nice :)
- Shouldn't Bar_line::print use bar-extent rather than bar-size? That seems more natural.
- The centering issue regarding compound_barline still seems strange. Wouldn't it be nicer if compound_barline took Real bottom and Real top (or maybe Interval) parameters (instead of Real h) and drew a bar line whose Y-extent stretched from bottom to top? That seems like a more intuitive API to me (and unlike the current one, it's self-documenting).
Thanks for your work,
Joe