Spacing with empty contexts; issue 1669

commit 
    page-layout: skip spacing spec for empty lines; issue 1669

 input/regression/dynamics-empty.ly |   26 ++++++++++++++++++++++++++
 lily/page-layout-problem.cc        |    3 +++
 2 files changed, 29 insertions(+), 0 deletions(-)

commit 
    align-interface.cc: Clarify code for empty staves

 lily/align-interface.cc |   79 ++++++++++++++++-------------------------------
 1 files changed, 27 insertions(+), 52 deletions(-)

commit
    Revert "Prevents chord names from being pushed into the staff (issue 3160)"
    
    This reverts commit d4802c72d26def39030f2ac897b66c3a268888d5.

 lily/align-interface.cc |   55 ++++++++++++++++------------------------------
 1 files changed, 19 insertions(+), 36 deletions(-)

[joeneeman, 2011-06-07 04:48:01]

http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc
File lily/align-interface.cc (right):

http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newco...
lily/align-interface.cc:222: dy = max (dy, min_distance);
If pure is true, there may be some staves that are going to be removed. However,
these staves won't be removed until after line-breaking. Now, they're thrown
away because they have an empty skyline but if you change that, you'll probably
need to add a check here, before including padding and min_distance.

A good way to check that it's working would be to add annotate-spacing = ##t to
input/regression/hara-kiri-staff.ly and check that the Y-extent-estimates are
accurate.

http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newco...
lily/align-interface.cc:229: // So if stretchability=0, treat basic-distance as
a minimum-distance.
This is no longer needed, right?

[Keith, 2011-06-09 02:55:54]

http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc
File lily/align-interface.cc (right):

http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newco...
lily/align-interface.cc:222: dy = max (dy, min_distance);
On 2011/06/07 04:48:01, joeneeman wrote:
> If pure is true, there may be some staves that are going to be removed.
However,
> these staves won't be removed until after line-breaking. 

... and, as you say, these staves are not yet removed when Y-extent-estimates
for the page breaker are calculated, so this patch messes up the page breaker.

If the page breaker depends on empty, but living, staves disappearing for
vertical spacing purposes, then maybe empty staves should be treated that way
consistently. That would be tantamount to giving all empty-able contexts the
hara-kiri engraver, as you suggested much earlier.

The obvious alternative would be to have the empty staves decide whether to
suicide before page breaking, but my suspicion is that is awkward, or it would
have been done that way at first.

Having minimum_distances() change behavior based on whether 'pure' is true is
too tricky for me. (how do we know that any staff that is empty we are pure will
suicide later)

I'll think on it, but mark the patch as abandoned for now.

[joeneeman, 2011-06-09 06:22:27]

On 2011/06/09 02:55:54, Keith wrote:
> http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc
> File lily/align-interface.cc (right):
> 
>
http://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newco...
> lily/align-interface.cc:222: dy = max (dy, min_distance);
> On 2011/06/07 04:48:01, joeneeman wrote:
> > If pure is true, there may be some staves that are going to be removed.
> However,
> > these staves won't be removed until after line-breaking. 
> 
> ... and, as you say, these staves are not yet removed when Y-extent-estimates
> for the page breaker are calculated, so this patch messes up the page breaker.
> 
> If the page breaker depends on empty, but living, staves disappearing for
> vertical spacing purposes, then maybe empty staves should be treated that way
> consistently.

Or maybe we should just be checking for hara-kiri instead of checking whether
the extent is empty... I'll try putting together a patch for this, since it
would be less confusing than the current situation.

> The obvious alternative would be to have the empty staves decide whether to
> suicide before page breaking, but my suspicion is that is awkward, or it would
> have been done that way at first.

Yes: we don't know whether a staff is empty until after line breaking.

[MikeSol, 2013-02-11 05:01:28]

https://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc
File lily/align-interface.cc (right):

https://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newc...
lily/align-interface.cc:222: dy = max (dy, min_distance);
On 2011/06/09 02:55:55, Keith wrote:
> On 2011/06/07 04:48:01, joeneeman wrote:
> > If pure is true, there may be some staves that are going to be removed.
> However,
> > these staves won't be removed until after line-breaking. 
> 
> ... and, as you say, these staves are not yet removed when Y-extent-estimates
> for the page breaker are calculated, so this patch messes up the page breaker.
> 
> If the page breaker depends on empty, but living, staves disappearing for
> vertical spacing purposes, then maybe empty staves should be treated that way
> consistently. That would be tantamount to giving all empty-able contexts the
> hara-kiri engraver, as you suggested much earlier.
> 
> The obvious alternative would be to have the empty staves decide whether to
> suicide before page breaking, but my suspicion is that is awkward, or it would
> have been done that way at first.
> 
> Having minimum_distances() change behavior based on whether 'pure' is true is
> too tricky for me. (how do we know that any staff that is empty we are pure
will
> suicide later)
> 
> I'll think on it, but mark the patch as abandoned for now.

Couldn't there just be a vector<bool> ignore_me that contains the result
request_suicide for all elements (when pure)?  Then, the for loop would be
continued for these elements and they wouldn't have to be removed in
get_skylines.

[Keith, 2013-02-11 05:41:59]

https://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc
File lily/align-interface.cc (right):

https://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newc...
lily/align-interface.cc:222: dy = max (dy, min_distance);
On 2013/02/11 05:01:28, MikeSol wrote:
> 
> Couldn't there just be a vector<bool> ignore_me that contains the result
> request_suicide for all elements (when pure)?  Then, the for loop would be
> continued for these elements and they wouldn't have to be removed in
> get_skylines.

I think yes. ('removed_as_empty' would be a good name)
That array could be built by get_skylines(), since it already calls
pure_height(), which in turn calls request_suicide(..., start, end).

To truly distinguish the concepts of 'empty' versus 'removed', we could, in
get_skylines line 98
else if ( !(removed_as_empty[i] = request_suicide(elements[i], start, end)))
but that results in an additional call to request_suicide() per VertAxisGroup
per pass through the line-breaking

https://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newc...
lily/align-interface.cc:229: // So if stretchability=0, treat basic-distance as
a minimum-distance.
On 2011/06/07 04:48:01, joeneeman wrote:
> This is no longer needed, right?

This chunk is now no longer needed.

[mike7, 2013-02-11 06:20:39]

On 11 févr. 2013, at 06:41, k-ohara5a5a@oco.net wrote:

> 
> https://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc
> File lily/align-interface.cc (right):
> 
>
https://codereview.appspot.com/4515158/diff/2009/lily/align-interface.cc#newc...
> lily/align-interface.cc:222: dy = max (dy, min_distance);
> On 2013/02/11 05:01:28, MikeSol wrote:
> 
>> Couldn't there just be a vector<bool> ignore_me that contains the
> result
>> request_suicide for all elements (when pure)?  Then, the for loop
> would be
>> continued for these elements and they wouldn't have to be removed in
>> get_skylines.
> 
> I think yes. ('removed_as_empty' would be a good name)
> That array could be built by get_skylines(), since it already calls
> pure_height(), which in turn calls request_suicide(..., start, end).
> 
> To truly distinguish the concepts of 'empty' versus 'removed', we could,
> in get_skylines line 98
> else if ( !(removed_as_empty[i] = request_suicide(elements[i], start,
> end)))
> but that results in an additional call to request_suicide() per
> VertAxisGroup per pass through the line-breaking

Ok - I'll be able to work on this once the assertion error clears up from
annotate-spacing (reported to the bug list a few minutes ago).

Cheers,
MS

[Keith, 2013-05-01 19:57:30]

https://codereview.appspot.com/4515158/diff/18002/lily/align-interface.cc
File lily/align-interface.cc (left):

https://codereview.appspot.com/4515158/diff/18002/lily/align-interface.cc#old...
lily/align-interface.cc:66: create a flat skyline from the bounding box */
Some comments seemed to be no longer true, so I'm gradually verifying them
before I put them back in.

[Keith, 2013-05-07 03:07:43]

https://codereview.appspot.com/4515158/diff/34002/lily/align-interface.cc
File lily/align-interface.cc (right):

https://codereview.appspot.com/4515158/diff/34002/lily/align-interface.cc#new...
lily/align-interface.cc:74: {
I removed an outer loop, so I uploaded a version *before* running the code
indenter.  (remember to run the indenter before pushing.)