Moves tuplet numbers outside staff with their brackets.

Moves tuplet numbers outside staff with their brackets.

[MikeSol, 2011-06-27 17:38:04]

Useful for stuff like:


\version "2.15"
\relative c' {
\override TupletBracket #'outside-staff-priority = #1
\times 2/3 { c2^.^>^|^-^\espressivo c c }  
}

[Neil Puttock, 2011-06-27 21:56:00]

On 2011/06/27 17:38:04, MikeSol wrote:
> Useful for stuff like:
> 
> 
> \version "2.15"
> \relative c' {
> \override TupletBracket #'outside-staff-priority = #1
> \times 2/3 { c2^.^>^|^-^\espressivo c c }  
> }

This doesn't look too good:

\relative c' {  
  \override TupletBracket #'outside-staff-priority = #1
  \times 2/3 { c2 c c }
}

-> needlessly translated away from stave

[MikeSol, 2011-06-28 07:40:49]

On 2011/06/27 21:56:00, Neil Puttock wrote:

> This doesn't look too good:
> 
> \relative c' {  
>   \override TupletBracket #'outside-staff-priority = #1
>   \times 2/3 { c2 c c }
> }
> 
> -> needlessly translated away from stave

Fixed in the newest patchset.  Thanks for catching that!

[hanwenn, 2011-07-01 15:02:58]

This patch doesnt make sense to me.  The TupletBracket grob is already linked to
the TupletNumber.  The correct way to fix this is to make sure the number lets
the bracket decide its position.

http://codereview.appspot.com/4639075/diff/4001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/4639075/diff/4001/lily/axis-group-interface.cc#...
lily/axis-group-interface.cc:199: bool outside_staff_rider = unsmob_grob
(g->get_object ("outside-staff-carrier")) ? true : false;
what is a 'rider' ?  Are you sure we need another property for this?

http://codereview.appspot.com/4639075/diff/4001/lily/axis-group-interface.cc#...
lily/axis-group-interface.cc:262: mid_line_heights[j].unite (
code dup. please fix

http://codereview.appspot.com/4639075/diff/4001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/4639075/diff/4001/scm/define-grob-properties.sc...
scm/define-grob-properties.scm:1033: outside the staff.")
how is this different from side-support-elements ?

the definition is unclear as well- what does it mean to "carry" ?

[mike_apollinemike.com, 2011-07-01 15:44:15]

On Jul 1, 2011, at 5:02 PM, hanwenn@gmail.com wrote:

> This patch doesnt make sense to me.  The TupletBracket grob is already
> linked to the TupletNumber.  The correct way to fix this is to make sure
> the number lets the bracket decide its position.
> 
> 

The problem with this is that, in the axis-group-interface, the TupletNumber
either:

(1) Does not have an outside staff priority and therefore is included in the
staff's skyline, which pushes the bracket too high.
(2) Does have an outside staff priority, in which case it may push other grobs
too high above it (and it would have to be artificially re-placed in the correct
spot with respect to the TupletBracket).


> http://codereview.appspot.com/4639075/diff/4001/lily/axis-group-interface.cc
> File lily/axis-group-interface.cc (right):
> 
>
http://codereview.appspot.com/4639075/diff/4001/lily/axis-group-interface.cc#...
> lily/axis-group-interface.cc:199: bool outside_staff_rider = unsmob_grob
> (g->get_object ("outside-staff-carrier")) ? true : false;
> what is a 'rider' ?  Are you sure we need another property for this?
> 

A grob that rides a carrier grob outside the staff.

>
http://codereview.appspot.com/4639075/diff/4001/lily/axis-group-interface.cc#...
> lily/axis-group-interface.cc:262: mid_line_heights[j].unite (
> code dup. please fix
> 

I agree that it's a code dup, but it was a code dup before with
begin-line-heights and mid-line-heights (which could have been rolled into a
drul aray), just a smaller, two line code dup.  The present patch just
exacerbates the code-dupitude.  Perhaps in a separate patch a Drul_array can be
created for begin and mid so that a do / while loop can take care of a lot of
this function?

> http://codereview.appspot.com/4639075/diff/4001/scm/define-grob-properties.scm
> File scm/define-grob-properties.scm (right):
> 
>
http://codereview.appspot.com/4639075/diff/4001/scm/define-grob-properties.sc...
> scm/define-grob-properties.scm:1033: outside the staff.")
> how is this different from side-support-elements ?
> 
> the definition is unclear as well- what does it mean to "carry" ?
> 

As I understand it, side support elements go to a side (left right up down) of a
grob.  The issue with the TupletNumber element is that:

(a) It goes on the inside of that which would side support it.
(b) The side-position-interface only helps grobs position themselves with
respect to a parent.  It does not subsume the grob into the other grob's
outside-staffitude.  The issue here is that we need to eliminate TupletNumber
from the skyline calculation of the staff so that it's height does not push up
all outside-staff grobs.

Cheers,
MS

[hanwenn, 2011-07-01 16:09:35]

On Fri, Jul 1, 2011 at 12:44 PM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> On Jul 1, 2011, at 5:02 PM, hanwenn@gmail.com wrote:
>
>> This patch doesnt make sense to me.  The TupletBracket grob is already
>> linked to the TupletNumber.  The correct way to fix this is to make sure
>> the number lets the bracket decide its position.
>>
>>
>
> The problem with this is that, in the axis-group-interface, the TupletNumber
either:
>
> (1) Does not have an outside staff priority and therefore is included in the
staff's skyline, which pushes the bracket too high.
> (2) Does have an outside staff priority, in which case it may push other grobs
too high above it (and it would have to be artificially re-placed in the correct
spot with respect to the TupletBracket).

maybe I misunderstand the problem.  Are you fixing the slope of the
bracket (should be sloped), or the position of the number (not
centered on the bracket?)


-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[mike_apollinemike.com, 2011-07-01 16:13:00]

On Jul 1, 2011, at 6:09 PM, Han-Wen Nienhuys wrote:

> On Fri, Jul 1, 2011 at 12:44 PM, mike@apollinemike.com
> <mike@apollinemike.com> wrote:
>> On Jul 1, 2011, at 5:02 PM, hanwenn@gmail.com wrote:
>> 
>>> This patch doesnt make sense to me.  The TupletBracket grob is already
>>> linked to the TupletNumber.  The correct way to fix this is to make sure
>>> the number lets the bracket decide its position.
>>> 
>>> 
>> 
>> The problem with this is that, in the axis-group-interface, the TupletNumber
either:
>> 
>> (1) Does not have an outside staff priority and therefore is included in the
staff's skyline, which pushes the bracket too high.
>> (2) Does have an outside staff priority, in which case it may push other
grobs too high above it (and it would have to be artificially re-placed in the
correct spot with respect to the TupletBracket).
> 
> maybe I misunderstand the problem.  Are you fixing the slope of the
> bracket (should be sloped), or the position of the number (not
> centered on the bracket?)

The position of the number.

Cheers,
MS

[MikeSol, 2011-07-13 05:47:47]

On 2011/07/01 16:13:00, mike_apollinemike.com wrote:
> On Jul 1, 2011, at 6:09 PM, Han-Wen Nienhuys wrote:
> 
> > maybe I misunderstand the problem.  Are you fixing the slope of the
> > bracket (should be sloped), or the position of the number (not
> > centered on the bracket?)
> 
> The position of the number.
> 
> Cheers,
> MS

Just touching base to see where people are with the review of this patch.  It
allows for TupetNumber grobs to ride along with their TupetBrackets if the
TupletBrackets receive an outside staff priority.

Cheers,
MS

[hanwenn, 2011-07-13 14:13:54]

On Fri, Jul 1, 2011 at 1:09 PM, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
>> The problem with this is that, in the axis-group-interface, the TupletNumber
either:
>>
>> (1) Does not have an outside staff priority and therefore is included in the
staff's skyline, which pushes the bracket too high.
>> (2) Does have an outside staff priority, in which case it may push other
grobs too high above it (and it would have to be artificially re-placed in the
correct spot with respect to the TupletBracket).

Wouldnt it be much easier for the tuplet number's extents  to be
ignored for skyline purposes if there already is an associated tuplet
bracket?

Practically speaking, the tupletnumber adapts its position to the
bracket, so it is best for the implementation to also do this.

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[mike_apollinemike.com, 2011-07-13 14:27:19]

On Jul 13, 2011, at 4:13 PM, Han-Wen Nienhuys wrote:

> On Fri, Jul 1, 2011 at 1:09 PM, Han-Wen Nienhuys <hanwenn@gmail.com> wrote:
>>> The problem with this is that, in the axis-group-interface, the TupletNumber
either:
>>> 
>>> (1) Does not have an outside staff priority and therefore is included in the
staff's skyline, which pushes the bracket too high.
>>> (2) Does have an outside staff priority, in which case it may push other
grobs too high above it (and it would have to be artificially re-placed in the
correct spot with respect to the TupletBracket).
> 
> Wouldnt it be much easier for the tuplet number's extents  to be
> ignored for skyline purposes if there already is an associated tuplet
> bracket?
> 

That's what the new line 250 of axis-group-interface.cc in the patch does: it
makes sure that the rider grob is not counted in the skyline (I think).
The tuplet number sticks out of the tuplet bracket, so it makes sense for its
extent to be combined with that of its carrier (lines 244 and 255 of
axis-group-interface.cc in the patch).

> Practically speaking, the tupletnumber adapts its position to the
> bracket, so it is best for the implementation to also do this.
> 

The idea of the rider grob in this implementation is a generic concept that
would allow for certain grobs (i.e. TupletNumbers) to ride their carriers (i.e.
TupletBrackets) outside of the staff and then subsequently be counted as part of
their carrier's extent instead of as part of the staff extent (or as another
outside-staff grob).

Cheers,
MS

[hanwenn, 2011-07-13 14:42:39]

On Wed, Jul 13, 2011 at 11:27 AM, mike@apollinemike.com
<mike@apollinemike.com> wrote:

>> Wouldnt it be much easier for the tuplet number's extents  to be
>> ignored for skyline purposes if there already is an associated tuplet
>> bracket?
>>
>
> That's what the new line 250 of axis-group-interface.cc in the patch does: it
makes sure that the rider grob is not counted in the skyline (I think).
> The tuplet number sticks out of the tuplet bracket, so it makes sense for its
extent to be combined with that of its carrier (lines 244 and 255 of
axis-group-interface.cc in the patch).
>
>> Practically speaking, the tupletnumber adapts its position to the
>> bracket, so it is best for the implementation to also do this.
>>
>
> The idea of the rider grob in this implementation is a generic concept that
would allow for certain grobs (i.e. TupletNumbers) to ride their carriers (i.e.
TupletBrackets) outside of the staff and then subsequently be counted as part of
their carrier's extent instead of as part of the staff extent (or as another
outside-staff grob).

Can you think of a way to make the impact of this to axis-group code
minimal?  I think the only thing needed is that the axis-group doesnt
know about the tuplet number.  You could either not add it to the
axis-group in the first place, or alternatively, you could remove the
number from the 'elements list in axis group once you know it is
irrelevant.

The number should be parented by the bracket so you get the
translations for free.

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[mike_apollinemike.com, 2011-07-13 20:12:26]

On Jul 13, 2011, at 4:42 PM, Han-Wen Nienhuys wrote:

> On Wed, Jul 13, 2011 at 11:27 AM, mike@apollinemike.com
> <mike@apollinemike.com> wrote:
> 
>>> Wouldnt it be much easier for the tuplet number's extents  to be
>>> ignored for skyline purposes if there already is an associated tuplet
>>> bracket?
>>> 
>> 
>> That's what the new line 250 of axis-group-interface.cc in the patch does: it
makes sure that the rider grob is not counted in the skyline (I think).
>> The tuplet number sticks out of the tuplet bracket, so it makes sense for its
extent to be combined with that of its carrier (lines 244 and 255 of
axis-group-interface.cc in the patch).
>> 
>>> Practically speaking, the tupletnumber adapts its position to the
>>> bracket, so it is best for the implementation to also do this.
>>> 
>> 
>> The idea of the rider grob in this implementation is a generic concept that
would allow for certain grobs (i.e. TupletNumbers) to ride their carriers (i.e.
TupletBrackets) outside of the staff and then subsequently be counted as part of
their carrier's extent instead of as part of the staff extent (or as another
outside-staff grob).
> 
> Can you think of a way to make the impact of this to axis-group code
> minimal?  I think the only thing needed is that the axis-group doesnt
> know about the tuplet number.  You could either not add it to the
> axis-group in the first place, or alternatively, you could remove the
> number from the 'elements list in axis group once you know it is
> irrelevant.
> 
> The number should be parented by the bracket so you get the
> translations for free.
> 

Most of the current patch does the minimal amount of work to get the result. 
Note that this does not mean that this is the ideal way to do it - it is just
the only way I know how given my understanding of the code.

Added line 199: identifies if a grob should be ignored (is it an
outside-staff-rider?)

Added line 240-250 and 255-264: Admittedly code-dupious, so these really do the
same thing.  I am trying to follow the
patches-as-tiny-and-self-contained-as-possible policy as strictly as possible,
especially as I am a cardinal offender in other patches under review.  These
lines do two things: (1) Add the extent of the tuplet number to the tuplet
bracket (which I do here instead of in the Y-extent override because it seems
that it should be temporary); and (2) ignores all rider grobs that have already
been added into their carriers' extent.

Added lines 653-655, 674-675: Move the rider relative to its carrier.  This is
the spot where I may be able to get some of the code outside of
axis-group-interface.cc (thus the "most" in my first sentence).  It seems that
the best place may be side-position-interface.cc, but the issue in this file is
that, as I see it, things can be positioned with respect to their parents on the
left/right/above/below but not in the middle of a grob's control points, which
is currently where the tuplet number is placed.  Do you think it'd be a good
idea to modify side-position-interface to do this?  Alternatively, another way
to get at this may be in the axis-group-interface to change the value of a
grob's "control-points" property if it has one such that these points are
shifted by the same amount as the grob.  Does this seem like a good idea?  As
always, I'll do whatever is most lily-pond-esque.

Lemme know if you see in my explanation anything that seems frivolous/too much,
and thanks for all of your feedback!

Cheers,
MS

[hanwenn, 2011-07-13 20:48:06]

On Wed, Jul 13, 2011 at 5:12 PM, mike@apollinemike.com
<mike@apollinemike.com> wrote:
> On Jul 13, 2011, at 4:42 PM, Han-Wen Nienhuys wrote:
>
>> On Wed, Jul 13, 2011 at 11:27 AM, mike@apollinemike.com
>> <mike@apollinemike.com> wrote:
>>
>>>> Wouldnt it be much easier for the tuplet number's extents  to be
>>>> ignored for skyline purposes if there already is an associated tuplet
>>>> bracket?
>>>>
>>>
>>> That's what the new line 250 of axis-group-interface.cc in the patch does:
it makes sure that the rider grob is not counted in the skyline (I think).
>>> The tuplet number sticks out of the tuplet bracket, so it makes sense for
its extent to be combined with that of its carrier (lines 244 and 255 of
axis-group-interface.cc in the patch).
>>>
>>>> Practically speaking, the tupletnumber adapts its position to the
>>>> bracket, so it is best for the implementation to also do this.
>>>>
>>>
>>> The idea of the rider grob in this implementation is a generic concept that
would allow for certain grobs (i.e. TupletNumbers) to ride their carriers (i.e.
TupletBrackets) outside of the staff and then subsequently be counted as part of
their carrier's extent instead of as part of the staff extent (or as another
outside-staff grob).
>>
>> Can you think of a way to make the impact of this to axis-group code
>> minimal?  I think the only thing needed is that the axis-group doesnt
>> know about the tuplet number.  You could either not add it to the
>> axis-group in the first place, or alternatively, you could remove the
>> number from the 'elements list in axis group once you know it is
>> irrelevant.
>>
>> The number should be parented by the bracket so you get the
>> translations for free.
>>
>
> Most of the current patch does the minimal amount of work to get the result.
 Note that this does not mean that this is the ideal way to do it - it is just
the only way I know how given my understanding of the code.

It's not about the minimal amount of work, it's about having minimal
dependencies between parts; it were much better if this feature did
not have to modify the axis-group code at all.  Can you try to do it
without having axis-group know about a 'rider/carrier' concept?

> Added line 199: identifies if a grob should be ignored (is it an
outside-staff-rider?)

if you don't add the grob to the list to begin with, you don't have need this.


> Added line 240-250 and 255-264: Admittedly code-dupious, so these really do
the same thing.  I am trying to follow the
patches-as-tiny-and-self-contained-as-possible policy as strictly as possible,
especially as I am a cardinal offender in other patches under review.  These
lines do two things: (1) Add the extent of the tuplet number to the tuplet
bracket (which I do here instead of in the Y-extent override because it seems
that it should be temporary); and (2) ignores all rider grobs that have already
been added into their carriers' extent.

I would leave handling the vertical extent of the number for a next change.

> Added lines 653-655, 674-675: Move the rider relative to its carrier.  This is
the spot where I may be able to get some of the code outside of
axis-group-interface.cc

+		  for (vsize j = 0; j < riders.size (); j++)
+		    riders[j]->translate_axis (dir*dist, Y_AXIS);

if the rider has the carrier as it's Y parent, moving the carrier
already accomplishes this.


> Lemme know if you see in my explanation anything that seems frivolous/too
much, and thanks for all of your feedback!
>
> Cheers,
> MS



-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[mike_apollinemike.com, 2011-07-14 12:27:31]

On Jul 13, 2011, at 10:48 PM, Han-Wen Nienhuys wrote:

> On Wed, Jul 13, 2011 at 5:12 PM, mike@apollinemike.com
> <mike@apollinemike.com> wrote:
>> On Jul 13, 2011, at 4:42 PM, Han-Wen Nienhuys wrote:
>> 
>>> On Wed, Jul 13, 2011 at 11:27 AM, mike@apollinemike.com
>>> <mike@apollinemike.com> wrote:
>>> 
>>>>> Wouldnt it be much easier for the tuplet number's extents  to be
>>>>> ignored for skyline purposes if there already is an associated tuplet
>>>>> bracket?
>>>>> 
>>>> 
>>>> That's what the new line 250 of axis-group-interface.cc in the patch does:
it makes sure that the rider grob is not counted in the skyline (I think).
>>>> The tuplet number sticks out of the tuplet bracket, so it makes sense for
its extent to be combined with that of its carrier (lines 244 and 255 of
axis-group-interface.cc in the patch).
>>>> 
>>>>> Practically speaking, the tupletnumber adapts its position to the
>>>>> bracket, so it is best for the implementation to also do this.
>>>>> 
>>>> 
>>>> The idea of the rider grob in this implementation is a generic concept that
would allow for certain grobs (i.e. TupletNumbers) to ride their carriers (i.e.
TupletBrackets) outside of the staff and then subsequently be counted as part of
their carrier's extent instead of as part of the staff extent (or as another
outside-staff grob).
>>> 
>>> Can you think of a way to make the impact of this to axis-group code
>>> minimal?  I think the only thing needed is that the axis-group doesnt
>>> know about the tuplet number.  You could either not add it to the
>>> axis-group in the first place, or alternatively, you could remove the
>>> number from the 'elements list in axis group once you know it is
>>> irrelevant.
>>> 
>>> The number should be parented by the bracket so you get the
>>> translations for free.
>>> 
>> 
>> Most of the current patch does the minimal amount of work to get the result. 
Note that this does not mean that this is the ideal way to do it - it is just
the only way I know how given my understanding of the code.
> 
> It's not about the minimal amount of work, it's about having minimal
> dependencies between parts; it were much better if this feature did
> not have to modify the axis-group code at all.  Can you try to do it
> without having axis-group know about a 'rider/carrier' concept?
> 
>> Added line 199: identifies if a grob should be ignored (is it an
outside-staff-rider?)
> 
> if you don't add the grob to the list to begin with, you don't have need this.
> 
> 
>> Added line 240-250 and 255-264: Admittedly code-dupious, so these really do
the same thing.  I am trying to follow the
patches-as-tiny-and-self-contained-as-possible policy as strictly as possible,
especially as I am a cardinal offender in other patches under review.  These
lines do two things: (1) Add the extent of the tuplet number to the tuplet
bracket (which I do here instead of in the Y-extent override because it seems
that it should be temporary); and (2) ignores all rider grobs that have already
been added into their carriers' extent.
> 
> I would leave handling the vertical extent of the number for a next change.
> 
>> Added lines 653-655, 674-675: Move the rider relative to its carrier.  This
is the spot where I may be able to get some of the code outside of
axis-group-interface.cc
> 
> +		  for (vsize j = 0; j < riders.size (); j++)
> +		    riders[j]->translate_axis (dir*dist, Y_AXIS);
> 
> if the rider has the carrier as it's Y parent, moving the carrier
> already accomplishes this.
> 

I think the newest patch set should do the trick - you're right about not adding
the grob to the list to begin with, and I went down this route instead.

Cheers,
MS

[Carl, 2011-07-14 12:47:12]

I like the fact that there's no more "rider" concept.

The only architectural question I have is whether has_outside_staff_parent
should be in Axis_group_interface or Grob.

Otherwise, LGTM.

Thanks,

Carl

http://codereview.appspot.com/4639075/diff/18001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/4639075/diff/18001/lily/axis-group-interface.cc...
lily/axis-group-interface.cc:670: Axis_group_interface::has_outside_staff_parent
(Grob *me)
Why not make this part of the Grob class instead of the Axis_group_interface? 
It seems to be a Grob operation, and so belongs there.

But I'm treading on thin ice here; my comments could be entirely wrong.

http://codereview.appspot.com/4639075/diff/18001/lily/axis-group-interface.cc...
lily/axis-group-interface.cc:675: || has_outside_staff_parent (me->get_parent
(Y_AXIS)))
Do you want one of these to be X_AXIS?  I can't see any reason to duplicate the
Y_AXIS calls.

[mike_apollinemike.com, 2011-07-14 13:19:31]

On Jul 14, 2011, at 2:47 PM, Carl.D.Sorensen@gmail.com wrote:

> 
>
http://codereview.appspot.com/4639075/diff/18001/lily/axis-group-interface.cc...
> lily/axis-group-interface.cc:670:
> Axis_group_interface::has_outside_staff_parent (Grob *me)
> Why not make this part of the Grob class instead of the
> Axis_group_interface?  It seems to be a Grob operation, and so belongs
> there.
> 
> But I'm treading on thin ice here; my comments could be entirely wrong.
> 

I'm not sure either - you bring up a good question.  We'll see what other people
have to say.

>
http://codereview.appspot.com/4639075/diff/18001/lily/axis-group-interface.cc...
> lily/axis-group-interface.cc:675: || has_outside_staff_parent
> (me->get_parent (Y_AXIS)))
> Do you want one of these to be X_AXIS?  I can't see any reason to
> duplicate the Y_AXIS calls.


Oops...fixing and reposting the patch now.

Cheers,
MS

[hanwenn, 2011-07-14 13:51:14]

much better.  I bet there will skeletons coming out the regtest closet now, but
let's hope that we can fix things up if necessary

http://codereview.appspot.com/4639075/diff/23001/lily/axis-group-interface.cc
File lily/axis-group-interface.cc (right):

http://codereview.appspot.com/4639075/diff/23001/lily/axis-group-interface.cc...
lily/axis-group-interface.cc:673: ? (scm_is_number (me->get_property
("outside-staff-priority"))
why the null check? - I'd move the null check to the caller.

http://codereview.appspot.com/4639075/diff/23001/lily/axis-group-interface.cc...
lily/axis-group-interface.cc:675: || has_outside_staff_parent (me->get_parent
(Y_AXIS)))
shouldnt this only trigger for Y axis?

[mike_apollinemike.com, 2011-07-14 20:01:41]

On Jul 14, 2011, at 3:51 PM, hanwenn@gmail.com wrote:

> much better.  I bet there will skeletons coming out the regtest closet
> now, but let's hope that we can fix things up if necessary
> 
> 
> http://codereview.appspot.com/4639075/diff/23001/lily/axis-group-interface.cc
> File lily/axis-group-interface.cc (right):
> 
>
http://codereview.appspot.com/4639075/diff/23001/lily/axis-group-interface.cc...
> lily/axis-group-interface.cc:673: ? (scm_is_number (me->get_property
> ("outside-staff-priority"))
> why the null check? - I'd move the null check to the caller.
> 

Do you mean:

bool
Axis_group_interface::has_outside_staff_parent (Grob *me)
{
  Grob *parent = me->get_parent (Y_AXIS);
  return (scm_is_number (me->get_property ("outside-staff-priority"))
               || (parent
                   ? has_outside_staff_parent (parent)
                    : false))
}

It's true that this avoids an extra function call, but it is decidedly less sexy
as a two-line function.  Doable, though - I don't know much about CS & the value
of less function calls versus less declarations of intermediary variables.

I'll run the regtests tonight or tomorrow and report back.

Cheers,
MS

[Neil Puttock, 2011-07-14 20:23:03]

On 14 July 2011 21:01, mike@apollinemike.com <mike@apollinemike.com> wrote:

> I'll run the regtests tonight or tomorrow and report back.

I've just tried this and I think you've got an infinite loop somewhere
(I had to kill the process and reboot).

Edit: it's from tie-pitched-trill.ly; segfaults due to stack overflow.

Cheers,
Neil

[MikeSol, 2011-07-15 16:27:46]

On 2011/07/14 20:23:03, Neil Puttock wrote:
> On 14 July 2011 21:01, mailto:mike@apollinemike.com
<mailto:mike@apollinemike.com> wrote:
> 
> > I'll run the regtests tonight or tomorrow and report back.
> 
> I've just tried this and I think you've got an infinite loop somewhere
> (I had to kill the process and reboot).
> 
> Edit: it's from tie-pitched-trill.ly; segfaults due to stack overflow.
> 
> Cheers,
> Neil

I just ran regtests with this new patch and that fixes it.
There must be some type of loop in the grobs' family trees...

Cheers,
MS

[MikeSol, 2011-07-28 08:29:49]

Pushed as 72210831b0366cb7c1f7009ba99a56538d68a7e5.

Cheers,
MS