[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: hanwenn
Subject: Re: Reduces algorithm time by prefinding footnoted grobs (issue4213042)
Date: Fri, 04 Mar 2011 15:42:49 +0000

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.
File lily/ (right):
lily/ 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
lily/ 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
lily/ 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?
File lily/include/constrained-breaking.hh (right):
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?
File lily/include/system.hh (right):
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.
File lily/ (right):
lily/ // 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.
lily/ Real separator_span = max
(separator_extent[UP] - separator_extent[DOWN], 0.0);
extent.length(). No need to do a max().
lily/ Stencil *foot = unsmob_stencil
(p->get_property ("foot-stencil"));
is foot- something conceptually different from footnote?
File lily/ (right):
lily/ // ugh...code dup
please note from where.
lily/ 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?
lily/ 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.
lily/ if (stencil->extent (Y_AXIS).length ()
stencil has an is_empty method.
lily/ if (stencil->extent (Y_AXIS).length ()
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..
File lily/ (right):
lily/ 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,
lily/ 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 *tail = &l
    *tail = scm_cons(element, SCM_EOL)
    tail = SCM_CDRLOC(*tail)

(search for examples in the source code.)
lily/ else if (SCM_EOL != footnote)
check explicitly. do you mean unsmob_stencil() here ?
File lily/ (right):
lily/ if (all_elts[i]->internal_has_interface
(ly_symbol2scm ("footnote-interface")))
Can you make a class Footnote_interface:: to contain this check?
lily/ vector<Grob *>
if you have a lot of grobs , this is expensive.  Better pass a ptr to
vector into the function
lily/ System::get_footnote_grobs_in_range (vsize st, vsize
st -> start
lily/ 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.
lily/ pos = (int)((rpos - pos) * place_on_spanner + pos +
this is code dup from what I saw earlier, and the same comments hold.
lily/ 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.
lily/ }
up to here does not depend on st end and at all. Why don't you move this
into the populate function?
File scm/define-grob-properties.scm (right):
scm/define-grob-properties.scm:1011: (parent-spanner ,ly:grob? "The
parent spanner of a FootnoteSpanner.")
can't you use set_parent(Y_AXIS) to store this?
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.
File scm/define-markup-commands.scm (right):
scm/define-markup-commands.scm:1834: "Apply the footnote @var{arg2} to
Can you be more explicit? What does 'apply' mean in this context?  Also,
rename the args to indicate what you are doing.
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.
File scm/define-music-properties.scm (right):
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?
File scm/define-music-types.scm (right):
scm/define-music-types.scm:213: . ((description . "Footnote a grob.")
can this use a real verb?

reply via email to

[Prev in Thread] Current Thread [Next in Thread]