Eliminates the Hara_kiri_engraver.

Eliminates the Hara_kiri_engraver.

Uses a haraKiri context property in the Axis_group_engraver to
acheive the same functionality.

[MikeSol, 2013-01-10 19:49:51]

This doesn't crash in the test case Keith proposed:

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

[dak, 2013-01-14 08:46:23]

What happened to "I'll have a look later" in
<URL:https://codereview.appspot.com/7069044#msg5>?  Have you even _tried_ using
the remove-empty property of the created axis spanner instead of adding a new
haraKiri property duplicating its functionality?

[mike7, 2013-01-14 09:02:04]

On 14 janv. 2013, at 09:46, dak@gnu.org wrote:

> What happened to "I'll have a look later" in
> <URL:https://codereview.appspot.com/7069044#msg5>?  Have you even
> _tried_ using the remove-empty property of the created axis spanner
> instead of adding a new haraKiri property duplicating its functionality?
> 

No, no time yet - will do ASAP.

Cheers,
MS

[Keith, 2013-01-14 17:50:47]

https://codereview.appspot.com/7061062/diff/1/lily/axis-group-engraver.cc
File lily/axis-group-engraver.cc (right):

https://codereview.appspot.com/7061062/diff/1/lily/axis-group-engraver.cc#new...
lily/axis-group-engraver.cc:122: {
The property haraKiri is misleadingly named (because it selects book-keeping,
not suicide) and is now redundant with keepAliveInterfaces.

For backward-compatibility, with useful methods like 
<http://lists.gnu.org/archive/html/lilypond-user-fr/2012-08/msg00051.html>,
 I suggest leaving it defined as-is at Score level, with an override to '()
wherever we used Axis_group rather than Hara_kiri_engraver.

https://codereview.appspot.com/7061062/diff/1/ly/context-mods-init.ly
File ly/context-mods-init.ly (right):

https://codereview.appspot.com/7061062/diff/1/ly/context-mods-init.ly#newcode23
ly/context-mods-init.ly:23: haraKiri = ##t
This could be  
\unset KeepAliveInterfaces  % use default, set at Score level

https://codereview.appspot.com/7061062/diff/1/ly/engraver-init.ly
File ly/engraver-init.ly (right):

https://codereview.appspot.com/7061062/diff/1/ly/engraver-init.ly#newcode46
ly/engraver-init.ly:46: haraKiri = ##t
Why insert this line between the comment and the code being commented? 

If we use keepAliveInterfaces instead, and leave it defined as-is at the Score
level, then we need nothing here.

https://codereview.appspot.com/7061062/diff/1/ly/engraver-init.ly#newcode89
ly/engraver-init.ly:89: 
But here, we would want
  KeepAliveInterfaces = '()
to avoid the wasted book-keeping effort (as your boolean haraKiri defaulting to
false does in the current patch).

https://codereview.appspot.com/7061062/diff/1/ly/engraver-init.ly#newcode194
ly/engraver-init.ly:194: 
and here

https://codereview.appspot.com/7061062/diff/1/ly/engraver-init.ly#newcode409
ly/engraver-init.ly:409: 
and here

[Keith, 2013-01-14 18:20:44]

https://codereview.appspot.com/7061062/diff/1/lily/axis-group-engraver.cc
File lily/axis-group-engraver.cc (right):

https://codereview.appspot.com/7061062/diff/1/lily/axis-group-engraver.cc#new...
lily/axis-group-engraver.cc:122: {
On 2013/01/14 17:50:47, Keith wrote:
> The property haraKiri [...] is now redundant with keepAliveInterfaces.
> 

Or with remove-empty, I suppose.
If you can access the value with 
 staffline_ && to_boolean(staffline_ -> get_property("remove-empty"))
at this point, that would seem to be simpler.

[dak, 2013-01-14 18:25:24]

https://codereview.appspot.com/7061062/diff/1/lily/axis-group-engraver.cc
File lily/axis-group-engraver.cc (right):

https://codereview.appspot.com/7061062/diff/1/lily/axis-group-engraver.cc#new...
lily/axis-group-engraver.cc:122: {
On 2013/01/14 18:20:44, Keith wrote:
> On 2013/01/14 17:50:47, Keith wrote:
> > The property haraKiri [...] is now redundant with keepAliveInterfaces.
> > 
> 
> Or with remove-empty, I suppose.
> If you can access the value with 
>  staffline_ && to_boolean(staffline_ -> get_property("remove-empty"))
> at this point, that would seem to be simpler.

It would also mean that we can just leave keepAliveInterfaces alone when
switching between harakiri behavior and otherwise.  Having to override a complex
variable back and forth for a basic switch in behavior is error-prone.

[Keith, 2013-01-14 19:22:09]

On Mon, 14 Jan 2013 10:25:24 -0800, <dak@gnu.org> wrote:

>> If you can access the value with
>>   staffline_ && to_boolean(staffline_ -> get_property("remove-empty"))
>> at this point, that would seem to be simpler.
>
> It would also mean that we can just leave keepAliveInterfaces alone when
> switching between harakiri behavior and otherwise.  Having to override a
> complex variable back and forth for a basic switch in behavior is
> error-prone.
>

Unfortunately, 'remove-empty is a property of the VerticalAxisGroup (the
grouping-object that holds a line) so the same value of 'remove-empty gets
copied to the broken lines.  The separate VerticalAxisGroup objects for each
broken line are not created by the engraver, but by the line-breaker, so
changing the property with \set s in the input does not change it in the
appropriate lines of output.

keepAliveInterfaces can be changed in the music stream.

> https://codereview.appspot.com/7061062/

[dak, 2013-01-20 12:51:06]

Comment #7 from Keith has not been addressed as far as I can see, but I have to
admit that I don't understand a thing from it, so I can't estimate whether it is
mistaken or indicates a real usage problem.

Other than that, it looks like a welcome simplification.

https://codereview.appspot.com/7061062/diff/9001/lily/axis-group-engraver.cc
File lily/axis-group-engraver.cc (right):

https://codereview.appspot.com/7061062/diff/9001/lily/axis-group-engraver.cc#...
lily/axis-group-engraver.cc:122: if (staffline_ &&
to_boolean(staffline_->get_property("remove-empty")))
If "sneaky callback" is a real concern, it would be feasible to just store a
copy of remove-empty in the engraver right after staffline_ has been created.

However, I don't really think that a sneaky callback could cause any awful
effects: it would just likely remove a group too many or too few because of
missing or spurious bookkeeping.

[MikeSol, 2013-01-20 16:11:48]

"sneaky callback" is not a real concern...it's a pedantic concern.
If we're following the model to a T, grob properties should not be read in
engravers, just set.  The idea is that the engravers are creating the grobs and
fixing them up with properties if need be.  I prefer duplicating a context and
grob property rather than just having the grob property, but people seem against
this.  It is a trivial fix if people become for it.

I think Keith's comment is fixed in this patch set (although I must admit that I
don't completely follow it either...this is a new corner of the code for me).

[Keith, 2013-01-21 18:23:33]

On 2013/01/14 18:25:24, dak wrote:
> On 2013/01/14 18:20:44, Keith wrote:
> >
> > If you can access the value with 
> >  staffline_ && to_boolean(staffline_ -> get_property("remove-empty"))
> > at this point, that would seem to be simpler.
> 
> It would also mean that we can just leave keepAliveInterfaces alone when
> switching between harakiri behavior and otherwise.

How would you propose to do that ?

Changing the remove-empty property mid-score will not work, for the same reason
that 
  \paper {ragged-right = ##t }
  \relative f {
    c''2( d \break 
    \override Slur #'color = #red
    e f \break g f e d c1) }
does not re-color a portion of the broken slur.

[Keith, 2013-01-21 18:24:23]

LGTM

[Keith, 2013-01-22 04:30:25]

But don't forget to run scripts/auxiliar/fixcc.py before pushing.

[Keith, 2013-01-25 08:05:28]

https://codereview.appspot.com/7061062/diff/9001/lily/axis-group-engraver.cc
File lily/axis-group-engraver.cc (right):

https://codereview.appspot.com/7061062/diff/9001/lily/axis-group-engraver.cc#...
lily/axis-group-engraver.cc:120: // mess everything up...but at least this
avoids a duplicate context
Please demonstrate a sneaky callback messing things up.

[dak, 2013-01-25 09:29:28]

On 2013/01/25 08:05:28, Keith wrote:
> https://codereview.appspot.com/7061062/diff/9001/lily/axis-group-engraver.cc
> File lily/axis-group-engraver.cc (right):
> 
>
https://codereview.appspot.com/7061062/diff/9001/lily/axis-group-engraver.cc#...
> lily/axis-group-engraver.cc:120: // mess everything up...but at least this
> avoids a duplicate context
> Please demonstrate a sneaky callback messing things up.

Since sneaky callbacks can mess a lot of things up, a comment like that would
likely be only warranted if "sneaky callback" can cause conditions that lead to
programming errors, and possibly even just crashes.

[mike7, 2013-01-25 13:02:39]

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

> On 2013/01/25 08:05:28, Keith wrote:
> 
> https://codereview.appspot.com/7061062/diff/9001/lily/axis-group-engraver.cc
>> File lily/axis-group-engraver.cc (right):
> 
> 
>
https://codereview.appspot.com/7061062/diff/9001/lily/axis-group-engraver.cc#...
>> lily/axis-group-engraver.cc:120: // mess everything up...but at least
> this
>> avoids a duplicate context
>> Please demonstrate a sneaky callback messing things up.
> 
> Since sneaky callbacks can mess a lot of things up, a comment like that
> would likely be only warranted if "sneaky callback" can cause conditions
> that lead to programming errors, and possibly even just crashes.

To be fair, the callback in question would be no sneakier than your average
sneaky callback.  In general, reading grob properties in the engraver stage is a
bad idea. I am guilty of writing code that does this, but that was before I saw
the light.  Something to the effect of:

\override Staff.VerticalAxisGroup #'remove-empty = #(lambda (grob)
(iterate-through-all-elements-and-get-Y-extent grob))

would do the trick.  Granted, this would also be bad after the engraver stage,
but worse in the engraver stage.

However, as I said above, this type of thing can be done at other places in the
code too, so perhaps it is worth it to take out the comment.

Cheers,
MS

[dak, 2013-01-25 13:29:19]

On 2013/01/25 13:02:39, mike7 wrote:

> To be fair, the callback in question would be no sneakier than your
> average sneaky callback.  In general, reading grob properties in the
> engraver stage is a bad idea.

The engraver created the grob in question itself, announced it itself
(so all acknowledgers have been called) and actually keeps a pointer
to that grob in itself.

It basically _owns_ that grob.

> Something to the effect
> of: \override Staff.VerticalAxisGroup #'remove-empty = #(lambda
> (grob) (iterate-through-all-elements-and-get-Y-extent grob)) would
> do the trick.

What would happen?

> Granted, this would also be bad after the engraver stage, but worse
> in the engraver stage.

How so?

[Keith, 2013-01-26 06:04:44]

> On 25 janv. 2013, at 10:29, mailto:dak@gnu.org wrote:
> 
> > On 2013/01/25 08:05:28, Keith wrote:
> >> Please demonstrate a sneaky callback messing things up.
> > 
> Something to the effect of:
> 
> \override Staff.VerticalAxisGroup #'remove-empty = #(lambda (grob)
> (iterate-through-all-elements-and-get-Y-extent grob))
> 
> would do the trick.  

That doesn't help; that just gives me
"Unbound variable: iterate-through-all-elements-and-get-Y-extent"

I vaguely understand the principle that you want to stick to, but if I could
create an example myself, to demonstrate why the principle is valuable, I would
not have needed to ask you to demonstrate.

The comment has the effect of saying: "Maintenance programmers, stay away; Here
be dragons."