Looks pretty good. The change to tabstaff looks suspicious the way you describe it. Keeping regtests unchanged is a bad reason to put invisible rests in tablature, but keeping users' existing scores unchanged might be a good reason. I suppose the former behavior of giving Rests an extent regardless of their stencil had (probably unwanted) effect on tablature, and you are preserving the (proably unwanted) behavior.
It does seem complicated, for the simple tasks of waiting until layout is finalized enough to know if a rest was pushed ouside the staff to draw ledgers on rests. But this does seem better than the old way. Storing a tentative stencil in 'stencil-pure is better than the old way of looking it up repeatedly from fontforge. I still suspect that you could put the tentative stencil in the usual 'stencil property, and give responsibility for finalizing the rest glyph to the Rest 'positioning-done callback. Almost always, the final stencil is the same as the tentative, so having code to re-write the stencil for the exceptional case seems conceptually simpler. https://codereview.appspot.com/200860043/diff/20001/input/regression/rest-dot-position.ly File input/regression/rest-dot-position.ly (right): https://codereview.appspot.com/200860043/diff/20001/input/regression/rest-dot-position.ly#newcode11 input/regression/rest-dot-position.ly:11: Why not in rest.scm ? I guess you are going to personally tell everyone who has used the mirrored-z rest how to add these lines to thier input files ... https://codereview.appspot.com/200860043/diff/20001/lily/rest.cc File lily/rest.cc (right): https://codereview.appspot.com/200860043/diff/20001/lily/rest.cc#newcode166 lily/rest.cc:166: + offset); I guess that get_position() could possibly trigger a full layout, but I can't tell for sure without booting Linux to experiment. I trust that you are certain there is a reason we need below to carefully avoid calling is_ledgered(). https://codereview.appspot.com/200860043/diff/20001/lily/rest.cc#newcode203 lily/rest.cc:203: The comments depend on the meaning of the word 'pure' as used by a subset of LilyPond programmers. You could just say: // Get a stencil, but if 'pure'=true do so without testing if the rest needs a ledger https://codereview.appspot.com/200860043/diff/20001/ly/engraver-init.ly File ly/engraver-init.ly (right): https://codereview.appspot.com/200860043/diff/20001/ly/engraver-init.ly#newcode897 ly/engraver-init.ly:897: \hide Rest %% Should probably change to the logical \omit Rest %% but before 2015 omitted rests were allocated space, %% so this \omit Rest preserves former (maybe undesired) behavior https://codereview.appspot.com/200860043/ _______________________________________________ lilypond-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/lilypond-devel
