Issue 15060044: Add documentation for the music function \alterBroken to the NR.
|
Created:
10 years, 6 months ago by david.nalesnik Modified:
10 years, 6 months ago CC:
lilypond-devel_gnu.org Base URL:
http://git.savannah.gnu.org/gitweb/?p=lilypond.git/trunk/ Visibility:
Public. |
DescriptionAdd documentation for the music function \alterBroken to the NR.
Issue 3458: Document \alterBroken
Patch Set 1 #
Total comments: 4
Patch Set 2 : changes in response to Trevor's review #Patch Set 3 : small omission #
Total comments: 2
Patch Set 4 : New examples; warning about breaks #MessagesTotal messages: 11
Please review.
Sign in to reply to this message.
On 2013/10/18 22:18:44, david.nalesnik wrote: > Please review. I cannot review _and_ comment inline, when I click on side-by-sdie diffs I get 'error chunk mismatch'. Does anyone else?
Sign in to reply to this message.
2013/10/19 <pkx166h@gmail.com>: > I cannot review _and_ comment inline, when I click on side-by-sdie diffs > I get 'error chunk mismatch'. Does anyone else? me too.
Sign in to reply to this message.
LGTM with a couple of nitpicks and a query (although I haven't tried a make doc to check the appearance of the examples). At first I was a little concerned about the explanations in what is supposed to be a _reference_ manual, but in this case I think they are justified. They are well-written, not overly long, and the command is quite complex. Trevor https://codereview.appspot.com/15060044/diff/1/Documentation/notation/changin... File Documentation/notation/changing-defaults.itely (right): https://codereview.appspot.com/15060044/diff/1/Documentation/notation/changin... Documentation/notation/changing-defaults.itely:4318: "to" -> "on" https://codereview.appspot.com/15060044/diff/1/Documentation/notation/changin... Documentation/notation/changing-defaults.itely:4369: replace full stop by a comma https://codereview.appspot.com/15060044/diff/1/Documentation/notation/changin... Documentation/notation/changing-defaults.itely:4370: drop "to name several" https://codereview.appspot.com/15060044/diff/1/Documentation/notation/changin... Documentation/notation/changing-defaults.itely:4396: Should there be a warning about not using \break to enforce line breaks?
Sign in to reply to this message.
On 2013/10/19 09:00:31, janek wrote: > 2013/10/19 <pkx166h@gmail.com>: > > I cannot review _and_ comment inline, when I click on side-by-sdie diffs > > I get 'error chunk mismatch'. Does anyone else? > > me too. Yes, I also get "error: old chunk mismatch" and this happens to me when I try to view the side-by-sides of some other patches, including the one for issue 3619.
Sign in to reply to this message.
On 2013/10/19 09:50:43, Trevor Daniels wrote: > LGTM with a couple of nitpicks and a query > (although I haven't tried a make doc to > check the appearance of the examples). > > At first I was a little concerned about the > explanations in what is supposed to be a > _reference_ manual, but in this case I think > they are justified. They are well-written, > not overly long, and the command is quite complex. > > Trevor > > https://codereview.appspot.com/15060044/diff/1/Documentation/notation/changin... Trevor, When I uploaded the my changes, your inline comments aren't available unfortunately. (The good news is the side-by-side now is--at least for me.) I'll respond to your comments here. > File Documentation/notation/changing-defaults.itely (right): > > https://codereview.appspot.com/15060044/diff/1/Documentation/notation/changin... > Documentation/notation/changing-defaults.itely:4318: > "to" -> "on" Done. Oddly enough, I had it this way the first time around. > > https://codereview.appspot.com/15060044/diff/1/Documentation/notation/changin... > Documentation/notation/changing-defaults.itely:4369: > replace full stop by a comma Done. You have better eyes than me! (But I was distracted by the perennial question: Oxford comma or no?) > > https://codereview.appspot.com/15060044/diff/1/Documentation/notation/changin... > Documentation/notation/changing-defaults.itely:4370: > drop "to name several" Done. > https://codereview.appspot.com/15060044/diff/1/Documentation/notation/changin... > Documentation/notation/changing-defaults.itely:4396: > Should there be a warning about not using \break > to enforce line breaks? The command will work if the break isn't forced. The \breaks elsewhere are only a consequence of the minimal examples. If you think there might be confusion, I could possibly add a comment to one of the \break lines. "\break added to force a line break on a short line. \alterBroken does not require forced breaks."
Sign in to reply to this message.
LGTM except for one comment. https://codereview.appspot.com/15060044/diff/350001/Documentation/notation/ch... File Documentation/notation/changing-defaults.itely (right): https://codereview.appspot.com/15060044/diff/350001/Documentation/notation/ch... Documentation/notation/changing-defaults.itely:4362: \alterBroken damping #'(10 1) Beam I think this is not a very good example, as it's not immediately obvious what the code did (the change can be overloked). I think it would be better to use something like this: \relative c'' { r2 \alterBroken thickness #'(1 10) Slur c8( d e f \break g8 f e d) r2 }
Sign in to reply to this message.
From: <david.nalesnik@gmail.com> > https://codereview.appspot.com/15060044/diff/1/Documentation/notation/changin... >> Documentation/notation/changing-defaults.itely:4396: >> Should there be a warning about not using \break >> to enforce line breaks? > > The command will work if the break isn't forced. The \breaks elsewhere > are only a consequence of the minimal examples. If you think there > might be confusion, I could possibly add a comment to one of the \break > lines. "\break added to force a line break on a short line. > \alterBroken does not require forced breaks." What I had in mind was a warning that if an explicit \break is not included the break may occur at a different place following changes, and the values may no longer be appropriate.
Sign in to reply to this message.
https://codereview.appspot.com/15060044/diff/350001/Documentation/notation/ch... File Documentation/notation/changing-defaults.itely (right): https://codereview.appspot.com/15060044/diff/350001/Documentation/notation/ch... Documentation/notation/changing-defaults.itely:4362: \alterBroken damping #'(10 1) Beam On 2013/10/19 17:07:55, janek wrote: > I think this is not a very good example, as it's not immediately obvious what > the code did (the change can be overloked). > > I think it would be better to use something like this: > > \relative c'' { > r2 > \alterBroken thickness #'(1 10) Slur > c8( d e f > \break > g8 f e d) r2 > } Done. Yes, much more dramatic, and I like how the input is still simple--numbers for 'thickness. Plus, I know that overrides are frowned on outside of snippets. I changed the dashed curve below to use PhrasingSlur, so we can see the command working with different spanner types.
Sign in to reply to this message.
On 2013/10/19 17:56:28, t.daniels_treda.co.uk wrote: > From: <mailto:david.nalesnik@gmail.com> > > > > https://codereview.appspot.com/15060044/diff/1/Documentation/notation/changin... > >> Documentation/notation/changing-defaults.itely:4396: > >> Should there be a warning about not using \break > >> to enforce line breaks? > > > > The command will work if the break isn't forced. The \breaks elsewhere > > are only a consequence of the minimal examples. If you think there > > might be confusion, I could possibly add a comment to one of the \break > > lines. "\break added to force a line break on a short line. > > \alterBroken does not require forced breaks." > > What I had in mind was a warning that if an explicit \break is not included > the break may occur at a different place following changes, and the > values may no longer be appropriate. Ah, now I see. Warning added.
Sign in to reply to this message.
|
