On Mar 4, 2011, at 5:02 PM, mts...@gmail.com wrote: > 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.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) > 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/
For reasons I do not understand, the type of every mode in my git repo changed this morning, and thus, when I tried to upload my most recent patch with Han Wen's changes, git cl attempted to upload my entire git repo. I killed the process, and now, the old issue does not load (I probably shouldn't have killed the process, but when I panic, I tend just to turn things off...usually works, but this time it didn't). I've created a new one: http://codereview.appspot.com/4245062/ I compiled this patch and it works but does not completely build the source for some reason...I think it may be the current master that's buggy (my current master does not compile either), although I'll do some more research. Sorry for the inconvenience! Cheers, Mike
_______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
