Re: Allow scripts to be defined either as glyphs or stencils. (issue 348120043 by address@hidden)

[v . villenave]

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