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

[hanwenn]

Hi mike,

wow , you have a penchant for attacking difficult problems!

A lot of comments below; overall my impression is that you try to do
everything at the same time. It would be easier (also for reviewers) if
you feed them your ideas bit by bit.  I think that the whole balloon
hack and position-column-rank functionality can be dispensed of for a
first version.

I leave to Joe to decide if the logic for linebreaking is OK, as it is
his expertise.


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

http://codereview.appspot.com/4213042/diff/47004/lily/balloon.cc#newcode34
lily/balloon.cc:34: Balloon_interface::is_visible (Item* item)
why is this patch modifying the balloon code?  I understand they are
conceptually related, but the patch is very big as it is.  If this is
non-essential please drop it until the footnotes are working and
stabilized.

http://codereview.appspot.com/4213042/diff/47004/lily/balloon.cc#newcode93
lily/balloon.cc:93: Real place_on_spanner = robust_scm2double
(me->get_property ("place-on-spanner"), 0.0);
stylistic comment: - if you can avoid using prepositions in names (both
methods and vars), the code usually gets easier to read.   In this case
the normal use would be to have a number -1 .. 1, with CENTER being the
center, and LEFT/RIGHT the outer edges.

There is linear_combination() in drul-array.hh to help you do the
interpolation.

http://codereview.appspot.com/4213042/diff/47004/lily/balloon.cc#newcode104
lily/balloon.cc:104: return SCM_EOL;
this code is weird  because it uses a continuous var to align on an
erratic measure (the rank numbers).  The rank numbers should not be
taken to mean anything except for ordering the columns.

what are you trying to do?  iF people need exact control of where their
footnotes appear, they shouldn't put them on a spanner?

http://codereview.appspot.com/4213042/diff/47004/lily/include/constrained-breaking.hh
File lily/include/constrained-breaking.hh (right):

http://codereview.appspot.com/4213042/diff/47004/lily/include/constrained-breaking.hh#newcode47
lily/include/constrained-breaking.hh:47: vector<Stencil *> footnotes_;
document. what are these? Are these the things a system wants to put at
the bottom of the page? Or is the bottom page content itself?

http://codereview.appspot.com/4213042/diff/47004/lily/include/system.hh
File lily/include/system.hh (right):

http://codereview.appspot.com/4213042/diff/47004/lily/include/system.hh#newcode39
lily/include/system.hh:39: vector<Grob *> footnote_grobs_;
Can you comment on why this can't be a grob object property?
see grob-array.h for how to manipulate vectors of grobs.

http://codereview.appspot.com/4213042/diff/47004/lily/page-breaking.cc
File lily/page-breaking.cc (right):

http://codereview.appspot.com/4213042/diff/47004/lily/page-breaking.cc#newcode185
lily/page-breaking.cc:185: // take care of footnotes
(did we forbid tabs? I forgot.)

Can you drop the comment or explain what is going on? I can see
footnotes being taken care of, but don't understand what is happening.

http://codereview.appspot.com/4213042/diff/47004/lily/page-breaking.cc#newcode258
lily/page-breaking.cc:258: Real separator_span = max
(separator_extent[UP] - separator_extent[DOWN], 0.0);
extent.length(). No need to do a max().

http://codereview.appspot.com/4213042/diff/47004/lily/page-breaking.cc#newcode551
lily/page-breaking.cc:551: Stencil *foot = unsmob_stencil
(p->get_property ("foot-stencil"));
is foot- something conceptually different from footnote?

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

http://codereview.appspot.com/4213042/diff/47004/lily/page-layout-problem.cc#newcode41
lily/page-layout-problem.cc:41: // ugh...code dup
please note from where.

http://codereview.appspot.com/4213042/diff/47004/lily/page-layout-problem.cc#newcode62
lily/page-layout-problem.cc:62: footnote_stencil.add_at_edge (Y_AXIS,
DOWN, *unsmob_stencil (scm_car (st)), padding);
why are you placing the stencils together in case of a prob, but not in
case of System?

http://codereview.appspot.com/4213042/diff/47004/lily/page-layout-problem.cc#newcode95
lily/page-layout-problem.cc:95: bool are_footnotes = false;
couldn't you use foot->is_empty() instead?  or are you assuming that
foot already has some info?  i'd name the var 'found' I guess.

http://codereview.appspot.com/4213042/diff/47004/lily/page-layout-problem.cc#newcode104
lily/page-layout-problem.cc:104: if (stencil->extent (Y_AXIS).length ()
> 0.0)
stencil has an is_empty method.

http://codereview.appspot.com/4213042/diff/47004/lily/page-layout-problem.cc#newcode104
lily/page-layout-problem.cc:104: if (stencil->extent (Y_AXIS).length ()
> 0.0)
you should check for NULL. - then you can drop the SCM_EOL case.

In general, you should avoid checking for just SCM_EOL; there are many
other, non EOL objects that you can't run car/cdr on either; always use
scm_is_pair() instead..

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

http://codereview.appspot.com/4213042/diff/47004/lily/paper-system.cc#newcode31
lily/paper-system.cc:31: get_footnotes (SCM expr)
it might be interesting to split off the footnotes as well, ie.

  get_footnotes(SCM expr, SCM* footnotes, SCM* cleaned)

by doing it this way and overwriting the old expr in the caller, you can
make sure nobody tries to handle footnotes differently downstream.

maybe it should be a todo,

http://codereview.appspot.com/4213042/diff/47004/lily/paper-system.cc#newcode52
lily/paper-system.cc:52: for (SCM y = scm_reverse (footnote);
scm_is_pair (y); y = scm_cdr (y))
don't do reverse inside loops.  Use a pointer to SCM instead, i.e.

  SCM l = SCM_EOL
  SCM *tail = &l
  for
    ..
    *tail = scm_cons(element, SCM_EOL)
    tail = SCM_CDRLOC(*tail)

(search for examples in the source code.)

http://codereview.appspot.com/4213042/diff/47004/lily/paper-system.cc#newcode55
lily/paper-system.cc:55: else if (SCM_EOL != footnote)
check explicitly. do you mean unsmob_stencil() here ?

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

http://codereview.appspot.com/4213042/diff/47004/lily/system.cc#newcode237
lily/system.cc:237: if (all_elts[i]->internal_has_interface
(ly_symbol2scm ("footnote-interface")))
Can you make a class Footnote_interface:: to contain this check?

http://codereview.appspot.com/4213042/diff/47004/lily/system.cc#newcode244
lily/system.cc:244: vector<Grob *>
if you have a lot of grobs , this is expensive.  Better pass a ptr to
vector into the function

http://codereview.appspot.com/4213042/diff/47004/lily/system.cc#newcode245
lily/system.cc:245: System::get_footnote_grobs_in_range (vsize st, vsize
end)
st -> start

http://codereview.appspot.com/4213042/diff/47004/lily/system.cc#newcode249
lily/system.cc:249: populate_footnote_grob_vector ();
if you don't have a footnotes at all, you will run through all-elements
on every call of this function.

http://codereview.appspot.com/4213042/diff/47004/lily/system.cc#newcode262
lily/system.cc:262: pos = (int)((rpos - pos) * place_on_spanner + pos +
0.5);
this is code dup from what I saw earlier, and the same comments hold.

http://codereview.appspot.com/4213042/diff/47004/lily/system.cc#newcode267
lily/system.cc:267: Annotation_visibility item_visible =
Balloon_interface::is_visible (item);
you could do

  bool is_visible(Grob*, Direction* dir)

so you don't need to introduce a new type.

http://codereview.appspot.com/4213042/diff/47004/lily/system.cc#newcode274
lily/system.cc:274: }
up to here does not depend on st end and at all. Why don't you move this
into the populate function?

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

http://codereview.appspot.com/4213042/diff/47004/scm/define-grob-properties.scm#newcode1011
scm/define-grob-properties.scm:1011: (parent-spanner ,ly:grob? "The
parent spanner of a FootnoteSpanner.")
huh?
can't you use set_parent(Y_AXIS) to store this?

http://codereview.appspot.com/4213042/diff/47004/scm/define-grob-properties.scm#newcode1017
scm/define-grob-properties.scm:1017: still be placed at the left or
right extremity of the spanner, but this
as commented earlier, I am weary of this, because of the caveat.  Why
not have people choose between LEFT and RIGHT for now?  This is making
the code more complicated than necessary.

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

http://codereview.appspot.com/4213042/diff/47004/scm/define-markup-commands.scm#newcode1834
scm/define-markup-commands.scm:1834: "Apply the footnote @var{arg2} to
@var{arg1}."
Can you be more explicit? What does 'apply' mean in this context?  Also,
rename the args to indicate what you are doing.

http://codereview.appspot.com/4213042/diff/47004/scm/define-markup-commands.scm#newcode1835
scm/define-markup-commands.scm:1835: (ly:stencil-combine-at-edge
since the footnote has zero dimensions, why don't you simply construct a
new stencil with combine and the arg1's dimensions.

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

http://codereview.appspot.com/4213042/diff/47004/scm/define-music-properties.scm#newcode85
scm/define-music-properties.scm:85: (footnote-text ,markup? "Text to
appear in a footnote.")
why can't this be in the 'text property?  In what event do you plan to
use this?

http://codereview.appspot.com/4213042/diff/47004/scm/define-music-types.scm
File scm/define-music-types.scm (right):

http://codereview.appspot.com/4213042/diff/47004/scm/define-music-types.scm#newcode213
scm/define-music-types.scm:213: . ((description . "Footnote a grob.")
can this use a real verb?

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