Set X-parent of TextScript to NoteColumn instead of PaperColumn

Set X-parent of TextScript to NoteColumn instead of PaperColumn

This makes TextScripts consistent with DynamicTexts and LyricTexts.  I'm also marking all TextScripts as non-cross-staff.

This is a follow-up to https://code.google.com/p/lilypond/issues/detail?id=2245 (http://codereview.appspot.com/108270044)

[janek, 2014-07-10 21:05:31]

don't segfault when first head is NULL

[dak, 2014-07-11 11:10:10]

https://codereview.appspot.com/106640043/diff/20001/lily/note-column.cc
File lily/note-column.cc (right):

https://codereview.appspot.com/106640043/diff/20001/lily/note-column.cc#newco...
lily/note-column.cc:169: if (head)
Why would this test become required when it wasn't before?  Doesn't this imply
that you are now calling this function prematurely?

https://codereview.appspot.com/106640043/diff/20001/lily/text-engraver.cc
File lily/text-engraver.cc (right):

https://codereview.appspot.com/106640043/diff/20001/lily/text-engraver.cc#new...
lily/text-engraver.cc:34: struct Script_tuple
Why?  You never exploit the connection between grob and event, so why would you
need to combine grobs and events in one array?

[janek, 2014-07-20 14:35:44]

Text_engraver: store events and grobs separately

[janek, 2014-07-20 14:42:23]

New patchset uploaded.

https://codereview.appspot.com/106640043/diff/20001/lily/note-column.cc
File lily/note-column.cc (right):

https://codereview.appspot.com/106640043/diff/20001/lily/note-column.cc#newco...
lily/note-column.cc:169: if (head)
On 2014/07/11 11:10:10, dak wrote:
> Why would this test become required when it wasn't before?  Doesn't this imply
> that you are now calling this function prematurely?

Good question; you may be right.  But i don't have any ideas why LilyPond would
call that function prematurely instead of waiting till all necessary information
is available.  In other words, why there were no such problems with
DynamicTexts?  If i had an example input on which the code without this check
fails, maybe i could figurre this out.

https://codereview.appspot.com/106640043/diff/20001/lily/text-engraver.cc
File lily/text-engraver.cc (right):

https://codereview.appspot.com/106640043/diff/20001/lily/text-engraver.cc#new...
lily/text-engraver.cc:34: struct Script_tuple
On 2014/07/11 11:10:10, dak wrote:
> Why?  You never exploit the connection between grob and event, so why would
you
> need to combine grobs and events in one array?

Because i didn't pay enough attention when i copied the code from
script_engraver.  Btw, script_engraver doesn't need such connection either,
shall i fix it in this issue or a separate one?

[janek, 2014-07-20 15:04:21]

Set TextScript's X-parent to rest if applicable.

[janek, 2014-07-20 15:09:38]

Fixed all issues identified by David.  Thanks for review!

Janek

https://codereview.appspot.com/106640043/diff/20001/lily/note-column.cc
File lily/note-column.cc (right):

https://codereview.appspot.com/106640043/diff/20001/lily/note-column.cc#newco...
lily/note-column.cc:169: if (head)
Okay, i found it - and it was quite simple: Textscripts can be attached to
rests, and rests don't have any NoteHeads :)  Newest patchset fixes that.

[janek, 2014-07-26 06:49:41]

Pushed as 

commit 2371d6ba3b62d4d6dc349ab50fa0d76eadfba044
Author: Janek Warchoł <lemniskata.bernoullego@gmail.com>
Date:   Sun Jun 29 18:25:04 2014 +0200

    Issue 4005: Set X-parent of TextScript to NoteColumn instead of PaperColumn
    
    This makes TextScripts consistent with DynamicTexts and LyricTexts.
    This is a follow-up to commit 59a842eba0f7ad78289a58a (Issue 2245).
    
    Setting TextScript.cross-staff property to #f is required to ensure
    that there are no collisions between TextScripts and cross-staff notes:
    
    \context PianoStaff <<
      \new Staff = "up" {
        b8[
        \change Staff="down"
        d'] ^"text"
      }
      \new Staff = "down" {
        \clef bass
        s4
      }
    >>
    
    (see also beam-cross-staff-auto-knee.ly)
    
    As far as I can see, we don't want TextScript.cross-staff to be true
    in any situation, because it would result in unwanted collisions.
    
    Why it worked before: cross-staff property in this example evaluated to #f,
    but only because of a bug in Script_interface::calc_cross_staff.  That
    function should have marked the TextScript as cross-staff if the stem
    of the note to which TextScript was attached was cross-staff, but it
    didn't work correctly because it expected the parent of the TextScript
    to be a NoteColumn, while it actually was a PaperColumn.  When I changed
    the parent to be a NoteColumn, the function started working correctly
    and marked the TextScript as cross-staff, so I had to change the default
    value of the property.


thanks!

[Keith, 2014-07-26 21:36:12]

On 2014/07/26 06:49:41, janek wrote:
>     
>     Setting TextScript.cross-staff property to #f is required to ensure
>     that there are no collisions between TextScripts and cross-staff notes:
     
>     (see also beam-cross-staff-auto-knee.ly)
>     
>     As far as I can see, we don't want TextScript.cross-staff to be true
>     in any situation, because it would result in unwanted collisions.
>     
The old setting for TextScript.cross-staff was put there to solve issue 1300.
Almost everything has changed since then in the code that spaces things around 
staves, but with TextScript.cross-staff = #f the text collides with the accent 
in that example.

The concept of a "cross-staff note" seems strange.  It appeared with the change 
for issue 2527 https://codereview.appspot.com/6827072#msg13

That patch made Scripts reference NoteColumns, rather than PaperColumns for 
their placement -- very similar to the change here -- but Scripts can be 
cross-staff without hitting note-heads.   Can you reconcile this patch and 
https://codereview.appspot.com/6827072 ?

[janek, 2014-07-27 16:44:22]

On 2014/07/26 21:36:12, Keith wrote:
> On 2014/07/26 06:49:41, janek wrote:
> >     
> >     Setting TextScript.cross-staff property to #f is required to ensure
> >     that there are no collisions between TextScripts and cross-staff notes:
>      
> >     (see also beam-cross-staff-auto-knee.ly)
> >     
> >     As far as I can see, we don't want TextScript.cross-staff to be true
> >     in any situation, because it would result in unwanted collisions.
> >     
> The old setting for TextScript.cross-staff was put there to solve issue 1300.
> Almost everything has changed since then in the code that spaces things around

> staves, but with TextScript.cross-staff = #f the text collides with the accent

> in that example.

Oops, i should have checked that myself!  I apologize.

> The concept of a "cross-staff note" seems strange.  It appeared with the
change 
> for issue 2527 https://codereview.appspot.com/6827072#msg13
> 
> That patch made Scripts reference NoteColumns, rather than PaperColumns for 
> their placement -- very similar to the change here -- but Scripts can be 
> cross-staff without hitting note-heads.   Can you reconcile this patch and 
> https://codereview.appspot.com/6827072 ?

Hmm.  Do i see correctly that the patch in
https://codereview.appspot.com/6827072 was then partially reverted with commit
7891600a5dd421c1f25776ea3b405c64f4f14752 ?

I'll investigate this, but since the weekend is about to end (and the next one
will already be busy for me), it will take me an unpredictable amount of time to
get this done.  Shall i revert commit 2371d6ba3b62d4d6dc349ab50fa0d76eadfba044
for now?

Thanks for spotting this.
Janek

[Keith, 2014-07-28 01:17:18]

On Sun, 27 Jul 2014 09:44:22 -0700, <janek.lilypond@gmail.com> wrote:

> On 2014/07/26 21:36:12, Keith wrote:
>> On 2014/07/26 06:49:41, janek wrote:
>> >
>> >     Setting TextScript.cross-staff property to #f is required to
> ensure
>> >     that there are no collisions between TextScripts and cross-staff
> notes:

>> The concept of a "cross-staff note" seems strange.  It appeared with
> the change
>> for issue 2527 https://codereview.appspot.com/6827072#msg13


> Hmm.  Do i see correctly that the patch in
> https://codereview.appspot.com/6827072 was then partially reverted with
> commit 7891600a5dd421c1f25776ea3b405c64f4f14752 ?

Right.  NoteColumns are no longer cross-staff.
If we mark TextScript.cross-staff=#t it collides with /any/ note.

Cross-staff things are skipped during outside-staff placement
   axis-group-interface.cc:939
(though it seems they could, with more code, be placed relative to their parent
staff, without being included in the parent staff's skyline).

Most things that go cross-staff use the side-position-interface to avoid
collisions, but the engraver for TextScripts does not put anything into its
'support' list so that method has no effect.

I think the example of issue 1300 succeeds only because TextScript is put in a
ScriptColumn with the accent.  It collides in the stable release if there is no
accent, or a trill in place of the accent.

define-grob-properties says 'cross-staff' means that the object can change shape
or move relative to its parent, depending on how staves are spaced on the page. 
 TextScripts do not yet respond to staff-spacing, except when they are in a
ScriptColumn that knows how to avoid a cross-staff beam, and that case seems
inconsistent.

> Shall i revert commit
> 2371d6ba3b62d4d6dc349ab50fa0d76eadfba044 for now?

I don't know.   The case of issue 1300 was not a realistic input, and similar
cases fail in the stable build.  On the other hand, from the tracker issue, it
looks like your commit doesn't provide us with any improvements.