Issue 560670043: Document bound-details (sub-)properties in line-spanner-cc for IR (Closed)
|
Created:
4 years, 11 months ago by thomasmorley651 Modified:
4 years, 11 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionDocument bound-details (sub-)properties in line-spanner-cc for IR
Patch Set 1 #
Total comments: 13
Patch Set 2 : Adress Werner and David #
Total comments: 2
Patch Set 3 : Werners recent comments (hopefully) #Patch Set 4 : Corrrect misleading doc of stencil-align-dir-y/stencil-offset #
Total comments: 6
Patch Set 5 : Werner's recent comments #MessagesTotal messages: 15
Please review
Sign in to reply to this message.
LGTM, thanks. Only cosmetic nits :-) https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc File lily/line-spanner.cc (right): https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc#... lily/line-spanner.cc:395: "Sets the Y-coordinate of the end point, in staff-spaces" I think this should rather be Sets the y@tie{}coordinate..., in staff spaces. Ditto in other places. https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc#... lily/line-spanner.cc:398: " vertical center of the note head.\n" The final `\n' here (and in similar places) in the middle of a paragraph doesn't have any effect. texinfo simply slurps it. Only two `\n' in a row to indicate a paragraph end are honored. https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc#... lily/line-spanner.cc:404: " So, a value of -1 (or LEFT) makes the line start/end at" ... or @code{LEFT} ... https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc#... lily/line-spanner.cc:416: "@item stencil-offset\n" @itemx stencil-offset\n https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc#... lily/line-spanner.cc:418: " end-point, centered on the line, as defined by the X and" end point, ... by the @code{X} and @code{Y} ... https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc#... lily/line-spanner.cc:420: " stencil-offset will move the symbol at the edge vertically" Setting either @code{...} or @code{...}
Sign in to reply to this message.
https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc File lily/line-spanner.cc (right): https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc#... lily/line-spanner.cc:395: "Sets the Y-coordinate of the end point, in staff-spaces" On 2019/05/10 05:22:32, lemzwerg wrote: > I think this should rather be > > Sets the y@tie{}coordinate..., in staff spaces. > > Ditto in other places. I think I'd at least stick with uppercase since it evokes the #X and 'X-offset and other programmatic use of X inside of LilyPond. Other than that, agree.
Sign in to reply to this message.
> I think I'd at least stick with uppercase since it evokes the #X > and 'X-offset and other programmatic use of X inside of LilyPond. Mhmm, I rather disagree, but it's a really minor nit.
Sign in to reply to this message.
Adress Werner and David
Sign in to reply to this message.
https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc File lily/line-spanner.cc (right): https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc#... lily/line-spanner.cc:395: "Sets the Y-coordinate of the end point, in staff-spaces" On 2019/05/10 10:33:12, dak wrote: > On 2019/05/10 05:22:32, lemzwerg wrote: > > I think this should rather be > > > > Sets the y@tie{}coordinate..., in staff spaces. > > > > Ditto in other places. > > I think I'd at least stick with uppercase since it evokes the #X and 'X-offset > and other programmatic use of X inside of LilyPond. Other than that, agree. Y@tie{}coordinate etc is used now, here and in other places. Done. https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc#... lily/line-spanner.cc:398: " vertical center of the note head.\n" On 2019/05/10 05:22:31, lemzwerg wrote: > The final `\n' here (and in similar places) in the middle of a paragraph doesn't > have any effect. texinfo simply slurps it. Only two `\n' in a row to indicate > a paragraph end are honored. It was built after slur.cc. Thus I guess many `\n' there could be deleted as well. Corrected here. https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc#... lily/line-spanner.cc:404: " So, a value of -1 (or LEFT) makes the line start/end at" On 2019/05/10 05:22:32, lemzwerg wrote: > ... or @code{LEFT} ... Done. https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc#... lily/line-spanner.cc:416: "@item stencil-offset\n" On 2019/05/10 05:22:32, lemzwerg wrote: > @itemx stencil-offset\n I didn't know about @itemx. Done. https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc#... lily/line-spanner.cc:418: " end-point, centered on the line, as defined by the X and" On 2019/05/10 05:22:32, lemzwerg wrote: > end point, ... by the @code{X} and @code{Y} ... Done. https://codereview.appspot.com/560670043/diff/550690043/lily/line-spanner.cc#... lily/line-spanner.cc:420: " stencil-offset will move the symbol at the edge vertically" On 2019/05/10 05:22:31, lemzwerg wrote: > Setting either @code{...} or @code{...} Done.
Sign in to reply to this message.
https://codereview.appspot.com/560670043/diff/552680043/lily/line-spanner.cc File lily/line-spanner.cc (right): https://codereview.appspot.com/560670043/diff/552680043/lily/line-spanner.cc#... lily/line-spanner.cc:421: " of the line" Oh, here (and at similar places) you should retain the `\n' since `@item' and similar texinfo commands must start a line. https://codereview.appspot.com/560670043/diff/552680043/lily/line-spanner.cc#... lily/line-spanner.cc:428: "@end table", Ditto, to assure that `@end table' stays on a line of its own. (Well, it would probably not be necessary since the docstring extraction code inserts a newline anyway, I guess, but...)
Sign in to reply to this message.
Werners recent comments (hopefully)
Sign in to reply to this message.
On 2019/05/11 03:55:46, lemzwerg wrote: > https://codereview.appspot.com/560670043/diff/552680043/lily/line-spanner.cc > File lily/line-spanner.cc (right): > > https://codereview.appspot.com/560670043/diff/552680043/lily/line-spanner.cc#... > lily/line-spanner.cc:421: " of the line" > Oh, here (and at similar places) you should retain the `\n' since `@item' and > similar texinfo commands must start a line. > > https://codereview.appspot.com/560670043/diff/552680043/lily/line-spanner.cc#... > lily/line-spanner.cc:428: "@end table", > Ditto, to assure that `@end table' stays on a line of its own. (Well, it would > probably not be necessary since the docstring extraction code inserts a newline > anyway, I guess, but...) Done (hopefully).
Sign in to reply to this message.
LGTM, thanks.
Sign in to reply to this message.
Corrrect misleading doc of stencil-align-dir-y/stencil-offset
Sign in to reply to this message.
https://codereview.appspot.com/560670043/diff/582680043/lily/line-spanner.cc File lily/line-spanner.cc (right): https://codereview.appspot.com/560670043/diff/582680043/lily/line-spanner.cc#... lily/line-spanner.cc:421: " @code{stencil-offset}, expecting a number-pair, the stencil" s/number-pair/number pair/ https://codereview.appspot.com/560670043/diff/582680043/lily/line-spanner.cc#... lily/line-spanner.cc:422: " is moved in X@tie{}axis direction according to the first" s/in X@tie{}axis direction/along the X@tie{}axis/ https://codereview.appspot.com/560670043/diff/582680043/lily/line-spanner.cc#... lily/line-spanner.cc:423: " value, the second value moves in Y@tie{}axis direction.\n" s/.../value; the second value moves the stencil along the Y@tie{}axis.\n/
Sign in to reply to this message.
Werner's recent comments
Sign in to reply to this message.
https://codereview.appspot.com/560670043/diff/582680043/lily/line-spanner.cc File lily/line-spanner.cc (right): https://codereview.appspot.com/560670043/diff/582680043/lily/line-spanner.cc#... lily/line-spanner.cc:421: " @code{stencil-offset}, expecting a number-pair, the stencil" On 2019/05/14 03:52:11, lemzwerg wrote: > s/number-pair/number pair/ Done. https://codereview.appspot.com/560670043/diff/582680043/lily/line-spanner.cc#... lily/line-spanner.cc:422: " is moved in X@tie{}axis direction according to the first" On 2019/05/14 03:52:11, lemzwerg wrote: > s/in X@tie{}axis direction/along the X@tie{}axis/ Done. https://codereview.appspot.com/560670043/diff/582680043/lily/line-spanner.cc#... lily/line-spanner.cc:423: " value, the second value moves in Y@tie{}axis direction.\n" On 2019/05/14 03:52:11, lemzwerg wrote: > s/.../value; the second value moves the stencil along the mailto:Y@tie{}axis.\n/ Done.
Sign in to reply to this message.
|
