[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044
From: |
mtsolo |
Subject: |
Re: Add \path markup command, and use it for \eyeglasses. (issue1730044) |
Date: |
Mon, 05 Jul 2010 05:48:16 +0000 |
Great work!
http://codereview.appspot.com/1730044/diff/17001/12002
File scm/define-markup-commands.scm (right):
http://codereview.appspot.com/1730044/diff/17001/12002#newcode743
scm/define-markup-commands.scm:743:
I think this is EXCELLENT work and has enough shared features with the
recently-committed connected-shape stencil so that the best elements of
the two can be combined into one unified stencil. What I think is very
strong about this approach (and what connected-shape lacks) is the
relative versus absolute distinction with coordinates: the fact that
here one can choose between the two on a line-by-line basis is very
slick.
I see 3.5 places where the patch may need improvement. 1 is that lineto
and curveto seem unnecessary, as they can be automatically detected by
the number of function arguments. Two is that I try to use predefined
lilypond commands as much as possible when they exist - could the
moveto's be calls to ly:stencil-translate? Third is that I am wary of
any loop and/or for-each construct in scheme: I think there is a way to
do this with tail-regression that dispenses with the loop and is more
Schemy. Three.5, I think the extents are off for the curves in certain
problematic cases, but that'll work hopefully work itself out via this
proposition, to wit:
I think the best way to move forward on this patch would be to work on
merging its functionality and nomenclature into the connected-shape
stencil. I'd be more than happy to iron that out with you on the
sidelines - just shoot me an email and we'll get that up and running.
http://codereview.appspot.com/1730044/show
- Re: Add \path markup command, and use it for \eyeglasses. (issue1730044),
mtsolo <=
- Re: Add \path markup command, and use it for \eyeglasses. (issue1730044), Carl . D . Sorensen, 2010/07/05
- Re: Add \path markup command, and use it for \eyeglasses. (issue1730044), pnorcks, 2010/07/13
- Re: Add \path markup command, and use it for \eyeglasses. (issue1730044), pnorcks, 2010/07/28
- Re: Add \path markup command, and use it for \eyeglasses. (issue1730044), Carl . D . Sorensen, 2010/07/28
- Re: Add \path markup command, and use it for \eyeglasses. (issue1730044), pnorcks, 2010/07/28