Code review - Issue 2275042: Fix #888: Add ly:stencil-scale.https://codereview.appspot.com/2010-10-29T06:09:10+00:00rietveld
Message from unknown
2010-09-26T00:51:41+00:00Neil Puttockurn:md5:3d4542cf0d88bf6413647a4279afbb30
Message from unknown
2010-10-03T20:18:39+00:00Neil Puttockurn:md5:0f1ea29d976209c094e3744de7fe42f7
Message from v.villenave@gmail.com
2010-10-04T10:36:47+00:00Valentin Villenaveurn:md5:f2cef1b4de6504970390373176268d04
http://codereview.appspot.com/2275042/diff/2001/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):
http://codereview.appspot.com/2275042/diff/2001/scm/define-markup-commands.scm#newcode3375
scm/define-markup-commands.scm:3375: (number? number? markup?)
Hi Neil, your patch looks awesome! Minor comment:
Wouldn't a number-pair be more consistent with existing markup commands such as translate-scaled?
Message from pnorcks@gmail.com
2010-10-08T05:32:32+00:00Patrick McCartyurn:md5:2f59c8c40c25ea0d255a46665a4a41dd
Hi Neil,
Looks great!
I just have a couple of comments for you (below).
Thanks,
Patrick
http://codereview.appspot.com/2275042/diff/2001/input/regression/stencil-scale.ly
File input/regression/stencil-scale.ly (right):
http://codereview.appspot.com/2275042/diff/2001/input/regression/stencil-scale.ly#newcode21
input/regression/stencil-scale.ly:21: (ly:stencil-scale (ly:text-interface::print grob) 2 1.6))
There is trailing whitespace on this line.
http://codereview.appspot.com/2275042/diff/2001/lily/stencil-interpret.cc
File lily/stencil-interpret.cc (right):
http://codereview.appspot.com/2275042/diff/2001/lily/stencil-interpret.cc#newcode104
lily/stencil-interpret.cc:104: y_scale));
"resetscale" doesn't really need any formal parameters, since they are unused in the backends.
Interestingly, "resetrotation" also has (unused) formal parameters, but "resetcolor" does not. :)
http://codereview.appspot.com/2275042/diff/2001/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):
http://codereview.appspot.com/2275042/diff/2001/scm/define-markup-commands.scm#newcode3375
scm/define-markup-commands.scm:3375: (number? number? markup?)
On 2010/10/04 10:36:47, Valentin Villenave wrote:
> Hi Neil, your patch looks awesome! Minor comment:
> Wouldn't a number-pair be more consistent with existing markup commands such as
> translate-scaled?
I have a slight preference for a number-pair instead of two separate numbers.
Message from n.puttock@gmail.com
2010-10-24T11:46:39+00:00Neil Puttockurn:md5:ff5aa03d0765f3a8fab9c3fbbd95d1f3
On 2010/10/04 10:36:47, Valentin Villenave wrote:
> Hi Neil, your patch looks awesome!
Thanks.
> Wouldn't a number-pair be more consistent with existing markup commands such as
> translate-scaled?
Yep, and it saves a hash too. :)
Cheers,
Neil
Message from n.puttock@gmail.com
2010-10-24T11:51:22+00:00Neil Puttockurn:md5:d95ecb6eee4c61e4a5e90266c5a410de
On 2010/10/08 05:32:32, Patrick McCarty wrote:
> Looks great!
Thanks.
> http://codereview.appspot.com/2275042/diff/2001/input/regression/stencil-scale.ly
> File input/regression/stencil-scale.ly (right):
>
> http://codereview.appspot.com/2275042/diff/2001/input/regression/stencil-scale.ly#newcode21
> input/regression/stencil-scale.ly:21: (ly:stencil-scale
> (ly:text-interface::print grob) 2 1.6))
> There is trailing whitespace on this line.
Oops, standards are slipping. ;)
> http://codereview.appspot.com/2275042/diff/2001/lily/stencil-interpret.cc
> File lily/stencil-interpret.cc (right):
>
> http://codereview.appspot.com/2275042/diff/2001/lily/stencil-interpret.cc#newcode104
> lily/stencil-interpret.cc:104: y_scale));
> "resetscale" doesn't really need any formal parameters, since they are unused in
> the backends.
>
> Interestingly, "resetrotation" also has (unused) formal parameters, but
> "resetcolor" does not. :)
Ah, I just obediently followed the example of `resetrotation', while thinking it slightly silly to send unused params.
> http://codereview.appspot.com/2275042/diff/2001/scm/define-markup-commands.scm
> File scm/define-markup-commands.scm (right):
>
> http://codereview.appspot.com/2275042/diff/2001/scm/define-markup-commands.scm#newcode3375
> scm/define-markup-commands.scm:3375: (number? number? markup?)
> On 2010/10/04 10:36:47, Valentin Villenave wrote:
> > Hi Neil, your patch looks awesome! Minor comment:
> > Wouldn't a number-pair be more consistent with existing markup commands such
> as
> > translate-scaled?
>
> I have a slight preference for a number-pair instead of two separate numbers.
OK, I'll sort out a revised patch with all the suggested changes.
Cheers,
Neil
Message from unknown
2010-10-24T13:01:59+00:00Neil Puttockurn:md5:d183d37aa452e3fd9ee536bff2558eb4
Message from unknown
2010-10-24T13:04:40+00:00Neil Puttockurn:md5:a8e9a77024c136caaa1c5151344a64a2
Message from unknown
2010-10-24T13:07:19+00:00Neil Puttockurn:md5:5476afa099ce6ea00fb6d58ffd974bd2
Message from pnorcks@gmail.com
2010-10-25T05:41:21+00:00Patrick McCartyurn:md5:cb0619d9ea4169e8dae31419a704b9b9
LGTM.
Thanks,
Patrick
Message from lemzwerg@googlemail.com
2010-10-25T06:03:36+00:00lemzwergurn:md5:2d5efe4f2bbaba634ce61f4cd0200679
LGTM too.
http://codereview.appspot.com/2275042/diff/18001/lily/stencil-scheme.cc
File lily/stencil-scheme.cc (right):
http://codereview.appspot.com/2275042/diff/18001/lily/stencil-scheme.cc#newcode398
lily/stencil-scheme.cc:398: {
Don't say @code{@var{...}} but @var{...}.
http://codereview.appspot.com/2275042/diff/18001/scm/define-markup-commands.scm
File scm/define-markup-commands.scm (right):
http://codereview.appspot.com/2275042/diff/18001/scm/define-markup-commands.scm#newcode3381
scm/define-markup-commands.scm:3381: Scale @code{@var{arg}}. @code{@var{factor-pair}} is a pair of numbers
Don't say @code{@var{...}} but @var{...}
Message from n.puttock@gmail.com
2010-10-25T21:53:54+00:00Neil Puttockurn:md5:c9ad07ac7ecb96c4d6b5950585989241
On 2010/10/25 06:03:36, lemzwerg wrote:
> Don't say @code{@var{...}} but @var{...}.
Hmm, have I misinterpreted Mark's post here?:
http://lists.gnu.org/archive/html/lilypond-devel/2010-09/msg00285.html
Message from wl@gnu.org
2010-10-26T05:52:08+00:00wl_gnu.orgurn:md5:3fdaee741f5cbda820978debafd17368
>> Don't say @code{@var{...}} but @var{...}.
>
> Hmm, have I misinterpreted Mark's post here?:
>
> http://lists.gnu.org/archive/html/lilypond-devel/2010-09/msg00285.html
Well, have a look at section A.17 (Scheme functions) and see how
function names and arguments are formatted. Mark is referring to code
blocks which contain metasyntactical variables. However, for your
function (which is eventually appearing in A.17 too), this is not
appropriate.
Werner
Message from markpolesky@gmail.com
2010-10-26T08:21:41+00:00Mark Poleskyurn:md5:c9f42466095462c5eb55fd15efc577f7
On 2010/10/26 05:52:08, wl_gnu.org wrote:
> Well, have a look at section A.17 (Scheme functions) and
> see how function names and arguments are formatted. Mark
> is referring to code blocks which contain metasyntactical
> variables. However, for your function (which is
> eventually appearing in A.17 too), this is not
> appropriate.
Well... Okay, yeah, but see this:
http://kainhofer.com/~lilypond/Documentation/contributor/syntax-survey.html#miscellany
I'm the one that wrote the @var description there. And yes,
the rationale is simplistic: "This improves readability in
the PDF and HTML output." But I think Neil's impulse to
format it that way matches the spirit of the rationale, no?
And I certainly wouldn't be opposed to instituting the same
policy for A.17, though of course that would be best left
for a different patch than this one.
Werner, if you have any objections to adding @code to all
the @var's in A.17, I'd like to hear them.
Thanks.
- Mark
Message from wl@gnu.org
2010-10-26T12:07:47+00:00wl_gnu.orgurn:md5:8413460c82c998782368a59610926a84
> I'm the one that wrote the @var description there. And yes, the
> rationale is simplistic: "This improves readability in the PDF and
> HTML output."
Does it? It's not clear to me why the formatting of A.17 looks better
if using typewriter in upright and italic shapes. It's really a
matter of taste...
> Werner, if you have any objections to adding @code to all the @var's
> in A.17, I'd like to hear them.
It is not an important issue to me whether to use typewriter or not.
Whatever you decide to use, it should be *consistent*. So for the
time being, stuff going to A.17 should look similar to the other
entries already there.
Werner
Message from Carl.D.Sorensen@gmail.com
2010-10-27T18:39:30+00:00Carlurn:md5:62535255dea14859fcb923ca9a32183f
On 2010/10/26 08:21:41, Mark Polesky wrote:
> Well... Okay, yeah, but see this:
> http://kainhofer.com/%7Elilypond/Documentation/contributor/syntax-survey.html#miscellany
>
> I'm the one that wrote the @var description there. And yes,
> the rationale is simplistic: "This improves readability in
> the PDF and HTML output." But I think Neil's impulse to
> format it that way matches the spirit of the rationale, no?
> And I certainly wouldn't be opposed to instituting the same
> policy for A.17, though of course that would be best left
> for a different patch than this one.
I think that wrapping a variable declaration in a code declaration is undesirable. It complicates the format of the documentation, for very little benefit (if any).
If we want to change the format of @var{} in HTML and PDF format, we ought to be able to do that in our texinfo program.
Or alternatively, we should get a different texinfo macro, e.g. @var{} for outside of code, and @codevar{} for use inside of code.
THanks,
Carl
Message from graham@percival-music.ca
2010-10-29T06:09:10+00:00Graham Percivalurn:md5:fdaa4539c31429b75f88c4e3fb097b54
On Wed, Oct 27, 2010 at 06:39:30PM +0000, Carl.D.Sorensen@gmail.com wrote:
> I think that wrapping a variable declaration in a code declaration is
> undesirable. It complicates the format of the documentation, for very
> little benefit (if any).
>
> If we want to change the format of @var{} in HTML and PDF format, we
> ought to be able to do that in our texinfo program.
I like this idea. We're never going to want a non-code variable
in our docs.
Cheers,
- Graham