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/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel