Fix segfault with ambitus and ligature (Issue 1715)

Fix segfault with ambitus and ligature (Issue 1715)
Check for an event-cause before acknowledging note_head.  This
prevents trying to make a ligature from the AmbitusNoteHeads.

Also includes regression test.

[Carl, 2011-07-03 21:13:22]

Here is a proposed patch for fixing issue 1715.  It works by checking for
event-cause before acknowledging a notehead, thus ignoring AmbitusNoteHeads

[Neil Puttock, 2011-07-03 21:25:58]

Hi Carl,

This LGTM, though the regtest could do with a bit of tidying.

A more correct approach would be to filter interfaces since the
Ligature_engraver also potentially acknowledges PitchTrillGroup.  It's probably
not worth the bother though: it does have an event-cause and is ignored by the
ligature engravers; furthermore, it's unlikely \pitchedTrill would be used in
conjunction with such ligatures.

Cheers,
Neil

http://codereview.appspot.com/4667055/diff/2001/lily/ligature-engraver.cc
File lily/ligature-engraver.cc (right):

http://codereview.appspot.com/4667055/diff/2001/lily/ligature-engraver.cc#new...
lily/ligature-engraver.cc:199: if (info.event_cause ()) // Ignore
AmbitusNoteHead which has no event_cause
if (info.event_cause ()
    && ligature_)

[Carl, 2011-07-03 21:32:45]

On 2011/07/03 21:25:58, Neil Puttock wrote:
> Hi Carl,
> 
> This LGTM, though the regtest could do with a bit of tidying.

Yeah, I thought of that, but I was too lazy.  I just took what was in the
tracker.

OK, I'll trim it up some more.

> 
> A more correct approach would be to filter interfaces since the
> Ligature_engraver also potentially acknowledges PitchTrillGroup.  

What do you mean "filter interfaces"?  As long as I'm working on it I might as
well get it right.

Thanks,

Carl

[Neil Puttock, 2011-07-03 21:39:16]

On 2011/07/03 21:32:45, Carl wrote:
> On 2011/07/03 21:25:58, Neil Puttock wrote:
> > Hi Carl,
> > 
> > This LGTM, though the regtest could do with a bit of tidying.
> 
> Yeah, I thought of that, but I was too lazy.  I just took what was in the
> tracker.
> 
> OK, I'll trim it up some more.
> 
> > 
> > A more correct approach would be to filter interfaces since the
> > Ligature_engraver also potentially acknowledges PitchTrillGroup.  
> 
> What do you mean "filter interfaces"?  As long as I'm working on it I might as
> well get it right.

Use info.grob ()->internal_has_interface ("...") to filter out the
AmbitusNoteHead and PitchTrillGroup (via ambitus-interface and
axis-group-interface).

[Neil Puttock, 2011-07-03 21:39:49]

On 2011/07/03 21:39:16, Neil Puttock wrote:
> On 2011/07/03 21:32:45, Carl wrote:
> > On 2011/07/03 21:25:58, Neil Puttock wrote:
> > > Hi Carl,
> > > 
> > > This LGTM, though the regtest could do with a bit of tidying.
> > 
> > Yeah, I thought of that, but I was too lazy.  I just took what was in the
> > tracker.
> > 
> > OK, I'll trim it up some more.
> > 
> > > 
> > > A more correct approach would be to filter interfaces since the
> > > Ligature_engraver also potentially acknowledges PitchTrillGroup.  
> > 
> > What do you mean "filter interfaces"?  As long as I'm working on it I might
as
> > well get it right.
> 
> Use info.grob ()->internal_has_interface ("...") to filter out the
> AmbitusNoteHead and PitchTrillGroup (via ambitus-interface and
> axis-group-interface).

Oops, TrillPitchGroup.

[pkx166h, 2011-07-04 00:13:02]

Passes make and reg test

James

[Carl, 2011-07-04 01:54:40]

Instead of filtering out bad events, I chose to filter in only events with
ligature interfaces.

I'm not sure my line-breaking is correct on the internal_has_interface () calls,
but we'll soon have fixcc.py working, right? :)  If it isn't right, please help
me know how to fix it.

I also trimmed the regtest.

Thanks,

Carl

http://codereview.appspot.com/4667055/diff/2001/lily/ligature-engraver.cc
File lily/ligature-engraver.cc (right):

http://codereview.appspot.com/4667055/diff/2001/lily/ligature-engraver.cc#new...
lily/ligature-engraver.cc:199: if (info.event_cause ()) // Ignore
AmbitusNoteHead which has no event_cause
On 2011/07/03 21:25:58, Neil Puttock wrote:
> if (info.event_cause ()
>     && ligature_)

Done.

[Neil Puttock, 2011-07-04 20:30:10]

On 2011/07/04 01:54:40, Carl wrote:
> Instead of filtering out bad events, I chose to filter in only events with
> ligature interfaces.

That's a lot of internal_has_interface () calls. :)  You might as well create a
new interface just for NoteHead (e.g., ligature-head-interface).

Cheers,
Neil

[Carl, 2011-07-04 20:57:52]

On 2011/07/04 20:30:10, Neil Puttock wrote:
> On 2011/07/04 01:54:40, Carl wrote:
> > Instead of filtering out bad events, I chose to filter in only events with
> > ligature interfaces.
> 
> That's a lot of internal_has_interface () calls. :)  

I wondered about that.  But I think the first one that is true will stop the
evaluation, so at the present only one would be called.

The original way you suggested was to have two internal_has_interface () calls;
this one only adds one.


> You might as well create a
> new interface just for NoteHead (e.g., ligature-head-interface).
> 

We already have the unused ligature-interface.  What if I just added
ligature-interface to NoteHead?

Thanks,

Carl

[Neil Puttock, 2011-07-04 21:22:58]

On 4 July 2011 21:57,  <Carl.D.Sorensen@gmail.com> wrote:

> The original way you suggested was to have two internal_has_interface ()
> calls; this one only adds one.

But for the invalid heads, all three calls would be made (and of
course, for a NoteHead itself only the first interface check will ever
happen, so the other calls are redundant).

> We already have the unused ligature-interface.  What if I just added
> ligature-interface to NoteHead?

Sounds fine to me.

Cheers,
Neil

[Carl, 2011-07-04 22:31:18]

On 2011/07/04 21:22:58, Neil Puttock wrote:
> On 4 July 2011 21:57,  <mailto:Carl.D.Sorensen@gmail.com> wrote:
 
> > We already have the unused ligature-interface. &nbsp;What if I just added
> > ligature-interface to NoteHead?
> 
> Sounds fine to me.


OK, so I've added ligature-interface to NoteHead, and everything seems to work
now.

Thanks,
Carl

[Neil Puttock, 2011-07-05 19:58:38]

On 2011/07/04 22:31:18, Carl wrote:

> OK, so I've added ligature-interface to NoteHead, and everything seems to work
> now.

Erm, can I take back what I said yesterday? :)

This looks really weird now, since it appears we're acknowledging ligature
grobs.  I think ligature-head-interface would be less confusing for the
acknowledgers.

Cheers,
Neil

[Carl, 2011-07-06 17:49:40]

On 2011/07/05 19:58:38, Neil Puttock wrote:
>> Erm, can I take back what I said yesterday? :)
> 
> This looks really weird now, since it appears we're acknowledging ligature
> grobs.  I think ligature-head-interface would be less confusing for the
> acknowledgers.

Not only did it look really weird, it was wrong, even though I don't know what
the code should be to prove the error, but I think it would be part of gregorian
engraving, where we create ligatures.

ligature_engraver *listens* to ligatures, and *acknowledges* ligature_heads.

I've fixed it properly now,.

[Neil Puttock, 2011-07-07 16:12:08]

LGTM, though the regtest is still a bit messy.

http://codereview.appspot.com/4667055/diff/14001/scm/define-grob-interfaces.scm
File scm/define-grob-interfaces.scm (right):

http://codereview.appspot.com/4667055/diff/14001/scm/define-grob-interfaces.s...
scm/define-grob-interfaces.scm:128: "A note-head that can become part of a
ligature."
note head

[Carl, 2011-07-07 22:22:17]

On 2011/07/07 16:12:08, Neil Puttock wrote:
> LGTM, though the regtest is still a bit messy.

What do you consider messy about the regtest?


> scm/define-grob-interfaces.scm:128: "A note-head that can become part of a
> ligature."
> note head

Done

Thanks,

Carl

[Colin Campbell, 2011-07-10 01:55:28]

This has had a 48-hour countdown, and can be pushed and closed, please.

Colin

[c_sorensen, 2011-07-10 02:14:15]

On 7/9/11 7:55 PM, "ColinPKCampbell@gmail.com" <ColinPKCampbell@gmail.com>
wrote:

> This has had a 48-hour countdown, and can be pushed and closed, please.

Issue 1715 issue marked fixed; Rietveld issue closed

Commit: 4fd0de625eba203265241fca20c504724013f534

Thanks,

Carl

[Neil Puttock, 2011-07-10 21:09:03]

On 7 July 2011 23:22,  <Carl.D.Sorensen@gmail.com> wrote:
> On 2011/07/07 16:12:08, Neil Puttock wrote:
>>
>> LGTM, though the regtest is still a bit messy.
>
> What do you consider messy about the regtest?

Just a few nitpicks:

+\score{

+ \context Staff="default" {

+ \context{

+ \consists Ambitus_engraver
+ \consists Mensural_ligature_engraver

Cheers,
Neil

[c_sorensen, 2011-07-10 22:34:52]

On 7/10/11 3:09 PM, "Neil Puttock" <n.puttock@gmail.com> wrote:

> On 7 July 2011 23:22,  <Carl.D.Sorensen@gmail.com> wrote:
>> On 2011/07/07 16:12:08, Neil Puttock wrote:
>>> 
>>> LGTM, though the regtest is still a bit messy.
>> 
>> What do you consider messy about the regtest?
> 
> Just a few nitpicks:
> 
> +\score{
> 
> + \context Staff="default" {
> 
> + \context{
> 
> + \consists Ambitus_engraver
> + \consists Mensural_ligature_engraver

Ahh, thanks.  Fixed now:

{
  \new Voice \with  {
    \consists Ambitus_engraver
    \consists Mensural_ligature_engraver
    } {
      \[ c'\longa c''\longa \]
    }
}

Thanks,

Carl

[Neil Puttock, 2011-07-10 22:41:20]

On 10 July 2011 23:34, Carl Sorensen <c_sorensen@byu.edu> wrote:

> Ahh, thanks.  Fixed now:
>
> {
>  \new Voice \with  {
>    \consists Ambitus_engraver
>    \consists Mensural_ligature_engraver

Heh, you didn't have to do this; I meant the missing quotation marks
around the engraver names. :)

(and now you've added a spurious sequential block ;)

Cheers,
Neil

[c_sorensen, 2011-07-10 22:50:07]

On 7/10/11 4:41 PM, "Neil Puttock" <n.puttock@gmail.com> wrote:

> On 10 July 2011 23:34, Carl Sorensen <c_sorensen@byu.edu> wrote:
> 
>> Ahh, thanks.  Fixed now:
>> 
>> {
>>  \new Voice \with  {
>>    \consists Ambitus_engraver
>>    \consists Mensural_ligature_engraver
> 
> Heh, you didn't have to do this; I meant the missing quotation marks
> around the engraver names. :)

Ahh -- oh, well.  The new version *is* shorter.
> 
> (and now you've added a spurious sequential block ;)

And removed it.....

Thanks,

Carl