http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc File lily/note-collision.cc (right):
http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode536 lily/note-collision.cc:536: s[LEFT]--; // why LEFT and RIGHT instead of UP and DOWN? sometimes i liked to think about intervals as lying on the horizontal line. Feel free to change to up/down. http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode539 lily/note-collision.cc:539: offsets[d].push_back (d * 0.5 * i); // what for? you can run a regtest after removing this to be sure, but afaics, this just takes care that the hshift = 2 group is shifted relative to the hshift = 1 group, and similar for 3, 4, 5 .. http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode559 lily/note-collision.cc:559: || (extents[-d].size () && d * (extents[d][i][-d] - extents[-d][0][d]) < 0)) // please, comment this condition expanding for d = UP extents[UP][i][DOWN] < extents[DOWN][0][UP] .. offsets[UP][..all..] += 0.5 ie. if the upper reaches into area of the lower groups, then shift the upper to the right. http://codereview.appspot.com/5651069/diff/1/lily/note-collision.cc#newcode587 lily/note-collision.cc:587: for_UP_and_DOWN (d) after reading some of the code, this actually looks nice. Can you rename it for_updown ? for brevity and writability? There should also be a for_leftright() http://codereview.appspot.com/5651069/diff/5002/flower/include/direction.hh File flower/include/direction.hh (right): http://codereview.appspot.com/5651069/diff/5002/flower/include/direction.hh#newcode67 flower/include/direction.hh:67: */ If you think there should be doxygen style commenting, rather than doing these one off, build consensus on the list, and fix all comments in the codebase in one go. I personally think they look terribly verbose, and are unpleasant to read in the sourcecode. Concise english sentences work better than @awkward comments to document things, IMO. http://codereview.appspot.com/5651069/diff/5002/flower/include/direction.hh#newcode90 flower/include/direction.hh:90: */ if we decide to go ahead with this, there is no need to refer to the old style syntax, nor is it necessary to call the old one ugly. http://codereview.appspot.com/5651069/diff/5002/lily/accidental-placement.cc File lily/accidental-placement.cc (right): http://codereview.appspot.com/5651069/diff/5002/lily/accidental-placement.cc#newcode116 lily/accidental-placement.cc:116: //TODO: add comment to this class drop http://codereview.appspot.com/5651069/diff/5002/lily/accidental-placement.cc#newcode230 lily/accidental-placement.cc:230: //TODO: please add comment to this function this sets the skylines on the ape argument. http://codereview.appspot.com/5651069/diff/5002/lily/accidental-placement.cc#newcode284 lily/accidental-placement.cc:284: //TODO: please add comment to this function this function does exactly what its name says it does: it extract the noteheads and stems from its argument. http://codereview.appspot.com/5651069/diff/5002/lily/include/grob-interface.hh File lily/include/grob-interface.hh (right): http://codereview.appspot.com/5651069/diff/5002/lily/include/grob-interface.hh#newcode31 lily/include/grob-interface.hh:31: bool cl::has_interface (Grob *me) /* TODO: please add comment to this function */ \ either do it or leave, otherwise we could adorn the entire codebase with TODO. http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc File lily/note-collision.cc (right): http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcode373 lily/note-collision.cc:373: update_offsets (offsets, clash_groups, shift_amount); there is not much value in abstracting into a function unless you can use the function at least twice. http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcode528 lily/note-collision.cc:528: Drul_array<vector<Slice> > extents; // vertical extends extents, not extends http://codereview.appspot.com/5651069/diff/5002/lily/note-collision.cc#newcode536 lily/note-collision.cc:536: s[LEFT]--; // why LEFT and RIGHT instead of UP and DOWN? On 2012/02/12 01:29:14, Keith wrote:
These lines first appeared in version 1.1.49, thirteen years ago. If
you do not
get answers to your questions during this review, you will probably
have to find
the answers yourself.
the reason is that 535 gets extents in posititions, but a single note head occupies 3 positions, since the symbol has thickness, hence the -- and ++ http://codereview.appspot.com/5651069/diff/5002/lily/note-column.cc File lily/note-column.cc (right): http://codereview.appspot.com/5651069/diff/5002/lily/note-column.cc#newcode68 lily/note-column.cc:68: * Returns a Slice (an interval of half-staff spaces) which ends are the lowest and highest note head respectively this comment doesn't add any information that the name already conveys, IMO. Slice is not so much an interval of halfspaces, but it is an interval of integers. The assumption is that heads are always either on or between staff lines. http://codereview.appspot.com/5651069/diff/5002/lily/staff-symbol-referencer.cc File lily/staff-symbol-referencer.cc (right): http://codereview.appspot.com/5651069/diff/5002/lily/staff-symbol-referencer.cc#newcode135 lily/staff-symbol-referencer.cc:135: */ again, this is all is implicit in the name. 'position' refers to the integer vertical position, 0 being the center of the staff. http://codereview.appspot.com/5651069/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
