Issue 2642043: Doc: NR 4.4.1: Rewrite. (Closed)
|
Created:
14 years, 6 months ago by Mark Polesky Modified:
14 years, 5 months ago CC:
lilypond-devel_gnu.org, carl.d.sorensen_gmail.com, joeneeman Visibility:
Public. |
DescriptionDoc: NR 4.4.1: Rewrite.
Patch Set 1 #
Total comments: 45
Patch Set 2 : Make requested changes. #
Total comments: 19
Patch Set 3 : More requested changes. #
Total comments: 1
MessagesTotal messages: 21
Here's a completely rewritten version of NR 4.4.1 Vertical spacing inside a system Comments? Thanks. - Mark
Sign in to reply to this message.
Mark, I think this is a great start, and will greatly help. The major issues I see are (1) repeating information (i.e. the meanings of space, minimum-distance, padding, and stretchability), and (2) introducing exhaustive lists into the NR. Repeating information is prohibited by policy because it becomes very difficult to keep two parallel sections in synch. Exhaustive lists belong in the IR, instead of the NR. THat way they are automatically updated. Other than that, I have some editorial comments. Great work! Thanks, Carl http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (left): http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1697: A non-staff line at the bottom of a system should have I think that this section should not be removed. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1501: @item @emph{staff-like contexts} (such as @code{Lyrics}). I would change this to "non-staff" contexts, I think. I liked the previous terminology. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1506: I think that you should explain the different grobs that are created by each of the contexts at this point. I got lost while I was waiting. I'd like to see: Staff contexts create VerticalAxisGrouper grobs StaffGroup contexts create StaffGrouper grobs non-staff contexts create VerticalAxisGrouper grobs Or maybe the word is "result in" instead of create. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1510: the staves. WHen do the staff-groups get spaced? http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1529: @subsubheading Structure of spacing alists for grob properties "Structure of alists for vertical spacing properties" is a better name, I think. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1554: @item @code{minimum-distance} -- the minimum required vertical I think I would change "minimum required" to "minimum allowable" http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1578: @subsubheading Modifying spacing alists for grob properties Reference to the between system spacing, instead of repeating. Just introduce the new syntax as it applies to the in-system contexts. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1814: @item @code{ChoirStaff} In general, we don't want to make exhaustive lists in the docs, because maintaining them is a problem. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1836: The default value of @code{next-staff-spacing} is a callback This is too much talking through the code, I think. Better to just say next-staff-spacing is a procedure that will return after-last-staff-spacing if the staff is the last staff of a group, the between-staff-spacing if the staff is not the last staff of a group, and the default-next-staff-spacing if the staff is not part of a group. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1881: @item @code{ChordNames} I don't think we want exhaustive lists. (But Graham will correct me if I'm wrong.) http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1897: @item @code{inter-loose-line-spacing} Since the property name is inter-loose-line-spacing, we may want to call these contexts "loose line contexts" instead of staff-like or non-staff contexts.
Sign in to reply to this message.
On 2010/10/23 00:51:31, Carl wrote: > Mark, > > I think this is a great start, and will greatly help. > > The major issues I see are (1) repeating information > (i.e. the meanings of space, minimum-distance, padding, > and stretchability), and (2) introducing exhaustive lists > into the NR. Carl, (1) repeating information -- I'll try to fix this. (1) repeating information -- I'll try to fix this. (2) exhaustive lists -- see my replies to your code comments. Thanks for looking over this so thoroughly! - Mark http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (left): http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1697: A non-staff line at the bottom of a system should have On 2010/10/23 00:51:31, Carl wrote: > I think that this section should not be removed. I moved it to the staff-affinity description (l.1674). http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1501: @item @emph{staff-like contexts} (such as @code{Lyrics}). On 2010/10/23 00:51:31, Carl wrote: > I would change this to "non-staff" contexts, I think. I > liked the previous terminology. The original "non-staff lines" is still better than "non-staff contexts" (which can imply Voice and Score), but "non-staff lines" reads like "the opposite of staff lines", which is confusing. I suppose defining the term clearly at the top will help. If I change it back to "non-staff lines", I should change the others to "staves" and "staff-groups". http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1506: 23 00:51:31, Carl wrote: > I think that you should explain the different grobs that > are created by each of the contexts at this point. I got > lost while I was waiting. Already in the intro? I think just after the "Inter-system spacing properties" heading is a better place. I'd prefer to just change the previous sentence to: "Each of these context types uses a slightly different method for specifying the vertical spacing, which will be explained below." Did you really get lost while you were waiting? And do users really need to know that different contexts create different grobs? Wouldn't that be "talking through the code" a little too much? Also, even though staff-group contexts don't create the VerticalAxisGroup grob, their constituent staves do, and this in turn influences their spacing. I think your suggestion might be a little misleading. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1510: the staves. On 2010/10/23 00:51:31, Carl wrote: > When do the staff-groups get spaced? This is trivial fix; I'll put it in the next patch set. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1554: @item @code{minimum-distance} -- the minimum required vertical On 2010/10/23 00:51:31, Carl wrote: > I think I would change "minimum required" to "minimum > allowable" Huh. For me, the phrase associations are "minimum required" and "maximum allowable". Such as "you must have at least x" and "you are allowed no more than y". http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1578: @subsubheading Modifying spacing alists for grob properties On 2010/10/23 00:51:31, Carl wrote: > Reference to the between system spacing, instead of > repeating. Just introduce the new syntax as it applies to > the in-system contexts. You mean a link to NR 4.1.2 "Modifying spacing alists for \paper variables"? That's not a bad idea, but that section is a texinfo "subsubheading", which IIRC can't be a cross-referencable node. I'll look into this. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1836: The default value of @code{next-staff-spacing} is a callback On 2010/10/23 00:51:31, Carl wrote: > This is too much talking through the code, I think. I mostly copied this from the previous version. I explained this from a user's perspective in the property descriptions above, so I'm fine with just removing this paragraph. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1881: @item @code{ChordNames} On 2010/10/23 00:51:31, Carl wrote: > I don't think we want exhaustive lists. (But Graham will > correct me if I'm wrong.) Hmm. One argument for an exhaustive list would be that it's just good to know that FretBoards and FiguredBass are "non-staff lines" with spacing that's just as customizable as Lyrics. Perhaps the ideal solution is to include a sentence in each of the sections that describe these contexts, such as: "For purposes of vertical spacing, FigureBass contexts are considered non-staff lines. See `Spacing of non-staff lines'." But I still like the appearance of FretBoards and the others alongside Lyrics and ChordNames. What if I just say: "Examples of non-staff lines include: ChordNames, Dynamics, FiguredBass, FretBoards, Lyrics, and NoteNames." That way, if a context is added to the code base later, it doesn't invalidate the sentence, since it doesn't claim to be exhaustive. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1897: @item @code{inter-loose-line-spacing} On 2010/10/23 00:51:31, Carl wrote: > Since the property name is inter-loose-line-spacing, we > may want to call these contexts "loose line contexts" > instead of staff-like or non-staff contexts. Um, I don't want to scare anyone, but I'll be proposing some name changes for these properties, too. And I currently prefer "non-staff lines" over "loose line contexts". But let's not get into that just yet.
Sign in to reply to this message.
More comments inlined. Thanks, Carl http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1501: @item @emph{staff-like contexts} (such as @code{Lyrics}). On 2010/10/23 05:01:51, Mark Polesky wrote: > On 2010/10/23 00:51:31, Carl wrote: > > I would change this to "non-staff" contexts, I think. I > > liked the previous terminology. > > The original "non-staff lines" is still better than > "non-staff contexts" (which can imply Voice and Score), but > "non-staff lines" reads like "the opposite of staff lines", > which is confusing. I suppose defining the term clearly at > the top will help. If I change it back to "non-staff > lines", I should change the others to "staves" and > "staff-groups". I don't think it's confusing at all. staff contexts are contexts that are displayed in a staff. staff-group contexts are contexts that are displayed in a group of staves non-staff contexts are contexts that are displayed in something besides a staff. Voice is a context that is displayed in a staff. Score is not displayed in anything, as far as this is concerned. Also, we're not talking about non-Staff contexts, we're talking about non-staff contexts. The capital letter is important (and it can be explained here if we need to). http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1506: On 2010/10/23 05:01:51, Mark Polesky wrote: > 23 00:51:31, Carl wrote: > > I think that you should explain the different grobs that > > are created by each of the contexts at this point. I got > > lost while I was waiting. > > Already in the intro? I think just after the "Inter-system > spacing properties" heading is a better place. I'd prefer > to just change the previous sentence to: No, not already in the intro, but before I got to the real explanation. You tell me about three different kinds of contexts, then you go talk about the different spacing properties in terms of Grobs -- VerticalAxisGroup and StaffGrouper. But I don't know what those mean in terms of staff-group, staff, and non-staff contexts that you've already explained. It's not until line 1746 (240 lines, or 4 pages + later) that you explain that VerticalAxisGroup properties apply to ungrouped staff contexts, and so forth. If you're going to tell me about the three kinds of contexts, give me some clues so I can understand the next stuff I'm reading. > Also, even though staff-group > contexts don't create the VerticalAxisGroup grob, their > constituent staves do, and this in turn influences their > spacing. I think your suggestion might be a little > misleading. No, because later on you describe the full spacing algorithm. The VerticalAxisGroup properties control the spacing of individual staves. The StaffGrouper properties control the spacing of groups of staves. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1554: @item @code{minimum-distance} -- the minimum required vertical On 2010/10/23 05:01:51, Mark Polesky wrote: > On 2010/10/23 00:51:31, Carl wrote: > > I think I would change "minimum required" to "minimum > > allowable" > That's true if you're starting from zero, I think. But if you're starting from a big number, and trying to squeeze it down, (which is what we're doing), then I think the "minimum allowable" is more descriptive. But I don't feel strongly about this. > Huh. For me, the phrase associations are "minimum required" > and "maximum allowable". Such as "you must have at least x" > and "you are allowed no more than y". http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1578: @subsubheading Modifying spacing alists for grob properties On 2010/10/23 05:01:51, Mark Polesky wrote: > On 2010/10/23 00:51:31, Carl wrote: > > Reference to the between system spacing, instead of > > repeating. Just introduce the new syntax as it applies to > > the in-system contexts. > > You mean a link to NR 4.1.2 "Modifying spacing alists for > \paper variables"? That's not a bad idea, but that section > is a texinfo "subsubheading", which IIRC can't be a > cross-referencable node. I'll look into this. Great. We should make it a node. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1881: @item @code{ChordNames} On 2010/10/23 05:01:51, Mark Polesky wrote: > Hmm. One argument for an exhaustive list would be that it's > just good to know that FretBoards and FiguredBass are > "non-staff lines" with spacing that's just as customizable > as Lyrics. Perhaps the ideal solution is to include a > sentence in each of the sections that describe these > contexts, such as: > > "For purposes of vertical spacing, FigureBass contexts are > considered non-staff lines. See `Spacing of non-staff > lines'." > > But I still like the appearance of FretBoards and the others > alongside Lyrics and ChordNames. What if I just say: > > "Examples of non-staff lines include: ChordNames, Dynamics, > FiguredBass, FretBoards, Lyrics, and NoteNames." > > That way, if a context is added to the code base later, it > doesn't invalidate the sentence, since it doesn't claim to be > exhaustive. THat would be better. It would be even better if we gave the user a clue as to how to find an exhaustive list in the IR, e.g. all contexts that create a VerticalAxisGroup but not a StaffSymbol (I'm not sure if this is the right criterion or not). http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1897: @item @code{inter-loose-line-spacing} On 2010/10/23 05:01:51, Mark Polesky wrote: > On 2010/10/23 00:51:31, Carl wrote: > > Since the property name is inter-loose-line-spacing, we > > may want to call these contexts "loose line contexts" > > instead of staff-like or non-staff contexts. > > Um, I don't want to scare anyone, but I'll be proposing some > name changes for these properties, too. And I currently > prefer "non-staff lines" over "loose line contexts". But > let's not get into that just yet. I'm fine to defer it, but we should be consistent when it finally gets done.
Sign in to reply to this message.
Looks pretty good, Mark. I've suggested a couple of changes near the top. Trevor http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1496: flexible vertical spacing: Within systems, the printed output is considered to consist of three types for vertical spacing purposes: http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1501: @item @emph{staff-like contexts} (such as @code{Lyrics}). Just to add my 2 cents: I don't really like using "contexts" here. We're talking about printed output, not internal objects, and it's the use of "contexts" that introduces the confusion. So I'd prefer to see the simpler "staves" or "staff lines", "staff groups", and "non-staff lines" throughout. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1505: specifying the vertical spacing. The vertical spacing of each of these types is specified in slightly different ways.
Sign in to reply to this message.
http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1624: size) will always reset all its default key-values. Everything from the subsubheading until here applies to any grob property with an alist (eg. Beam 'details, Slur 'details, NonMusicalPaperColumn 'line-break-system-details). It may be more appropriate (long-term) to put this detailed discussion elsewhere. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1662: @code{after-last-staff-spacing}). I find this wording a little confusing, since \override VerticalAxisGroup #'next-staff-spacing ... has a quite different effect from \override StaffGrouper #'... That is, overriding next-staff-spacing does not "override" any StaffGrouper properties for the usual use of "override" in lilypond. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1672: system. default-next-staff-spacing also applies to staves which are not part of a staff group http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1679: either side. Adjacent staff-like contexts should have I'm not sure how precise you want to be here, but this isn't quite true: if the upper staff, for example, has a very low note then a lyrics line with CENTER affinity might be placed closer to the lower staff. Also, by "equidistant between the ... staves," you mean "equidistant between the refpoints of the staves," right?
Sign in to reply to this message.
http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1624: size) will always reset all its default key-values. On 2010/10/26 22:28:16, joeneeman wrote: > Everything from the subsubheading until here applies to > any grob property with an alist (eg. Beam 'details, Slur > 'details, NonMusicalPaperColumn > 'line-break-system-details). It may be more appropriate > (long-term) to put this detailed discussion elsewhere. Good idea. Can anyone think of a good place for this? http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1662: @code{after-last-staff-spacing}). On 2010/10/26 22:28:16, joeneeman wrote: > I find this wording a little confusing, since \override > VerticalAxisGroup #'next-staff-spacing ... has a quite > different effect from \override StaffGrouper #'... > > That is, overriding next-staff-spacing does not "override" > any StaffGrouper properties for the usual use of > "override" in lilypond. I see. What's a better way to word it? http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1679: either side. Adjacent staff-like contexts should have On 2010/10/26 22:28:16, joeneeman wrote: > I'm not sure how precise you want to be here, but this > isn't quite true: if the upper staff, for example, has a > very low note then a lyrics line with CENTER affinity > might be placed closer to the lower staff. > > Also, by "equidistant between the ... staves," you mean > "equidistant between the refpoints of the staves," right? That's what I meant, but based on your previous statement, I assume even that would be incorrect. In fact, this is clearly wrong, since the refpoints of the staves are the midlines, right? So is it centered between the skylines?
Sign in to reply to this message.
http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1509: available. Then, the staff-like contexts are distributed between This seems to say that staff-like contexts cannot influence the spacing of staffs. Line 1711 about non-affinity-spacing, though, implies that staff-like contexts can influence the spacing to both their neighbors, pushing them apart if need be. (I do not yet understand the behavior well enough to suggest better text.) http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1513: * Inter-system spacing properties:: Within-system http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1520: @node Inter-system spacing properties Within-system http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1624: size) will always reset all its default key-values. On 2010/10/27 08:44:41, Mark Polesky wrote: > Can anyone think of a good place for this? End of section 5.3.1 Overview of modifying properties http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1662: @code{after-last-staff-spacing}). On 2010/10/27 08:44:41, Mark Polesky wrote: > What's a better way to word it? "... leave this property unset, because if set it will be used instead of any StaffGrouper property that would have otherwise applied."
Sign in to reply to this message.
http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1662: @code{after-last-staff-spacing}). On 2010/10/29 04:12:45, Keith wrote: > On 2010/10/27 08:44:41, Mark Polesky wrote: > > What's a better way to word it? > "... leave this property unset, because if set it will be used instead of any > StaffGrouper property that would have otherwise applied." There are some situations where you might want to override this, so I'm not sure if the docs have such a blanket statement. The point is that all of the logic involving StaffGrouper and its spacing variables is contained in the default callback associated with next-staff-spacing (ly:axis-group-interface::calc-next-staff-spacing). If you override that default callback, you will disable that logic for the relevant staff. However (and this is the source of my original comment) you will not actually override any of the properties in StaffGrouper, as the following example demonstrates: \new StaffGroup << % The override doesn't change the value of anything in % StaffGrouper. You can see this because the original % value of between-staff-spacing is used for all staves % but the first. Also, you can't achieve this effect by % changing 'default-next-staff-spacing, which shows that it % is sometimes useful to override 'next-staff-spacing. \new Staff \with { \override VerticalAxisGroup #'next-staff-spacing #'space = #30 } { c'1 } \new Staff { c'1 } \new Staff { c'1 } >> The above is probably an excessively long explanation for the docs. I would suggest something like "...since setting it will prevent StaffGrouper variables (such as ...) from having any effect on the current staff." http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1679: either side. Adjacent staff-like contexts should have On 2010/10/27 08:44:41, Mark Polesky wrote: > On 2010/10/26 22:28:16, joeneeman wrote: > > I'm not sure how precise you want to be here, but this > > isn't quite true: if the upper staff, for example, has a > > very low note then a lyrics line with CENTER affinity > > might be placed closer to the lower staff. > > > > Also, by "equidistant between the ... staves," you mean > > "equidistant between the refpoints of the staves," right? > > That's what I meant, but based on your previous statement, I > assume even that would be incorrect. In fact, this is > clearly wrong, since the refpoints of the staves are the > midlines, right? So is it centered between the skylines? It is "centered" between refpoints in the sense that its desired position is for its refpoint to be exactly centered between the refpoints of the surrounding staves. But it may not achieve this desired position due to other constraints like collisions or minimum-distances.
Sign in to reply to this message.
http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1501: @item @emph{staff-like contexts} (such as @code{Lyrics}). On 2010/10/23 07:05:35, Trevor Daniels wrote: > So I'd prefer to see the simpler "staves" or "staff lines", "staff > groups", and "non-staff lines" throughout. and Keith very much agrees. http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... Documentation/notation/spacing.itely:1509: available. Then, the staff-like contexts are distributed between After experimentation, this is clear. What I needed to know was "First, the staves are spaced to fill the space available, allowing the space required for any interleaving non-staff lines. Then, the non-staff lines are spaced between the staves." I say 'space required' to mean the hard distances of 'minimum-distance and 'padding, because the reader might not have seen these property names yet.
Sign in to reply to this message.
On 29/10/10 05:12, k-ohara5a5a@oco.net wrote: > > http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... > > File Documentation/notation/spacing.itely (right): > > http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... > > Documentation/notation/spacing.itely:1509: available. Then, the > staff-like contexts are distributed between > This seems to say that staff-like contexts cannot influence the spacing > of staffs. Line 1711 about non-affinity-spacing, though, implies that > staff-like contexts can influence the spacing to both their neighbors, > pushing them apart if need be. (I do not yet understand the behavior > well enough to suggest better text.) > > http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... > > Documentation/notation/spacing.itely:1513: * Inter-system spacing > properties:: > Within-system You said: inter = between, intra = within ==> Between-system, or do you mean the original "Inter-system spacing" should have read "Intra-system spacing"? > > http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... > > Documentation/notation/spacing.itely:1520: @node Inter-system spacing > properties > Within-system > See the comment above. > http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... > > Documentation/notation/spacing.itely:1624: size) will always reset all > its default key-values. > On 2010/10/27 08:44:41, Mark Polesky wrote: >> Can anyone think of a good place for this? > End of section 5.3.1 Overview of modifying properties > > http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.i... > > Documentation/notation/spacing.itely:1662: > @code{after-last-staff-spacing}). > On 2010/10/27 08:44:41, Mark Polesky wrote: >> What's a better way to word it? > "... leave this property unset, because if set it will be used instead > of any StaffGrouper property that would have otherwise applied." > > http://codereview.appspot.com/2642043/
Sign in to reply to this message.
On Fri, 29 Oct 2010 01:17:00 -0700, Ian Hulin <ian@hulin.org.uk> wrote: > On 29/10/10 05:12, Keith wrote: >> Documentation/notation/spacing.itely:1513: * Inter-system spacing >> properties:: >> Within-system > > You said: inter = between, intra = within [...] > or do you mean the original "Inter-system spacing" should have read > "Intra-system spacing"? > I believe the Latin prefixes were wrong in the original. I am confident in my Latin-Germanic translation, but could possibly have missed a subtlety in the distinctions that Mark was explaining. -Keith
Sign in to reply to this message.
Here's the next patch set for the "Vertical spacing inside a system" doc stuff. I tried to address everything you guys mentioned; let me know what you think. One significant thing I did here was to remove the entire @subsection called "Vertical spacing between systems", since I don't think it was really doing anything. I guess I'm ready for the next round of reviews. Thanks. - Mark
Sign in to reply to this message.
Hi Mark, It's the first time I read your NR4 patches, so I may certainly have missed a few things. Your patch looks good, but I was a bit surprised by the overall writing style of NR4. This chapter looks odd to me, and somehow not on par NR1 and 2. It's obviously a whole chapter that hasn't been much affected by GDP -- but of course, I'm not advocating that your patch should rewrite the whole chapter! :) http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:239: @subsubheading Structure of spacing alists for @code{\paper} variables Isn't "alist" an abbreviation of "association list"? http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:305: system-system-spacing #'space = #8 I'd move the comments to the main text. "...demonstrates two ways these alists can be modified: first by updating one key-value individually, then by completely redefining a variable" http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1794: % Here, we allow them to be stretched more widely. I would move this to the main text instead of leaving it as a comment. http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1836: @code{staff-affinity} from top to bottom. For example, the I wouldn't use the property name here in the main text, but rather write a sentence describing its effect (in order to not "talk through the code")... Also, I find that the sentence "the following code is undefined" isn't very clear. Could you rephrase it as "the following code does not have any effect", or something like that?
Sign in to reply to this message.
I've limited my comments to just one :) I think we need to push this now and move on - we still have to change all the names, and a further review of the wording after that might suggest a few more tweaks. I'm sure more clarification will be necessary after users try to understand and use this, but we're now so familiar with it that it's impossible to anticipate the problems they might have. Good work, Mark. I admire your persistence! Trevor http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1584: staves. Does not apply to the bottom staff of a system. The difference between next-staff-spacing and default-next staff-spacing is unclear (at least to me) from these two paragraphs.
Sign in to reply to this message.
LGTM. Thanks, Mark. Carl
Sign in to reply to this message.
http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:256: a system is the middle line of the nearest staff---even if a I think that we normally write "staff -- even". I don't know if texinfo treats different amounts of hyphens differently, or whether the space matters. I'm not particularly concerned about this, though. Just thought I'd mention it.
Sign in to reply to this message.
Mark, Too long, but hard to say what will be useful without getting away for a while. Looks good to me, whether you either take or leave my suggestions. http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:233: Inter-system spacing is controlled by grob properties, with Should be "Within-system" (Inter-system would be system-system-spacing) http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1502: non-staff line is its highest point. Suggest you stop after "its middle line." Ref pt for Lyrics seems to be lowest point, not sure for chords, and users will experiment anyway. http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1528: To change any spacing settings globally, put them in the "throughout a score" instead of "globally" http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1540: The global defaults for the following grob properties are defined if you say "defaults for StaffGrouper and VerticalAxisGroup are" then you can skip the list. http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1572: staff-group, it is usually best to leave this property unset, Let's be honest here " ... it is usually best to leave this property at its default, which is a function that chooses the appropriate spacing from the StaffGrouper object." http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1584: staves. Does not apply to the bottom staff of a system. Suggest : "The value used for next-staff-spacing, if you did not override next-staff-spacing explicitly, for a staff that is not in a group (or for which StaffGrouper does not define a spacing)."
Sign in to reply to this message.
Wow, this is almost done. New patch set uploaded. I've responded to some comments individually, and made a lot of small changes requested, but I want to call everyone's attention to the TODO on l.1504, regarding this clause: "the reference point of a non-staff line is its highest point." Keith points out that the refpoint for Lyrics seems to be the lowest point (or perhaps the baseline?). It would be nice if someone could clear this up (or figure it out). What are the actual reference points for each type of non-staff line (ChordNames, Dynamics, FiguredBass, FretBoards, Lyrics, and NoteNames). I'd like to fix this before pushing. Also, in an attempt to improve the examples, I made some changes. So feel free to look them over if you like. I know this is a lot longer than what's there now; hopefully that's more a reflection of the amount of information than any wordiness on my part. Thanks again to everyone reviewing this. It's a big help. - Mark http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:256: a system is the middle line of the nearest staff---even if a On 2010/11/04 16:11:54, Graham Percival wrote: > I think that we normally write "staff -- even". I don't > know if texinfo treats different amounts of hyphens > differently, or whether the space matters. Looks like yet another UK vs. US thing: http://en.wikipedia.org/wiki/En_dash#En_dash_versus_em_dash http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1528: To change any spacing settings globally, put them in the On 2010/11/05 05:20:39, Keith wrote: > "throughout a score" instead of "globally" But a toplevel \layout block can apply to multiple scores, right? http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1540: The global defaults for the following grob properties are defined On 2010/11/05 05:20:39, Keith wrote: > if you say "defaults for StaffGrouper and > VerticalAxisGroup are" then you can skip the list. I'd rather not since there are 5 properties that are *not* initialized there. http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1572: staff-group, it is usually best to leave this property unset, On 2010/11/05 05:20:39, Keith wrote: > Let's be honest here " ... it is usually best to leave > this property at its default, which is a function that > chooses the appropriate spacing from the StaffGrouper > object." But every time I write something like that, I get complaints about "talking through the code"! http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1584: staves. Does not apply to the bottom staff of a system. On 2010/11/05 05:20:39, Keith wrote: > Suggest : "The value used for next-staff-spacing, if you > did not override next-staff-spacing explicitly, for a > staff that is not in a group (or for which StaffGrouper > does not define a spacing)." This is better, so I'll use it (with a few changes). http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1836: @code{staff-affinity} from top to bottom. For example, the On 2010/11/04 08:37:34, Valentin Villenave wrote: > I wouldn't use the property name here in the main text, > but rather write a sentence describing its effect (in > order to not "talk through the code")... Or should I just remove this altogether, since it's already discussed on line 1592? > Also, I find that the sentence "the following code is > undefined" isn't very clear. Could you rephrase it as > "the following code does not have any effect", or > something like that? Ah yes, the correct word is "unspecified".
Sign in to reply to this message.
LGTM; just a minor typo and a suggestion to remove inadvisable code. http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/14001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1836: @code{staff-affinity} from top to bottom. For example, the I'd remove it. It's bad practice to show examples of incorrect or inadvisable code. http://codereview.appspot.com/2642043/diff/24001/Documentation/notation/spaci... Documentation/notation/spacing.itely:1682: \new StaffGroup << The following example shows how the @code{next-staff-spacing} property can affect the spacing of ungrouped staves:
Sign in to reply to this message.
Pushed with some final changes: http://git.sv.gnu.org/gitweb/?p=lilypond.git;a=commitdiff;h=a3f254b Closing this issue now. Thanks everyone! - Mark
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
