http://codereview.appspot.com/28134/diff/1/3 File lily/accidental-engraver.cc (right):
http://codereview.appspot.com/28134/diff/1/3#newcode431 Line 431: Grob *newa = 0; Please use a more descriptive name. Also, add a comment regarding why you need two copies of the accidental. http://codereview.appspot.com/28134/diff/1/4 File lily/accidental-placement.cc (right): http://codereview.appspot.com/28134/diff/1/4#newcode30 Line 30: Accidental_placement::add_accidental (Grob *me, Grob *a, Grob *newa) Please use more descriptive parameter names. http://codereview.appspot.com/28134/diff/1/4#newcode71 Line 71: if (break_reminder) slightly cleaner, perhaps, to do SCM accidental_sym = ly_symbol2scm(break_reminder ? "accidental-break-reminder-grobs" : "accidental_grobs"); and then accs = me->get_object(accidental_sym); ... me->set_object(accidental_sym, accs); http://codereview.appspot.com/28134/diff/1/4#newcode97 Line 97: ret.push_back(a); space before ( http://codereview.appspot.com/28134/diff/1/4#newcode433 Line 433: return; Don't need the return here http://codereview.appspot.com/28134/diff/1/4#newcode436 Line 436: void Accidental_placement::filter_tied_accidentals (Grob * me) Rather than coping every accidental and then filtering out some, have you considered delaying the copying until calc_positioning_done and only copying the accidentals you care about? This also has the advantage of hiding more stuff in accidental-placement instead of modifying accidental-engraver. http://codereview.appspot.com/28134/diff/1/4#newcode460 Line 460: "accidental-break-reminder-grobs " Only properties go here, not objects. http://codereview.appspot.com/28134/diff/1/5 File lily/accidental.cc (right): http://codereview.appspot.com/28134/diff/1/5#newcode181 Line 181: } I'm a little confused about this. Depending on whether a group of accidentals is at the beginning of a line or not, you want either all of the break-reminder accidentals or all of the non-break-reminder accidentals to suicide. It's not clear to me that this is achieved by the above code. http://codereview.appspot.com/28134/diff/1/8 File lily/ledger-line-spanner.cc (right): http://codereview.appspot.com/28134/diff/1/8#newcode237 Line 237: if (Grob *g = unsmob_grob (me->get_property ("accidental-grob"))) get_object (and subsequently, too) http://codereview.appspot.com/28134/diff/1/13 File scm/define-grob-properties.scm (right): http://codereview.appspot.com/28134/diff/1/13#newcode835 Line 835: @var{groblist})} entries. Includes accidentals that will be printed if at the beginning of the line.") "Consists of" is more accurate than "Includes", I think. http://codereview.appspot.com/28134/diff/1/13#newcode841 Line 841: @var{groblist})} entries. Does not include tied accidentals that won't be printed mid-line.") Can you rephrase to avoid the double-negative? http://codereview.appspot.com/28134 _______________________________________________ lilypond-devel mailing list [email protected] http://lists.gnu.org/mailman/listinfo/lilypond-devel
