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

[n . puttock]

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 { }
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.

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
Can't you move most of the stencil creation to a separate method?

http://codereview.appspot.com/4213042/diff/34032/lily/balloon.cc#newcode99
lily/balloon.cc:99: if (parent->broken_intos_.size () == 0)
if (!parent->is_broken ())

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

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>
Fix copyright.

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 ());
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 ())

http://codereview.appspot.com/4213042/diff/34032/lily/footnote-engraver.cc#newcode98
lily/footnote-engraver.cc:98: {
remove { }

http://codereview.appspot.com/4213042/diff/34032/lily/footnote-engraver.cc#newcode119
lily/footnote-engraver.cc:119: "Footnote ",
"Footnote "
"FootnoteSpanner ",

Perhaps rename Footnote FootnoteItem.

http://codereview.appspot.com/4213042/diff/34032/lily/footnote-engraver.cc#newcode122
lily/footnote-engraver.cc:122: "",
"currentCommandColumn "
"currentMusicalColumn "
"whichBar ",

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 ());
indent

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)
length ()

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

http://codereview.appspot.com/4213042/diff/34032/lily/system.cc#newcode236
lily/system.cc:236: if ((all_elts[i]->name () == "Footnote") ||
(all_elts[i]->name () == "FootnoteSpanner"))
cleaner with

all_elts[i]->internal_has_interface (ly_symbol2scm
("footnote-interface"))

assuming you add footnote-interface to FootnoteSpanner

http://codereview.appspot.com/4213042/diff/34032/lily/system.cc#newcode247
lily/system.cc:247: if (footnote_grobs_.size () == 0)
!footnote_grobs_size ()

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"
move back to Voice (sorry! :)

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))
+ parent-spanner if still required

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.")
+ parent-spanner if required

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

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)
mixing space with tabs

(also Footnote and FootnoteSpanner)

http://codereview.appspot.com/4213042/diff/34032/scm/define-grobs.scm#newcode886
scm/define-grobs.scm:886: (footnote-text . #f)
remove

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)
still needs simplifying via draw-line

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