Re: Implements footnotes in LilyPond (issue4245062)

[Mike Solomon]

On Mar 5, 2011, at 8:49 AM, Neil Puttock wrote:

> On 5 March 2011 12:57, Mike Solomon <address@hidden> wrote:
> 
>> Patch attached.  The stuff that comes from your comments regarding
>> break-visibility is implemented in Balloon_interface::is_visible.
>> The patch currently represents about 85% of the original, omitting the 15%
>> that Han Wan had previously identified as hold-off-on-able for a first push
>> (the actual annotations).  These are relatively painless to add back in.
>> I realize that this is not a "small" chunk, but if I shaved anything else
>> off, the footnotes wouldn't work.
>> I'm running regtests now & will report back if anything breaks.
>> Cheers,
>> MS
>> P.S. The original patch resides at http://codereview.appspot.com/4245062
> 
> I think there are several parts to this patch which should be removed:
> 
> 1) the break-visibility code in balloon.cc
> 
> You've implemented something which looks suspiciously like a callback,
> but you've also added a function to output-lib.scm which you call
> instead via scm_call_1 () (you shouldn't need to do this for a grob).
> All this code should be part of a callback for FootnoteItem
> #'break-visibility.
> 
> This code doesn't work very well in some cases:
> 
> \new Staff \with {
>  \consists Footnote_engraver
> }
> \relative c' {
>  \footnoteGrob #'Clef #'(0 . 2) foo bar
>  c1
> }
> 
> -> no footnote
> 
> \new Staff \with {
>  \consists Footnote_engraver
> }
> \relative c' {
>  c1
>  \footnoteGrob #'Clef #'(0 . 2) foo bar
>  c1
> }
> 
> -> no footnote (good!), but suicided clefs are still accounted for
> (all three) in account_for_footnotes ()
> 
> \new Staff \with {
>  \consists Footnote_engraver
> }
> \relative c' {
>  c1 \break
>  \footnoteGrob #'Clef #'(0 . 2) foo bar
>  c1
> }
> 
> -> two footnotes, account_for_foonotes () still thinks there are three
> 
> For all these cases setting FootnoteItem #'break-visibility to
> `inherit-x-parent-visibility' produces better results.
> 

Done - thanks for bearing with me as I learn about break-visibility.  It is a 
corner of the code that I never had to deal with directly, so I'm still getting 
my sea legs.

> 2) all the code related to `spanner-placement'
> 
> This is a really bad interface for selecting the correct spanner.  I'd
> much prefer something which would index the broken_intos_ (though I
> guess this would run into the same problem you're having with the
> footnote height calculations).
> 

You hit the nail on the head.  Currently, the page-spacer is figuring out the 
heights of footnotes before the spanners are broken.  So, because broken_intos_ 
doesn't exist yet, this is the only way I could come up with to make this work.

If you feel this is too hackish, I could make this direction-only (LEFT, 
CENTER, RIGHT) with CENTER defaulting to LEFT and have the footnote only apply 
to the first and last spanner.  But, for long spanners, this seems less than 
ideal.  As always, your suggestions are welcome!

> @@ -1237,7 +1277,7 @@
> Page_breaking::space_systems_with_fixed_number_per_page (vsize
> configuration,
>       while (system_count_on_this_page < systems_per_page_
>            && line < cached_line_details_.size ())
>       {
> -       Line_details const &cur_line = cached_line_details_[line];
> +       Line_details &cur_line = cached_line_details_[line];
> 
> looks like it's left over from a previous patch
> 

Fixed.

Thank you very much, Neil, for helping this thing evolve so much!  I feel that 
I'm learning a lot about LilyPond by integrating all these comments, and I 
really appreciate it.

If all looks good, please let me know - otherwise, please let me know what 
still needs to change.

Cheers,
MS

0001-Adds-support-for-footnotes-but-not-footnote-annotati.patch
Description: Binary data

0002-Implements-Neil-s-and-Joe-s-proposed-changes.patch
Description: Binary data