Issue 2990: \RemoveEmptyStaves in StaffGroup context crashes

Issue 2990: \RemoveEmptyStaves in StaffGroup context crashes

The problem is caused by multiple axis group engravers, and in
particular nested VerticalAxisGroup grobs in connection with the
skyline code.  An extensive fix catching any recursive grob definition
will lead to quite more cryptic error messages rather than this
hand-tailored fix for the special case of VerticalAxisGroup nesting.

The solution chosen here is to complain when a VerticalAxisGroup is
announced from a subordinate context and let the current group commit
suicide.

[Keith, 2013-01-10 06:40:08]

Anything with a line break still causes a crash.  I can't say exactly why,
because I'm a bit confused which VerticalAxisGroup gets removed, the outer or
the inner.

\new StaffGroup \with { \RemoveEmptyStaves
  }  << { b1 b1 b1}
        { b1 b1 \break b1 } >>

My only suggestion is to merge "Axis_group_engraver" with "Hara_kiri_engraver"
engraver (and give it a sensible name like "Line_engraver") but that is much
more work.

[dak, 2013-01-10 09:00:43]

On 2013/01/10 06:40:08, Keith wrote:
> Anything with a line break still causes a crash.  I can't say exactly why,
> because I'm a bit confused which VerticalAxisGroup gets removed, the outer or
> the inner.

The outer group commits suicide once it hears of an inner one.  I tried it the
other way round first (which would appear to make more sense), but that made the
original report bomb out: possibly one would have needed to better code the case
where an outer group is not happy with creation of an inner one.  It is also
possible that this created a timing problem where the inner group was first
created (with the outer group not being in existence), and then afterwards the
outer group being created and never getting notice of the inner one as its
creation time had already been over.

At any rate, it would appear that listening to the creation messages (which did
not require any additional bookkeeping overhead) is tricky.  If nobody comes up
with a "oh, this was just missing $x suggestion" in due time, it might provide a
cleaner slate for a better fix if this patch gets reverted.

[mike7, 2013-01-10 09:05:21]

On 10 janv. 2013, at 07:40, k-ohara5a5a@oco.net wrote:

> Anything with a line break still causes a crash.  I can't say exactly
> why, because I'm a bit confused which VerticalAxisGroup gets removed,
> the outer or the inner.
> 
> \new StaffGroup \with { \RemoveEmptyStaves
>  }  << { b1 b1 b1}
>        { b1 b1 \break b1 } >>
> 
> My only suggestion is to merge "Axis_group_engraver" with
> "Hara_kiri_engraver" engraver (and give it a sensible name like
> "Line_engraver") but that is much more work.
> 
> https://codereview.appspot.com/7069044/

It's actually not as bad as you'd think.  The Hara_kiri_engraver is just a thin
wrapper around the Axi_group_engraver.  Everything can be switched on and off
via an extra context property haraKiri.  I was able to drum up a sketch in 10
minutes.  Running the regtests now.

Cheers,
MS

[dak, 2013-01-10 09:19:09]

On 2013/01/10 09:05:21, mike7 wrote:
 
> It's actually not as bad as you'd think.  The Hara_kiri_engraver is just a
thin
> wrapper around the Axi_group_engraver.  Everything can be switched on and off
> via an extra context property haraKiri.  I was able to drum up a sketch in 10
> minutes.  Running the regtests now.

Why would one even need an extra context property?  Isn't that what the
remove-empty property is already supposed to be for?

[mike7, 2013-01-10 09:21:33]

On 10 janv. 2013, at 10:19, dak@gnu.org wrote:

> On 2013/01/10 09:05:21, mike7 wrote:
> 
>> It's actually not as bad as you'd think.  The Hara_kiri_engraver is
> just a thin
>> wrapper around the Axi_group_engraver.  Everything can be switched on
> and off
>> via an extra context property haraKiri.  I was able to drum up a
> sketch in 10
>> minutes.  Running the regtests now.
> 
> Why would one even need an extra context property?  Isn't that what the
> remove-empty property is already supposed to be for?
> 
> https://codereview.appspot.com/7069044/

Didn't really look into it, as remove-empty is a grob property so I assumed that
it was for the actions performed by the grob, not by the engraver making the
grob.  I'll have a look later.

Cheers,
MS

[dak, 2013-01-10 09:37:58]

On 2013/01/10 09:21:33, mike7 wrote:
> On 10 janv. 2013, at 10:19, mailto:dak@gnu.org wrote:
> 
> > On 2013/01/10 09:05:21, mike7 wrote:
> > 
> >> It's actually not as bad as you'd think.  The Hara_kiri_engraver is
> > just a thin
> >> wrapper around the Axi_group_engraver.  Everything can be switched on
> > and off
> >> via an extra context property haraKiri.  I was able to drum up a
> > sketch in 10
> >> minutes.  Running the regtests now.
> > 
> > Why would one even need an extra context property?  Isn't that what the
> > remove-empty property is already supposed to be for?
> > 
> > https://codereview.appspot.com/7069044/
> 
> Didn't really look into it, as remove-empty is a grob property so I assumed
that
> it was for the actions performed by the grob, not by the engraver making the
> grob.

I don't think that is a contradiction.  After announcing the grob, overrides,
tweaks and so on should be applied to it, and I think that referencing the
remove-empty property of the grob at that time in order to decide whether to do
the listening and bookkeeping for a harakiri engraver should be fine.

> I'll have a look later.

Thanks.