Thank you very much, Han Wen, for your comments. It optimizes several
sluggish parts of the algorithm and makes for more maintainable &
readable code.
I feel that, when you read over my comments below, you'll see that the
stuff in the balloon engraver is necessary to get correctly working
annotations that attach to (almost) grobs. The thing you're identifying
as hackish does not have to do with grob alignment but rather spanner
choice (see below). I'll e-mail you PDFs of the regtests so that you
don't have to run them & you'll see what I mean.
I see where you're coming from when you say that this chunk of code is
gargantuan and would be better if broken up. It is true that this is a
lot to chew, but I put the brakes on once I got to a point that I felt
was stable. Looking at the definition of what a footnote is, it seems
like an acceptable implementation of footnotes is anything providing
annotations that unambiguously apply to a given object as well as
bottom-of-page notes to which these annotations refer. I think this
implementation, in spite of its size, provides the bare minimum to fit
with what a footnote is.
Imagine that we left either of these two aspects out (annotations and
notes). If we gave users of the unstable version the ability to
annotate every grob (items, spanners, brokens, etc) but not have notes
at the bottom of the page to which these notes referred, then it would
not be much of an improvement over the balloon feature. If we gave
users footnotes without annotations, it would likely cause them to do
crazy machinations to get the grobs annotated that are not annotatable
by balloons, which they would then have to undo once the annotation
element were implemented (this is exactly what I was doing w/ my first
drafts of this code - I eventually gave up and just used TextScripts,
which are sometimes far from the grobs in question and seem like
quizzical articulations &/or fingering indications in a couple places).
I think that this version, while large, gives users the ability to not
only test out footnotes, but also to avoid putting in extra effort to
fully evaluate and benefit from this feature.
One major element that is not implemented and that I will not be
implementing for several weeks/months is automatic numbering. That
seems like a neatly separable problem that will not get in the way of
testing out this feature. Or rather, if it gets in the way, the extent
to which it encumbers users will be minimal compared to the annoyance of
drumming up custom annotations and/or notes.
So, rather than pushing this bit by bit, I'd like to push this all as
one feature. Because of that, I've been holding out for comments from
several reviewers and have been making many drafts. I know that this
slows down understanding when a new revier tries to digest this whole
thing, but I feel that ultimately, the effort that reviewers such as
yourself, Neil, and Joe are putting in will assure that we're pushing
quality code that allows users to comment upon the feature in the most
holistic way possible.
http://codereview.appspot.com/4213042/diff/47004/lily/balloon.ccFile 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)
On 2011/03/04 15:42:49, hanwenn wrote:
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.
The problem is that, without this change to the balloon engraver, the
footnotes will not attach to their parent grobs. I had an initial
version where people could just use text markups for footnotes, but this
proved dissatisfactory very quickly as I thought about footnoting time
signatures, spanners, etc. The work on the balloon engraver allows
almost any grob to be footnoted. I feel that it is essential if one
defines a footnote as an in-text reference coupled with the note at the
bottom of the page. This is the in-text reference part.
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);
On 2011/03/04 15:42:49, hanwenn wrote:
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.
I changed the variable name & the -1 ... 1 thing as well, but because
this is a slidable variable (meaning that it can have more than 3
values), I avoided linear_combination.
http://codereview.appspot.com/4213042/diff/47004/lily/balloon.cc#newcode104
lily/balloon.cc:104: return SCM_EOL;
On 2011/03/04 15:42:49, hanwenn wrote:
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?
Because FoonoteSpanner spans the same span as its parent, if you don't
use this method, the footnote will print out several times. So, this
code only uses rank to choose which child to footnote. After that, all
alignment happens using more or less the old balloon-engraver code.
I think that it's important to be able to put footnotes on all grobs
(including spanners). Check out the regtests for examples of that. I
believe that this allows for more exact control than if one used
markups.
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_;
On 2011/03/04 15:42:49, hanwenn wrote:
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?
Done.
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_;
On 2011/03/04 15:42:49, hanwenn wrote:
Can you comment on why this can't be a grob object property?
see grob-array.h for how to manipulate vectors of grobs.
I tried my best to implement this but could not. It is a really good
idea, and you're right that it's more LilyPond-ish, but the problem is
that when the system breaks, for some reason all of the broken spanners
become over-represented and footnotes that span multiple lines are
printed multiple times at the bottom of the page.
This is something that I'd like to do after pushing an initial patch.
I've added this as a TODO item in system.hh in case anyone else wants to
take a stab at it.
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
On 2011/03/04 15:42:49, hanwenn wrote:
(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.
Done.
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);
On 2011/03/04 15:42:49, hanwenn wrote:
extent.length(). No need to do a max().
Done.
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"));
On 2011/03/04 15:42:49, hanwenn wrote:
is foot- something conceptually different from footnote?
Yup. The foot-stencil is the original footer stencil, to which I add
footnotes.
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
On 2011/03/04 15:42:49, hanwenn wrote:
please note from where.
Done.
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);
On 2011/03/04 15:42:49, hanwenn wrote:
why are you placing the stencils together in case of a prob, but not
in case of
System?
This bit of code is returning one stencil for every line, which may or
may not be comprised of multiple footnotes depending on the line. I'll
comment that.
http://codereview.appspot.com/4213042/diff/47004/lily/page-layout-problem.cc#newcode95
lily/page-layout-problem.cc:95: bool are_footnotes = false;
On 2011/03/04 15:42:49, hanwenn wrote:
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.
Done.
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)
On 2011/03/04 15:42:49, hanwenn wrote:
stencil has an is_empty method.
Done.
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)
On 2011/03/04 15:42:49, hanwenn wrote:
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..
Done.
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)
On 2011/03/04 15:42:49, hanwenn wrote:
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,
I've made this 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))
On 2011/03/04 15:42:49, hanwenn wrote:
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.)
Done.
http://codereview.appspot.com/4213042/diff/47004/lily/paper-system.cc#newcode55
lily/paper-system.cc:55: else if (SCM_EOL != footnote)
On 2011/03/04 15:42:49, hanwenn wrote:
check explicitly. do you mean unsmob_stencil() here ?
This is a little tricky - it is essentially dealing with an internal
stencil property that no one in their right mind should ever set (like
translate-stencil). Basically I'm saying "if footnote is anything other
than nothing, than it is probably supposed to be there, in which case
you should use it."
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")))
On 2011/03/04 15:42:49, hanwenn wrote:
Can you make a class Footnote_interface:: to contain this check?
footnote-interface is defined in define-grob-interfaces.scm
http://codereview.appspot.com/4213042/diff/47004/lily/system.cc#newcode244
lily/system.cc:244: vector<Grob *>
On 2011/03/04 15:42:49, hanwenn wrote:
if you have a lot of grobs , this is expensive. Better pass a ptr to
vector
into the function
Done.
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)
On 2011/03/04 15:42:49, hanwenn wrote:
st -> start
Done.
http://codereview.appspot.com/4213042/diff/47004/lily/system.cc#newcode249
lily/system.cc:249: populate_footnote_grob_vector ();
On 2011/03/04 15:42:49, hanwenn wrote:
if you don't have a footnotes at all, you will run through
all-elements on every
call of this function.
Fixed
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);
On 2011/03/04 15:42:49, hanwenn wrote:
this is code dup from what I saw earlier, and the same comments hold.
Fixed.
http://codereview.appspot.com/4213042/diff/47004/lily/system.cc#newcode267
lily/system.cc:267: Annotation_visibility item_visible =
Balloon_interface::is_visible (item);
On 2011/03/04 15:42:49, hanwenn wrote:
you could do
bool is_visible(Grob*, Direction* dir)
so you don't need to introduce a new type.
The issue here would be that I'd need to call this function 3 times
instead of once for all 3 directions if I want to know if it is just
plain visible. i.e. is_visible(grob, RIGHT) || is_visible (grob,
CENTER) || is_visible (grob, LEFT)
http://codereview.appspot.com/4213042/diff/47004/lily/system.cc#newcode274
lily/system.cc:274: }
On 2011/03/04 15:42:49, hanwenn wrote:
up to here does not depend on st end and at all. Why don't you move
this into
the populate function?
Because the populate function exists to populate the vector with all
footnotes. This function finds footnotes between two points (start and
end).
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.")
On 2011/03/04 15:42:49, hanwenn wrote:
huh?
can't you use set_parent(Y_AXIS) to store this?
Done.
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
On 2011/03/04 15:42:49, hanwenn wrote:
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.
If there are many broken spanners, this gives the user the ability to
place the spanner on any of these broken siblings.
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}."
On 2011/03/04 15:42:49, hanwenn wrote:
Can you be more explicit? What does 'apply' mean in this context?
Also, rename
the args to indicate what you are doing.
Done.
http://codereview.appspot.com/4213042/diff/47004/scm/define-markup-commands.scm#newcode1835
scm/define-markup-commands.scm:1835: (ly:stencil-combine-at-edge
On 2011/03/04 15:42:49, hanwenn wrote:
since the footnote has zero dimensions, why don't you simply construct
a new
stencil with combine and the arg1's dimensions.
I'm not exactly sure what you mean here. If you get a chance, could you
type up an example?
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.")
On 2011/03/04 15:42:49, hanwenn wrote:
why can't this be in the 'text property? In what event do you plan to
use this?
There are two components. The actual annotation to be printed next to
the grob (text) and the footnote text at the bottom (footnote-text).
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.")
On 2011/03/04 15:42:49, hanwenn wrote:
can this use a real verb?
From the Oxford English Dictionary:
footnote v. to furnish with a footnote or footnotes; to comment on in a
footnote.
1893 N. & Q. 8th Ser. III. 190 Junius foot-notes a passing attack
on Chatham thus.
http://codereview.appspot.com/4213042/