Re: Issue 4275: Allow user-defined rest styles. (issue 200860043 by address@hidden)

[k-ohara5a5a]

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/