Reviewers: dak, Message: Thanks David! You make valid points, I just have a couple of doubts below.
https://codereview.appspot.com/348120043/diff/1/input/regression/script-stencil.ly File input/regression/script-stencil.ly (right): https://codereview.appspot.com/348120043/diff/1/input/regression/script-stencil.ly#newcode31 input/regression/script-stencil.ly:31: #(append! default-script-alist On 2019/02/22 16:19:53, dak wrote:
No. Don't change any system data structures destructively or the
changes will
persist across multiple files specified on the command line.
Done. https://codereview.appspot.com/348120043/diff/1/input/regression/script-stencil.ly#newcode43 input/regression/script-stencil.ly:43: #(assoc-set! On 2019/02/22 16:19:53, dak wrote:
Same here.
Done. https://codereview.appspot.com/348120043/diff/1/lily/include/lily-imports.hh File lily/include/lily-imports.hh (right): https://codereview.appspot.com/348120043/diff/1/lily/include/lily-imports.hh#newcode86 lily/include/lily-imports.hh:86: extern Variable ly_stencil_p; On 2019/02/22 16:19:53, dak wrote:
Unnecessary in C++. Use unsmob<Stencil> instead.
Done. https://codereview.appspot.com/348120043/diff/1/lily/lily-imports.cc File lily/lily-imports.cc (right): https://codereview.appspot.com/348120043/diff/1/lily/lily-imports.cc#newcode80 lily/lily-imports.cc:80: Variable ly_stencil_p ("ly:stencil?"); On 2019/02/22 16:19:53, dak wrote:
See above.
Done. https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc File lily/script-interface.cc (right): https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc#newcode32 lily/script-interface.cc:32: #include "lily-imports.hh" On 2019/02/22 16:19:53, dak wrote:
lily-imports.h should no longer be needed.
Done. https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc#newcode36 lily/script-interface.cc:36: Script_interface::get_glyph_or_stencil (Grob *me, Direction d) On 2019/02/22 16:19:53, dak wrote:
There is no point in renaming this into get_glyph_or_stencil if it
cannot return
anything but a stencil.
The point was to avoid any risk of confusion with Grob::get_stencil (). If you find it unnecessary, I’ll remove it. https://codereview.appspot.com/348120043/diff/1/lily/script-interface.cc#newcode59 lily/script-interface.cc:59: if (to_boolean (Lily::ly_stencil_p (s))) On 2019/02/22 16:19:53, dak wrote:
Should be something like if (Stencil *stil = unsmob<Stencil> (s)) return *stil;
Done.
Actually, should likely be "else if" since there is no point in
checking again
in case we ended with a programming_error above.
Well, that’s a problem because most stencil expressions do syntactically satisfy the scm_is_pair test above. So all I could come up with was to let the test unfold in case it’s a valid glyph lookup, and only then test for a possible Stencil smob. https://codereview.appspot.com/348120043/diff/1/scm/c++.scm File scm/c++.scm (right): https://codereview.appspot.com/348120043/diff/1/scm/c++.scm#newcode47 scm/c++.scm:47: (define-public (pair-or-stencil? x) On 2019/02/22 16:19:53, dak wrote:
Seems like "pair" is a stand-in for something more specific. While
the
predicate previously was just pair? likely for reasons of laziness, in combination with stencil? it seems like something more specific would
be
decidedly less awkward.
There are several awkward things here. To wit, (feta . ("upglyph" . "downglyph")) where I’d much rather see "feta" as a quoted string. But I didn’t want to change that in order to make my patch as little disruptive as possible. https://codereview.appspot.com/348120043/diff/1/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): https://codereview.appspot.com/348120043/diff/1/scm/define-grob-properties.scm#newcode1446 scm/define-grob-properties.scm:1446: (script-stencil ,pair-or-stencil? "A pair @code{(@var{type} . @var{arg})} which On 2019/02/22 16:19:53, dak wrote:
It seems to me like it would make more sense to leave script-stencil
as it is
and instead put a callback into stencil that interprets it.
Are you referring to the Script.stencil property? That wouldn’t be of much use since this property affects all Script objects whereas script-stencil definitions allow to define many distinct articulations. So your callback would have to refer to an alist to look up appropriate glyphs/stencils anyway.
Then you can just override stencil if you want to specify a specific stencil.
That’s already what we’re doing right now, no change needed whatsoever. But it’s a rather clunky hack, and doesn’t integrate well with the alist definitions. (But I may be the one missing something here…) Description: Allow scripts to be defined either as glyphs or stencils. This feature has been discussed several times over the years; this patch aims to lift the limitation that Script grobs are only to be defined as Feta glyphs rather than glyphs from any font (assuming their naming closely follows that of Feta), or any Stencil definition. (See regtest for a demonstration; proper doc to follow if this feature gets accepted.) Please review this at https://codereview.appspot.com/348120043/ Affected files (+87, -16 lines): A input/regression/script-stencil.ly M lily/include/lily-imports.hh M lily/include/script-interface.hh M lily/lily-imports.cc M lily/script-interface.cc M scm/c++.scm M scm/define-grob-properties.scm M scm/lily.scm _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel