Issue 235090043: Add stencil-flip function (Closed)
|
Created:
8 years, 12 months ago by pwm Modified:
8 years, 11 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionAdd stencil-flip function
Also includes this commit:
Fix variable name in make-path-stencil
To follow the convention that xxx?
indicates a predicate.
Patch Set 1 #
Total comments: 10
Patch Set 2 : Use stil not stl to match existing code #Patch Set 3 : improve doc strings #
Total comments: 10
Patch Set 4 : more doc string edits etc. #
Total comments: 2
Patch Set 5 : more edits #
MessagesTotal messages: 16
Use stil not stl to match existing code
Sign in to reply to this message.
Please review, thanks.
Sign in to reply to this message.
LGTM. https://codereview.appspot.com/235090043/diff/1/input/regression/stencil-scal... File input/regression/stencil-scale.ly (right): https://codereview.appspot.com/235090043/diff/1/input/regression/stencil-scal... input/regression/stencil-scale.ly:8: its bounding box.)" Please use two spaces after a period that ends a sentence. https://codereview.appspot.com/235090043/diff/1/scm/stencil.scm File scm/stencil.scm (right): https://codereview.appspot.com/235090043/diff/1/scm/stencil.scm#newcode667 scm/stencil.scm:667: @var{axis} is 0 for X-axis, 1 for Y-axis." Can you somehow add the words `horizontally' and `vertically'? Additionally, I think it helps to tell the reader where the mirroring axis is positioned.
Sign in to reply to this message.
Looks good! Thanks for posting it. A couple of comments included. https://codereview.appspot.com/235090043/diff/1/input/regression/stencil-scal... File input/regression/stencil-scale.ly (right): https://codereview.appspot.com/235090043/diff/1/input/regression/stencil-scal... input/regression/stencil-scale.ly:8: its bounding box.)" I don't think information about stencil-flip should be put in this regtest, unless stencil-flip is also added to the regtest. The headers of the regtests should only discuss the particular regtest, in my opinion. https://codereview.appspot.com/235090043/diff/1/scm/stencil.scm File scm/stencil.scm (right): https://codereview.appspot.com/235090043/diff/1/scm/stencil.scm#newcode467 scm/stencil.scm:467: (is-absolute (if (memq head-raw Probably a nice touch, since absolute? is probably not really a predicate. https://codereview.appspot.com/235090043/diff/1/scm/stencil.scm#newcode667 scm/stencil.scm:667: @var{axis} is 0 for X-axis, 1 for Y-axis." Perhaps you could also use X and Y instead of 0 and 1 (see the code below where you use X)
Sign in to reply to this message.
improve doc strings
Sign in to reply to this message.
Thanks for the feedback! I've improved the doc strings, and revised the doc string for ly:stencil-scale so that it covers negative arguments. -Paul https://codereview.appspot.com/235090043/diff/1/input/regression/stencil-scal... File input/regression/stencil-scale.ly (right): https://codereview.appspot.com/235090043/diff/1/input/regression/stencil-scal... input/regression/stencil-scale.ly:8: its bounding box.)" On 2015/05/05 03:53:05, lemzwerg wrote: > Please use two spaces after a period that ends a sentence. Thanks for the reminder. https://codereview.appspot.com/235090043/diff/1/input/regression/stencil-scal... input/regression/stencil-scale.ly:8: its bounding box.)" On 2015/05/05 04:16:37, Carl wrote: > I don't think information about stencil-flip should be put in this regtest, > unless stencil-flip is also added to the regtest. > > The headers of the regtests should only discuss the particular regtest, in my > opinion. Makes sense, and done. https://codereview.appspot.com/235090043/diff/1/scm/stencil.scm File scm/stencil.scm (right): https://codereview.appspot.com/235090043/diff/1/scm/stencil.scm#newcode467 scm/stencil.scm:467: (is-absolute (if (memq head-raw On 2015/05/05 04:16:38, Carl wrote: > Probably a nice touch, since absolute? is probably not really a predicate. Yep, just cleaning up after myself here. https://codereview.appspot.com/235090043/diff/1/scm/stencil.scm#newcode667 scm/stencil.scm:667: @var{axis} is 0 for X-axis, 1 for Y-axis." On 2015/05/05 04:16:38, Carl wrote: > Perhaps you could also use X and Y instead of 0 and 1 (see the code below where > you use X) Done. https://codereview.appspot.com/235090043/diff/1/scm/stencil.scm#newcode667 scm/stencil.scm:667: @var{axis} is 0 for X-axis, 1 for Y-axis." On 2015/05/05 03:53:05, lemzwerg wrote: > Can you somehow add the words `horizontally' and `vertically'? Done. > Additionally, I > think it helps to tell the reader where the mirroring axis is positioned. I think I have this covered now, although with different wording.
Sign in to reply to this message.
https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm File scm/stencil.scm (right): https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode668 scm/stencil.scm:668: An @var{axis} of Y or 1 will flip it vertically. What about: Value @code{X} (or @code{0}) for @var{axis} flips it horizontally. Value @code{Y} (or @code{1}) flips it vertically. Exactly defined values for a variable should be represented with `@code'. And I'm a great fan of avoiding future tense in documentation :-) https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode669 scm/stencil.scm:669: @var{stil} is flipped in place. Its position, the I suggest s/./;/
Sign in to reply to this message.
Some nitpicks. Otherwise LGTM https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm File scm/stencil.scm (right): https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode665 scm/stencil.scm:665: (define-public (stencil-flip axis stil) I'd call it 'flip-stencil', would be more in line with the majority of namings here in stencil.scm https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode668 scm/stencil.scm:668: An @var{axis} of Y or 1 will flip it vertically. Why not simply delete these two lines. I feel they are pretty much redundant. https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode670 scm/stencil.scm:670: X and Y coordinates of its bounding box, remains the same." I'd delete "X and Y" in order not to lead to confusing with the meaning of X- and Y-axis in LilyPond
Sign in to reply to this message.
more doc string edits etc.
Sign in to reply to this message.
Here's another round of edits, also: WARNING: could not change issue labels; please email lilypond-devel with the issue number: 4370 No permission to edit issue Thanks, -Paul https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm File scm/stencil.scm (right): https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode665 scm/stencil.scm:665: (define-public (stencil-flip axis stil) On 2015/05/06 23:51:16, thomasmorley651 wrote: > I'd call it 'flip-stencil', would be more in line with the majority of namings > here in stencil.scm Done. I was thinking of ly:stencil-rotate, but you're right most of the names in this file are the other way around, so lets go with that. https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode668 scm/stencil.scm:668: An @var{axis} of Y or 1 will flip it vertically. On 2015/05/06 06:01:30, lemzwerg wrote: > What about: > > Value @code{X} (or @code{0}) for @var{axis} flips it horizontally. > Value @code{Y} (or @code{1}) flips it vertically. > > Exactly defined values for a variable should be represented with `@code'. And > I'm a great fan of avoiding future tense in documentation :-) Done. https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode668 scm/stencil.scm:668: An @var{axis} of Y or 1 will flip it vertically. On 2015/05/06 23:51:15, thomasmorley651 wrote: > Why not simply delete these two lines. > I feel they are pretty much redundant. Well Werner requested the "horizontal" and "vertical" language so I'll leave these lines in. I'm ok with erring on the side of a little redundancy for the sake of clarity. https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode669 scm/stencil.scm:669: @var{stil} is flipped in place. Its position, the On 2015/05/06 06:01:30, lemzwerg wrote: > I suggest s/./;/ Done. https://codereview.appspot.com/235090043/diff/40001/scm/stencil.scm#newcode670 scm/stencil.scm:670: X and Y coordinates of its bounding box, remains the same." On 2015/05/06 23:51:16, thomasmorley651 wrote: > I'd delete "X and Y" in order not to lead to confusing with the meaning of X- > and Y-axis in LilyPond Done.
Sign in to reply to this message.
LGTM. https://codereview.appspot.com/235090043/diff/60001/input/regression/stencil-... File input/regression/stencil-flip.ly (right): https://codereview.appspot.com/235090043/diff/60001/input/regression/stencil-... input/regression/stencil-flip.ly:1: \version "2.19.19" I would rename the file to `flip-stencil.ly' now. And everywhere you need s/stencil-flip/flip-stencil/ of course.
Sign in to reply to this message.
more edits
Sign in to reply to this message.
https://codereview.appspot.com/235090043/diff/60001/input/regression/stencil-... File input/regression/stencil-flip.ly (right): https://codereview.appspot.com/235090043/diff/60001/input/regression/stencil-... input/regression/stencil-flip.ly:1: \version "2.19.19" On 2015/05/07 04:44:53, lemzwerg wrote: > I would rename the file to `flip-stencil.ly' now. And everywhere you need > > s/stencil-flip/flip-stencil/ > > of course. Ah, of course! Done.
Sign in to reply to this message.
Patch on countdown for May 16th
Sign in to reply to this message.
Patch counted down - please push
Sign in to reply to this message.
author Paul Morris <paulwmorris@gmail.com> Tue, 5 May 2015 03:18:15 +0000 (23:18 -0400) committer James Lowe <pkx166h@gmail.com> Sun, 17 May 2015 18:34:15 +0000 (19:34 +0100) commit 51aecfed170349c19e10923c9ce18773ad1786c3 and author Paul Morris <paulwmorris@gmail.com> Tue, 5 May 2015 03:12:52 +0000 (23:12 -0400) committer James Lowe <pkx166h@gmail.com> Sun, 17 May 2015 18:35:40 +0000 (19:35 +0100) commit 5910bd4b7113bc614136480740c53866f3c6c69a
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
