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.itely File Documentation/notation/spacing.itely (left): http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.itely#oldcode1697 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.itely File Documentation/notation/spacing.itely (right): http://codereview.appspot.com/2642043/diff/1/Documentation/notation/spacing.itely#newcode1501 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.itely#newcode1506 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.itely#newcode1510 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.itely#newcode1554 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.itely#newcode1578 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.itely#newcode1836 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.itely#newcode1881 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.itely#newcode1897 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. http://codereview.appspot.com/2642043/ _______________________________________________ lilypond-devel mailing list [email protected] http://lists.gnu.org/mailman/listinfo/lilypond-devel
