Re: Issue 3641: Keep only one Axis_group_engraver active (issue 20500043)

[dak]

https://codereview.appspot.com/20500043/diff/20001/lily/vertical-align-engraver.cc
File lily/vertical-align-engraver.cc (right):

https://codereview.appspot.com/20500043/diff/20001/lily/vertical-align-engraver.cc#newcode99
lily/vertical-align-engraver.cc:99:
On 2013/12/16 18:05:17, Keith wrote:
> The contexts with a Vertical_align_engraver, StaffGroup and such,
would not
> normally have the Axis_group_engraver that sets hasAxisGroup.

> If you add the Axis_group_engraver to a StaffGroup, setting the
contents of that
> StaffGroup as if all on one Staff, then I see how you might not want
> Vertical_align_engraver deciding whether to draw braces and creating
> StaffGroupers with spacing parameters that do not make sense.

> This removal of the Vertical_align_engraver looks like a convenience
feature,
> just for cases where you add Axis_group_engraver to an unusual
context.

> Usually, though, when users move an engraver from one context to
another, we add
> and remove individual engravers explicitly.
>   \new StaffGroup \with {\consists Axis_group_engraver } <<
>     \new Staff \with {\remove Axis_group_engraver }
>     \new Staff \with {\remove Axis_group_engraver } >>
> so it is good to restrict automatic removals to cases that are certain
errors,
> and to print the warning.

I don't actually understand that comment, nor do I understand whether
it is necessary to take any action on it.

The check and warning were here because this case was another one that
could cause infinite recursion akin to the one the first patch cured.
I think the use case of not messing with engravers at all and, say,
accepting a NoteNames context into a Staff (apparently typical for
Jazz sheets) would be easy and intuitive.

You would not at first try guess that this requires juggling with
engravers.  So I thought it better to just silently take the axis from
the topmost context.

For the Vertical_align_engraver, I did not see an obvious use case, so
I created a warning when shutting down the problematic operations.

I don't think I had much of a clue of what this is supposed to do,
rather the code definitely showed under which circumstances it would
explode.

If there is a good use case triggering the warning, one might remove
it.  I just did not see one.

https://codereview.appspot.com/20500043/