Re: Fix Issue 1290 (issue3832046)

[Carl . D . Sorensen]

Thanks for all the comments.

Keith has identified the correct place to put include the system
horizontal padding, and it now works properly.

New patch set coming shortly.

Carl



http://codereview.appspot.com/3832046/diff/2001/input/regression/skyline-horizontal-padding.ly
File input/regression/skyline-horizontal-padding.ly (right):

http://codereview.appspot.com/3832046/diff/2001/input/regression/skyline-horizontal-padding.ly#newcode13
input/regression/skyline-horizontal-padding.ly:13: \repeat unfold 80 {
<c'''-1 e'''-3 g'''-5> c' <c,-1 e,-3 g,-5> c' }
On 2011/01/02 09:01:11, Trevor Daniels wrote:
> Can not a more precise and shorter test using \break be devised?  With
this code
> interleaving occurs only once right at the bottom even without the
override.

Yes, I'll fix it in the next patch.

http://codereview.appspot.com/3832046/diff/2001/input/regression/skyline-horizontal-padding.ly#newcode13
input/regression/skyline-horizontal-padding.ly:13: \repeat unfold 80 {
<c'''-1 e'''-3 g'''-5> c' <c,-1 e,-3 g,-5> c' }
On 2011/01/02 23:09:02, Keith wrote:
> On 2011/01/02 09:01:11, Trevor Daniels wrote:
> > Can not a more precise and shorter test
> I had a test file for 1290, and related, that shows what does and does
not get
> displaced by the various skylines:
> #(ly:set-option 'debug-skylines #t)
> \score {
>    {
>      \repeat unfold 2 {
>        \mark "mark"
>        a,2_"fa" gisis'''!^"gg" |
>        \mark "m"
>        b,4 a'_"tx" c'2 \break
>      }
>    } \layout {
>      ragged-right = ##t
>      indent = #0
>      \context {
>        \Score
>        \override System #'skyline-horizontal-padding = #0.0
>        \override TimeSignature #'stencil = ##f
>      }
>    }
> }

Thanks, Keith.  I'll look into this as a regression test.

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/page-layout-problem.cc#newcode204
lily/page-layout-problem.cc:204: Real minimum_distance =
up_skyline.distance (bottom_skyline_) + padding;
On 2011/01/02 23:09:02, Keith wrote:
> We need to call your distance here, no?

Yes, thanks for finding this!  You saved me the time of doing it!

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc
File lily/skyline.cc (right):

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode396
lily/skyline.cc:396: if ((i->slope_ == 0) && !isinf (i->y_intercept_) &&
i->y_intercept_ >= 0)
On 2011/01/02 05:19:58, joeneeman wrote:
> Why restrict to y_intercept_ < 0?
 Because if I have a y intercept that is less than zero, and create a
box with that y intercept, the padded skyline comes out with all zero
heights.  I don't know why it works that way, but it does.

I tried several things to get it to behave differently, but was
unsuccessful.  And I don't think that a negative skyline will ever be
limiting in inter-system spacing, so I didn't feel that it was a
problem.

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode398
lily/skyline.cc:398: Interval (i->y_intercept_ < 0 ? i->y_intercept_ : 0
, i->y_intercept_)));
On 2011/01/02 05:19:58, joeneeman wrote:
> ...and if you do restrict to y_intercept_ < 0, then this is redundant.
Yes, this is redundant, left over from one of my tries to properly deal
with negative skylines.

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode399
lily/skyline.cc:399: Skyline padSky = Skyline (boxes, horizon_padding,
a, (Direction) 1);
On 2011/01/02 05:19:58, joeneeman wrote:
> pad_sky instead of padSky
> UP instead of (Direction) 1
> and perhaps a comment about why you need to use UP instead of sky_
(which would
> seem intuitive)

Yes, thank you.  These are good suggestions.  I will fix them.

http://codereview.appspot.com/3832046/diff/2001/lily/skyline.cc#newcode498
lily/skyline.cc:498: const Skyline *padded_other = &other;
On 2011/01/02 05:19:58, joeneeman wrote:
> I'm not sure, but I think "Skyline const *" is more consistent with
the rest of
> the code.

Thanks, I'll try to see if that works.

http://codereview.appspot.com/3832046/