Re: Implements footnotes in LilyPond (issue4245062)

[Neil Puttock]

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.

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).

@@ -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

Cheers,
Neil