Re: Doc: NR 4.4.1: Rewrite. (issue2642043)

[markpolesky]

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/