Issue 3641: Keep only one Axis_group_engraver active

Issue 3641: Keep only one Axis_group_engraver active

The internal context property axisEngraver is used for tracking the
currently active Axis_group_engraver.  This supercedes the solution
attempted in issue 2990.


Also:

Revert "Issue 2990: \RemoveEmptyStaves in StaffGroup context crashes"

This reverts commit d40362adfc1c3b24f466b7570e58f92885751019.

[dak, 2013-10-31 23:36:16]

Simplify tests, make robust against collision with Vertical_align_engraver

[Keith, 2013-11-01 07:30:47]

Looks good.

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

https://codereview.appspot.com/20500043/diff/20001/lily/axis-group-engraver.c...
lily/axis-group-engraver.cc:135: the context.  */
Probably this comment no longer applies.

https://codereview.appspot.com/20500043/diff/20001/lily/axis-group-engraver.c...
lily/axis-group-engraver.cc:147: && staffline_->get_parent (Y_AXIS) == elts_[i])
Maybe this test is no longer needed.

[ckenneth721, 2013-11-10 13:08:11]

https://codereview.appspot.com/22120043/

[ckenneth721, 2013-11-10 13:09:16]

https://codereview.appspot.com/22120043/

[dak, 2013-11-10 14:10:38]

Man, "unpublished drafts" _again_.  I really am too stupid for that Rietveld
user interface.  At any rate, they seem a bit dated now.  Might make sense for
followup issues.

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

https://codereview.appspot.com/20500043/diff/20001/lily/axis-group-engraver.c...
lily/axis-group-engraver.cc:135: the context.  */
On 2013/11/01 07:30:48, Keith wrote:
> Probably this comment no longer applies.

I should think so.

https://codereview.appspot.com/20500043/diff/20001/lily/axis-group-engraver.c...
lily/axis-group-engraver.cc:147: && staffline_->get_parent (Y_AXIS) == elts_[i])
On 2013/11/01 07:30:48, Keith wrote:
> Maybe this test is no longer needed.

Well, suiciding the staffline_ never lead to any useful result as far as I know.
 It just avoided a complete crash in some cases.  It is probably still perfectly
feasible to trigger this condition by manually tampering with data (and the
hasAxisGroup flag).

I lean towards making this a fatal error now.  If we don't know how the
condition can come about without bad meddling and don't know how we are supposed
to deal with it in any manner making sense, aborting is probably a defensible
action.

[dak, 2013-11-10 14:18:05]

On 2013/11/10 13:09:16, ckenneth721 wrote:
> https://codereview.appspot.com/22120043/

This looks rather like your cat got your mouse.  If you have a problem figuring
out the user interface here, perhaps just write your comment to the
lilypond-devel list directly.

[Keith, 2013-12-16 18:05:16]

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

https://codereview.appspot.com/20500043/diff/20001/lily/vertical-align-engrav...
lily/vertical-align-engraver.cc:99: 
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.

[dak, 2013-12-16 18:16:54]

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

https://codereview.appspot.com/20500043/diff/20001/lily/vertical-align-engrav...
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.

[Keith, 2013-12-16 21:58:08]

On 2013/12/16 18:16:54, dak wrote:
>
https://codereview.appspot.com/20500043/diff/20001/lily/vertical-align-engrav...
> lily/vertical-align-engraver.cc:99: 
> On 2013/12/16 18:05:17, Keith wrote:
> > 
> > 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.
> 
> 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 might still be talking about the check against creating VerticalAxisGroups
inside other VerticalAxisGroups.  The check here for is Vertical_align_engravers
and these engravers do not seem to create anything dangerous.

So I do understand the reason for the additional check you added here.  It does
not do what you momentarily thought it should over on -user.  It looks fine to
me, though, as it should not cause any problem, so I am not suggesting any
action.