Re: Adds padding between Hairpins and SpanBars. (issue 5438060)

[address@hidden]

On Nov 29, 2011, at 10:22 AM, Keith OHara wrote:

> On Mon, 28 Nov 2011 23:38:28 -0800, address@hidden <address@hidden> wrote:
> 
>> In the original report, my eyes zoomed directly to the second system, last 
>> measure (which my patch fixes).
> 
> Just to be clear, the one-line fix linked to issue 2060 fixes that original 
> report as well.
> 
>> I see what you mean that these two work independently.
> 
> Well, they don't work independently, because your code adds more gap to 
> whatever the original code already had.  It looks already like you had to 
> adapt your code to avoid double-padding where the original code was 
> successful.
> 
> You should incorporate the bugfix
>> if (bound->is_non_musical (bound) || bound->break_status_dir ())
> into your patch, so that you start with a consistent set of hairpin lengths 
> going into your new Hairpin::real_span_bar_padding(). Clearly the original 
> code intended to apply to all non-musical columns, so it will be very 
> confusing if Hairpin::print() fails to do what it appears to be doing, but 
> then Hairpin::real_span_bar_padding() compensates for the failure.
> 
> Also, if real_span_bar_padding() assumes that it starts with the buggy 
> lengths from print(), eventually somebody will adjust your new properties, 
> and uncover the former inconsistent behavior.
> 

What is difficult about all this is that the regtests don't help at all, as 
this is in the print function, so the results would only come out in a pixel 
comparator.

A couple things:

1) I think that it would be worth it to revamp (or rename) non_musical so that 
it operates more intuitively.  And by intuitively, I mean that if an Item is 
non-musical, it stands to reason that its broken copies should be non-musical 
as well.  It's a stretch to say that a clef, when broken, has musicality 
conferred onto it.  So, I think that the function should either be renamed to 
explain what it actually does (non_musical_and_non_original, for example) or 
made to actually be non_musical.

2) I've made a version of the patchset that deletes the bit about the extra 
padding for non_musical columns (so you're right that the two patches are not 
compatible).  Hairpins can only ever have a non-musical column for a bound if 
they are broken, and as my patch deals with broken hairpins, it makes this line 
redundant in all scenarios.  It seems like removing it gets rid of any 
potential issues, but again, w/o being able to see how the regtests change, 
it's tough to confirm @ 100% (I am running all hairpin regtests manually & 
looking @ them w/ the naked eye).

New patch set up w/ this minor change.

Cheers,
MS