Issue 3581041: Doc: Various to LM and NR from user email threads (Closed)
|
Created:
14 years, 3 months ago by pkx166h Modified:
14 years, 2 months ago Reviewers:
Valentin Villenave, Neil Puttock, carl.d.sorensen, Trevor Daniels, Graham Percival (old account) CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionDoc: Various to LM and NR from user email threads
Staff.itely (NR)
@knownissues that PianoStaff does not accept ChordNames
http://lists.gnu.org/archive/html/lilypond-user/2010-09/msg00297.html
Spacing.itely (NR)
@warning that odd page numbers are always on the RH page and that you must
have a blank second page if you want music to start on page '1' is you have
a cover page
http://lists.gnu.org/archive/html/lilypond-user/2010-06/msg00475.html
editorial.itely (NR)
@knownissue that using \teeny \tiny \small etc instead of explicit fontSize
will produce inconsistent line spacing comparitively
http://lists.gnu.org/archive/html/lilypond-user/2010-09/msg00665.html
Patch Set 1 #
Total comments: 19
Patch Set 2 : Changes to previous that there's concensus on. Editorial.itely edits moved to text.itely. #
MessagesTotal messages: 14
Hi James All good stuff. One suggested change to the wording to make it less perjorative. http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... File Documentation/notation/repeats.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... Documentation/notation/repeats.itely:153: number of endings. If you must use bar checks then put If you do use bar checks ...
Sign in to reply to this message.
Greetings, it does improve things; there's just one sentence I'm not entirely happy with. http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... File Documentation/notation/repeats.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... Documentation/notation/repeats.itely:151: of and in between the braces of the grouped notes when using This sentence is too long and not very clear. Perhaps some additional punctuation might help? -> "When using @code{@bs{}alternative} endings, do not place bar checks outside of, or in-between, the braces of the grouped notes: otherwise you will not get the expected number of endings." Besides, instead of "the braces of the grouped notes" I'd say "the blocks delimited by braces" or something like that. http://codereview.appspot.com/3581041/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:928: @warning{Odd page numbers are always on the right. If you want the I'm not sure this deserves a @warning. If it does, then another thing I'm not sure of, is whether we want @warning blocks at the end of sections, below @seealso etc. From what I can remember, @warning are generally printed in the main text aren't they?
Sign in to reply to this message.
Looks mostly good. I'd recommend a change to the bar check / alternative ending issue. THanks, Carl http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... File Documentation/notation/repeats.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... Documentation/notation/repeats.itely:150: @rprogram{An extra staff appears}. Do not place bar checks outside I think this is awkward. I think it would be better to do the following: 1) Put barchecks inside the alternatives in the example. 2) Show an example of the general case, which is that the first two music expressions following alternative, rather than the first two braced expressions following alternative, are taken as the alternatives. For example, it might be something like \alternative {c4 c c c |} \markup "This is an expression" {d4 d d d |} which will have the same effect as the bad bar check. In other words, it's not a bar check specific problem. http://codereview.appspot.com/3581041/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:928: @warning{Odd page numbers are always on the right. If you want the I think that this should be a known issue, rather than a warning.
Sign in to reply to this message.
Thanks for this! I've slightly modified the git-cl instructions, to include "make the CC list lilypond-devel". That's part of the git-cl config stage, so you don't need to remember it for every individual patch you upload. http://codereview.appspot.com/3581041/diff/1/Documentation/learning/fundament... File Documentation/learning/fundamental.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/learning/fundament... Documentation/learning/fundamental.itely:2113: @warning{The @code{Stem_engraver} and @code{Beam_engraver} attach their So far, we don't have any @warning inside the @seealso section. I'd say that unless you're specifically warning about one of our links (dunno what that might mean... "warning: this link might not work?" :), this should either be higher up in the section, or be a known issue. Since you're talking about removing an engraver (which isn't really standard NR-level stuff), I'd suggest making it a knownissue rather than a warning. http://codereview.appspot.com/3581041/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:928: @warning{Odd page numbers are always on the right. If you want the On 2010/12/11 00:36:29, Carl wrote: > I think that this should be a known issue, rather than a warning. Agreed, this is definitely better as a known issue. http://codereview.appspot.com/3581041/diff/1/Documentation/notation/staff.itely File Documentation/notation/staff.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/staff.ite... Documentation/notation/staff.itely:278: @code{PianoStaff} does not, by default, accept @code{ChordNames}. This should be a known issue, instead of a warn... oh, oops, sorry, it's already a known issue. :)
Sign in to reply to this message.
I hope this is the right method to have a discussion on Rietveld, I have added my own comments/responses to yours inline. James http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... File Documentation/notation/repeats.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... Documentation/notation/repeats.itely:150: @rprogram{An extra staff appears}. Do not place bar checks outside On 2010/12/11 00:36:29, Carl wrote: > I think this is awkward. > > I think it would be better to do the following: > > 1) Put barchecks inside the alternatives in the example. I know that we were trying to avoid bar checks on single measures - this was/is CG policy and bar checks are not mandatory anyway (helpful but not a requirement) so add nothing to an example other than this one case. If I put them here, then I have to put them throughout the other examples. I take the point that you might think it better to show an example which I could for instance with an @example but I'd like some opinion on that because we could then start setting precedence on showing what NOT to do if you see what I mean? > > 2) Show an example of the general case, which is that the first two music > expressions following alternative, rather than the first two braced expressions > following alternative, are taken as the alternatives. For example, it might be > something like > > \alternative > {c4 c c c |} > \markup "This is an expression" > {d4 d d d |} > > which will have the same effect as the bad bar check. In other words, it's not > a bar check specific problem. http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... Documentation/notation/repeats.itely:151: of and in between the braces of the grouped notes when using On 2010/12/10 23:56:08, Valentin Villenave wrote: > This sentence is too long and not very clear. Perhaps some additional > punctuation might help? > > -> "When using @code{@bs{}alternative} endings, do not place bar checks outside > of, or in-between, the braces of the grouped notes: otherwise you will not get > the expected number of endings." > > Besides, instead of "the braces of the grouped notes" I'd say "the blocks > delimited by braces" or something like that. Valentin, I have tried to be consistent with the other descriptions above for the different cases of \alternative repeats. We don't use 'blocks delimited..' there they are called 'grouped notes' and the word 'braces' is used. Blocks is too vague and could for instance refer to the whole 'block' of the \alternative {..{...} {...} } construct rather than the groups of notes within the \alternative { .. }
Sign in to reply to this message.
On 2010/12/11 16:33:48, pkx166h wrote: > I hope this is the right method to have a discussion on Rietveld, I have added > my own comments/responses to yours inline. Yes, this is a very good way to do it. You can also just add a comment in the patchset. http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... > Documentation/notation/repeats.itely:150: @rprogram{An extra staff appears}. Do > not place bar checks outside > On 2010/12/11 00:36:29, Carl wrote: > > I think this is awkward. > > > > I think it would be better to do the following: > > > > 1) Put barchecks inside the alternatives in the example. > > I know that we were trying to avoid bar checks on single measures - this was/is > CG policy But this isn't a single measure. There are three measures in the example. Bar checks *should* be used here, although they're not required. > and bar checks are not mandatory anyway (helpful but not a > requirement) so add nothing to an example other than this one case. If I put > them here, then I have to put them throughout the other examples. In my opinion, we should be moving toward bar checks in examples where it is helpful, rather than as a mindless policy requiring consistency in all cases. This particular case is a case where a user made a mistake by copying an example and doing what he thought was right. If the bar check were inside the alternative, the mistake would not have been made. So I think it's a positive change. > I take the > point that you might think it better to show an example which I could for > instance with an @example but I'd like some opinion on that because we could > then start setting precedence on showing what NOT to do if you see what I mean? To me the precedent is set by virtue of the fact that a user made the mistake. I agree we don't want to show what not to do in general. We could probably have avoided the problem if the bar check were just in the alternatives.
Sign in to reply to this message.
http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... File Documentation/notation/repeats.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... Documentation/notation/repeats.itely:151: of and in between the braces of the grouped notes when using On 2010/12/11 16:33:48, pkx166h wrote: > Valentin, I have tried to be consistent with the other descriptions above for > the different cases of \alternative repeats. We don't use 'blocks delimited..' > there they are called 'grouped notes' and the word 'braces' is used. Point taken. But my main concern was about punctuation, not wording :-)
Sign in to reply to this message.
Sorry I didn't read Graham;s comment first time round. http://codereview.appspot.com/3581041/diff/1/Documentation/learning/fundament... File Documentation/learning/fundamental.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/learning/fundament... Documentation/learning/fundamental.itely:2113: @warning{The @code{Stem_engraver} and @code{Beam_engraver} attach their On 2010/12/11 14:01:10, Graham Percival wrote: > So far, we don't have any @warning inside the @seealso section. I'd say that > unless you're specifically warning about one of our links (dunno what that might > mean... "warning: this link might not work?" :), this should either be higher up > in the section, or be a known issue. Yes sorry I meant this to be an @knownissue then changed my mind but not the position. > > Since you're talking about removing an engraver (which isn't really standard > NR-level stuff), I'd suggest making it a knownissue rather than a warning. This is LM Graham. Not that that makes a difference to me if you want it to be @warning or @knownissue. We do mention how to remove engravers/contexts in the LM as a gentle introduction, and I was thinking that 'removing the noteheads' is kind of what anyone new to LP might do, just because they can...but also we show in the LM how to create scores for 'teaching music' (remove bars for putting back in, remove notes to teach rhythm etc) so I thought this appropriate in the LM, but your comment about thinking it was NR now leads me to think maybe it should be in there instead? james
Sign in to reply to this message.
http://codereview.appspot.com/3581041/diff/1/Documentation/learning/fundament... File Documentation/learning/fundamental.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/learning/fundament... Documentation/learning/fundamental.itely:2115: no note heads are produced and so no Stem or Beams are created either.} stems or beams http://codereview.appspot.com/3581041/diff/1/Documentation/notation/editorial... File Documentation/notation/editorial.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/editorial... Documentation/notation/editorial.itely:145: using the @code{fontSize} property. This is a problem in markup only, thus applies to the \fontsize markup command (not the context property fontSize). I suggested changing the definitions of the named commands to remove the inconsistency (via \abs-fontsize), but if we'd rather leave them as they are this knowissue should be moved to text.itely.
Sign in to reply to this message.
Lots of good discussion here, but I'm finding a bit confusing. James, could you extract whatever part(s) of your first patch that everybody agreed on, and make a separate patch that only does those? That way, we can get the "obvious" stuff pushed, and then focus on whatever "not obvious" stuff is left. I'd also suggest that once the "obvious" stuff is pushed, you create a patch for only 1 of the remaining files -- that will let us focus even more. Remember that patches should not only *be* good, they must be *seen* to be good. When there's a lot of unrelated stuff in the same patch, it's harder to see that stuff is good.
Sign in to reply to this message.
http://codereview.appspot.com/3581041/diff/1/Documentation/learning/fundament... File Documentation/learning/fundamental.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/learning/fundament... Documentation/learning/fundamental.itely:2115: no note heads are produced and so no Stem or Beams are created either.} On 2010/12/11 22:56:02, Neil Puttock wrote: > stems or beams Leaving this file unedited for another patch until those other files that have consensus have been pushed. http://codereview.appspot.com/3581041/diff/1/Documentation/notation/editorial... File Documentation/notation/editorial.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/editorial... Documentation/notation/editorial.itely:145: using the @code{fontSize} property. On 2010/12/11 22:56:02, Neil Puttock wrote: > This is a problem in markup only, thus applies to the \fontsize markup command > (not the context property fontSize). > > I suggested changing the definitions of the named commands to remove the > inconsistency (via \abs-fontsize), but if we'd rather leave them as they are > this knowissue should be moved to text.itely. Done. http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... File Documentation/notation/repeats.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/repeats.i... Documentation/notation/repeats.itely:153: number of endings. If you must use bar checks then put On 2010/12/10 23:41:50, Trevor Daniels wrote: > If you do use bar checks ... Leaving this file unedited for another patch until those other files that have consensus have been pushed. http://codereview.appspot.com/3581041/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:928: @warning{Odd page numbers are always on the right. If you want the On 2010/12/11 14:01:10, Graham Percival wrote: > On 2010/12/11 00:36:29, Carl wrote: > > I think that this should be a known issue, rather than a warning. > > Agreed, this is definitely better as a known issue. Done. http://codereview.appspot.com/3581041/diff/1/Documentation/notation/staff.itely File Documentation/notation/staff.itely (right): http://codereview.appspot.com/3581041/diff/1/Documentation/notation/staff.ite... Documentation/notation/staff.itely:278: @code{PianoStaff} does not, by default, accept @code{ChordNames}. On 2010/12/11 14:01:10, Graham Percival wrote: > This should be a known issue, instead of a warn... oh, oops, sorry, it's already > a known issue. :) Done.
Sign in to reply to this message.
Is this ok to push? James
Sign in to reply to this message.
Looks fine to me.
Sign in to reply to this message.
On 2010/12/14 22:45:26, Trevor Daniels wrote: > Looks fine to me. LGTM too. James, could you send the git format-patch to Trevor, who will push it for you? Cheers, - Graham
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
