Re: Reduces algorithm time by prefinding footnoted grobs (issue4213042)

[mtsolo]

Thanks Neil!

Just a couple questions below - everything else was pretty automatic.


http://codereview.appspot.com/4213042/diff/34032/input/regression/footnotes.ly
File input/regression/footnotes.ly (right):

http://codereview.appspot.com/4213042/diff/34032/input/regression/footnotes.ly#newcode7
input/regression/footnotes.ly:7: \book { }
On 2011/02/27 22:42:24, Neil Puttock wrote:
> Sorry, I meant wrap all the music inside a \book { }, otherwise the
regression
> test won't show any footnotes at the bottom of each page.


Done.

http://codereview.appspot.com/4213042/diff/34032/lily/balloon.cc
File lily/balloon.cc (right):

http://codereview.appspot.com/4213042/diff/34032/lily/balloon.cc#newcode90
lily/balloon.cc:90: // ugh...code dup...hopefully can be consolidated w/
above one day
On 2011/02/27 22:42:24, Neil Puttock wrote:
> Can't you move most of the stencil creation to a separate method?

Done.

http://codereview.appspot.com/4213042/diff/34032/lily/balloon.cc#newcode99
lily/balloon.cc:99: if (parent->broken_intos_.size () == 0)
On 2011/02/27 22:42:24, Neil Puttock wrote:
> if (!parent->is_broken ())

Done.

http://codereview.appspot.com/4213042/diff/34032/lily/dynamic-align-engraver.cc
File lily/dynamic-align-engraver.cc (right):

http://codereview.appspot.com/4213042/diff/34032/lily/dynamic-align-engraver.cc#newcode92
lily/dynamic-align-engraver.cc:92: * robust_scm2double (info.grob
()->get_property ("Y-offset"), 0.0) > 0.0)) {
On 2011/02/27 22:42:24, Neil Puttock wrote:
> This hard-codes an exception to the footnote's extent; I think it
would be
> better to add it in every case then rely on overriding Y-extent if it
shouldn't
> take up space.

> This is still a kludge though, since there are other alignment
spanners which
> contain grobs we might like to add footnotes to (e.g., pedals).
Ideally, you
> need a more flexible mechanism which can be applied to any relevant
axis group.

For now, I'm just gonna remove this kludge and leave it as is.  It means
that certain spanners may budge when annotations are attached to them,
but I'll need more time to figure out a sustainable way to get the axis
engraver to ignore the height of these annotations.  My initial hunch
after reading your comment was to set cross-staff to #t, but this
segfaults on any spanner that has a line break.

This segfault for cross staff may reveal some problems with the way that
the FootnoteSpanner is being treated in the axis engraver - any
thoughts?

http://codereview.appspot.com/4213042/diff/34032/lily/footnote-engraver.cc
File lily/footnote-engraver.cc (right):

http://codereview.appspot.com/4213042/diff/34032/lily/footnote-engraver.cc#newcode4
lily/footnote-engraver.cc:4: Copyright (C) 2006--2011 Han-Wen Nienhuys
<address@hidden>
On 2011/02/27 22:42:24, Neil Puttock wrote:
> Fix copyright.

Done.

http://codereview.appspot.com/4213042/diff/34032/lily/footnote-engraver.cc#newcode76
lily/footnote-engraver.cc:76: b->set_property ("parent-spanner",
s->self_scm ());
On 2011/02/27 22:42:24, Neil Puttock wrote:
> What happens to the Y-parent setting above?  If it's valid, you
shouldn't need
> an object pointer.

> If you do need it, it should be set_object () (and for reading,
get_object ())

I think I do need it - a pretty print to the command line shows that the
parent of these spanners is a PaperColumn when the print function is
called in spite of the fact that I set it to be a spanner, whereas
"parent-spanner" doesn't change.  Somewhere in the code, this guy's
parents are getting reset.  If I can track that down, I'll change the
property, but for now, "parent-spanner" is the only thing that passes
through the correct spanner.  However, it is important for these guys to
have their parents set - otherwise, the spacing goes awry (again...I
don't understand why...I just know that it works!).

http://codereview.appspot.com/4213042/diff/34032/lily/footnote-engraver.cc#newcode98
lily/footnote-engraver.cc:98: {
On 2011/02/27 22:42:24, Neil Puttock wrote:
> remove { }

Done.

http://codereview.appspot.com/4213042/diff/34032/lily/footnote-engraver.cc#newcode119
lily/footnote-engraver.cc:119: "Footnote ",
On 2011/02/27 22:42:24, Neil Puttock wrote:
> "Footnote "
> "FootnoteSpanner ",

> Perhaps rename Footnote FootnoteItem.

I don't mind renaming it, but could you give a reason for doing so?
Right now, I think the names clearly communicate that one footnote is
for non-spanners and one is for spanners.  However, I know little about
LilyPond naming conventions and would be happy to make the change if you
felt this would be more appropriate.

http://codereview.appspot.com/4213042/diff/34032/lily/footnote-engraver.cc#newcode122
lily/footnote-engraver.cc:122: "",
On 2011/02/27 22:42:24, Neil Puttock wrote:
> "currentCommandColumn "
> "currentMusicalColumn "
> "whichBar ",

Done.

http://codereview.appspot.com/4213042/diff/34032/lily/page-layout-problem.cc
File lily/page-layout-problem.cc (right):

http://codereview.appspot.com/4213042/diff/34032/lily/page-layout-problem.cc#newcode77
lily/page-layout-problem.cc:77: paper->self_scm ());
On 2011/02/27 22:42:24, Neil Puttock wrote:
> indent

Done.

http://codereview.appspot.com/4213042/diff/34032/lily/page-layout-problem.cc#newcode104
lily/page-layout-problem.cc:104: if (stencil->extent (Y_AXIS).length() >
0.0)
On 2011/02/27 22:42:24, Neil Puttock wrote:
> length ()

Done.

http://codereview.appspot.com/4213042/diff/34032/ly/engraver-init.ly
File ly/engraver-init.ly (right):

http://codereview.appspot.com/4213042/diff/34032/ly/engraver-init.ly#newcode537
ly/engraver-init.ly:537: \consists "Footnote_engraver"
On 2011/02/27 22:42:24, Neil Puttock wrote:
> move back to Voice (sorry! :)

Done.

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

http://codereview.appspot.com/4213042/diff/34032/scm/define-grob-interfaces.scm#newcode92
scm/define-grob-interfaces.scm:92: '(footnote-text))
On 2011/02/27 22:42:24, Neil Puttock wrote:
> + parent-spanner if still required

Done.

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

http://codereview.appspot.com/4213042/diff/34032/scm/define-grob-properties.scm#newcode298
scm/define-grob-properties.scm:298: (footnote-text ,markup? "A footnote
for the grob.")
On 2011/02/27 22:42:24, Neil Puttock wrote:
> + parent-spanner if required

> (add to `all-internal-grob-properties')

Done.

http://codereview.appspot.com/4213042/diff/34032/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4213042/diff/34032/scm/define-grobs.scm#newcode180
scm/define-grobs.scm:180: (annotation-balloon . #t)
On 2011/02/27 22:42:24, Neil Puttock wrote:
> mixing space with tabs

> (also Footnote and FootnoteSpanner)

Done.

http://codereview.appspot.com/4213042/diff/34032/scm/define-grobs.scm#newcode886
scm/define-grobs.scm:886: (footnote-text . #f)
On 2011/02/27 22:42:24, Neil Puttock wrote:
> remove

Done.

http://codereview.appspot.com/4213042/diff/34032/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):

http://codereview.appspot.com/4213042/diff/34032/scm/define-markup-commands.scm#newcode142
scm/define-markup-commands.scm:142: (define-markup-command (draw-hline
layout props)
On 2011/02/27 22:42:24, Neil Puttock wrote:
> still needs simplifying via draw-line

I'm still not quite sure how to do this - do you want me to change the
thickness property of props and then call draw-line?

http://codereview.appspot.com/4213042/