Corrections and additions to NR 4.1.2, "Page formatting".

Corrections and additions to NR 4.1.2, "Page formatting".

- removed duplications / typos
- five -> four components in vertical spacing alists
- added explanation of bottom-system-spacing and top-system-spacing
- moved bottom-margin and top-margin from Horizontal to Vertical spacing section
- removed foot-separation and head-separation
  (RFC: Is it good or not to mention those no longer existing variables,
        along with a hint how to achieve their meaning with the current methods?
        -> see @warning note in Vertical spacing section)

[Carl, 2010-06-22 14:27:23]

Thanks for doing this.  I have a couple of comments.

Carl

http://codereview.appspot.com/1710046/diff/1/2
File Documentation/notation/spacing.itely (right):

http://codereview.appspot.com/1710046/diff/1/2#newcode280
Documentation/notation/spacing.itely:280: last staff and the top of the page
footer.
What about the top-level markup?  This information appears to be gone.

http://codereview.appspot.com/1710046/diff/1/2#newcode335
Documentation/notation/spacing.itely:335: @item @var{top-margin}
Why separate the top and bottom margins from the left and right margins for page
layout purposes?

[perpeduumimmobile, 2010-07-12 16:36:20]

Hi, Carl,

thanks for your comments. Joe, could you have a look at the paragraph about
top-system-spacing and top-title-spacing, w.r.t. top level markups?


Thanks!

http://codereview.appspot.com/1710046/diff/1/2
File Documentation/notation/spacing.itely (right):

http://codereview.appspot.com/1710046/diff/1/2#newcode280
Documentation/notation/spacing.itely:280: last staff and the top of the page
footer.
On 2010/06/22 14:27:23, Carl wrote:
> What about the top-level markup?  This information appears to be gone.

Reincluded. Not sure if the same should apply to top-system-spacing, though - it
wasn't there, and this might be handled by top-title-spacing, but it could be
just forgotten, too.

http://codereview.appspot.com/1710046/diff/1/2#newcode335
Documentation/notation/spacing.itely:335: @item @var{top-margin}
On 2010/06/22 14:27:23, Carl wrote:
> Why separate the top and bottom margins from the left and right margins for
page
> layout purposes?

Because left and right margins are mentioned in the Horizontal dimensions
section, where they don't belong. Of course, it's repeated information then, but
I guess we should have most (all) information about /vertical/ spacing here.

[joeneeman, 2010-07-12 17:06:36]

http://codereview.appspot.com/1710046/diff/4001/5001
File Documentation/notation/spacing.itely (right):

http://codereview.appspot.com/1710046/diff/4001/5001#newcode281
Documentation/notation/spacing.itely:281: whitespace between the last staff (or
markup) and the top of the page footer.
Yup, that's right. top-system-spacing too. If you think it's worth adding, the
same logic applies to top-title-spacing. That is, space and minimum-distance
apply to the baseline of the markup while padding applies to the top of its
bounding box.

[perpeduumimmobile, 2010-07-12 18:23:53]

http://codereview.appspot.com/1710046/diff/4001/5001
File Documentation/notation/spacing.itely (right):

http://codereview.appspot.com/1710046/diff/4001/5001#newcode281
Documentation/notation/spacing.itely:281: whitespace between the last staff (or
markup) and the top of the page footer.
On 2010/07/12 17:06:36, joeneeman wrote:
> Yup, that's right. top-system-spacing too. If you think it's worth adding, the
> same logic applies to top-title-spacing.

Okay, done. Thanks!

[Neil Puttock, 2010-08-02 22:47:00]

On 2010/07/12 17:06:36, joeneeman wrote:

> Yup, that's right. top-system-spacing too. If you think it's worth adding, the
> same logic applies to top-title-spacing. That is, space and minimum-distance
> apply to the baseline of the markup while padding applies to the top of its
> bounding box.

Unless I'm missing something here, top-system-spacing ignores the first
top-level markup (unlike top-title-spacing), since there's no distinction in
page-layout-problem.cc between a title and a markup (and the regression test for
top-title-spacing explicitly mentions that it works with titles and markup):

  81       top_system_spacing = paper->c_variable ("top-system-spacing");
  82       if (scm_is_pair (systems) && unsmob_prob (scm_car (systems)))
  83         top_system_spacing = paper->c_variable ("top-title-spacing");

Do we want to change the behaviour so the top-level markup obeys
top-system-spacing instead?

top_system_spacing = paper->c_variable ("top-system-spacing");
if (scm_is_pair (systems))
  {
    Prob *top = unsmob_prob (scm_car (systems));
    if (!to_boolean (top->get_property ("first-markup-line")))
      top_system_spacing = paper->c_variable ("top-title-spacing");
  }

[Neil Puttock, 2010-08-28 20:32:36]

Hi Alexander,

This looks great; if somebody can clarify the behaviour of top-system-spacing
with top-level markup, we should be able to commit your patch.

Cheers,
Neil

[perpeduumimmobile, 2010-09-06 10:34:49]

On 2010/08/28 20:32:36, Neil Puttock wrote:
> Hi Alexander,
> 
> This looks great; if somebody can clarify the behaviour of top-system-spacing
> with top-level markup, we should be able to commit your patch.

Joe, may I ask you for help on this?  I sure can try to figure out what's the
current behaviour; but the actual goal we want to achieve is beyond my
competence.

[joeneeman, 2010-09-06 16:30:25]

On 2010/09/06 10:34:49, perpeduumimmobile wrote:
> On 2010/08/28 20:32:36, Neil Puttock wrote:
> > Hi Alexander,
> > 
> > This looks great; if somebody can clarify the behaviour of
top-system-spacing
> > with top-level markup, we should be able to commit your patch.
> 
> Joe, may I ask you for help on this?  I sure can try to figure out what's the
> current behaviour; but the actual goal we want to achieve is beyond my
> competence.

So, the current behaviour is that a top-level markup gets spaced according to
top-title-spacing. If you (or someone else) wants to change it to
top-system-spacing, I'm ok with that, too; I don't really know which one would
be more useful. It would even be possible to introduce top-markup-spacing if
people really want that control. But maybe it's best, for this patch, to
document the current reality and worry about the other stuff if people ask for
it.

[Graham Percival (old account), 2010-09-27 20:06:27]

On 2010/09/06 16:30:25, joeneeman wrote:
> So, the current behaviour is that

...

> But maybe it's best, for this patch, to
> document the current reality and worry about the other stuff if people ask for
> it.

Absolutely!


Alexander, please finish writing up the "current reality" so that we can get
this pushed.  I'm not going to hold up 2.14 for it, but it would be really nice
if we had some better documentation for the new spacing code.

Cheers,
- Graham

[perpeduumimmobile, 2010-09-28 09:20:48]

On 2010-09-27 22:06, percival.music.ca@gmail.com wrote:
> On 2010/09/06 16:30:25, joeneeman wrote:
>> So, the current behaviour is that
> ...
>> But maybe it's best, for this patch, to
>> document the current reality and worry about the other
>> stuff if people ask for it.
> 
> Absolutely!
> 
> Alexander, please finish writing up the "current reality"
> so that we can get this pushed.

Here we are.  Today, Mark also posted another disagreement between docs and
reality on vertical spacing (see
<http://lists.gnu.org/archive/html/bug-lilypond/2010-09/msg00408.html> and the
inline comment.  Should I also change this in the docs?

http://codereview.appspot.com/1710046/diff/19001/Documentation/notation/spaci...
File Documentation/notation/spacing.itely (right):

http://codereview.appspot.com/1710046/diff/19001/Documentation/notation/spaci...
Documentation/notation/spacing.itely:211: @item @var{space} -- the amount of
stretchable space between the baseline
lines 211 and 219: change "baseline" to "top"?

(See <http://lists.gnu.org/archive/html/bug-lilypond/2010-09/msg00408.html> by
Mark Polesky; I checked the current behaviour and can confirm Mark's
observations.)

[Mark Polesky, 2010-09-29 17:51:34]

http://codereview.appspot.com/1710046/diff/19001/Documentation/notation/spaci...
File Documentation/notation/spacing.itely (right):

http://codereview.appspot.com/1710046/diff/19001/Documentation/notation/spaci...
Documentation/notation/spacing.itely:211: @item @var{space} -- the amount of
stretchable space between the baseline
On 2010/09/28 09:20:48, perpeduumimmobile wrote:
> lines 211 and 219: change "baseline" to "top"?

I am not in favor of making that change.  I
strongly consider this a code bug, not a doc
bug.  The height of titles varies tremendously,
and I think it makes little sense for something
called "after-title-spacing" to include the
height of the title itself in its measurement.

In practical terms, I think this gives the user
less flexibility, since simply adding a subtitle
could (IIUC) greatly alter the effect the
previous settings.

- Mark

[perpeduumimmobile, 2010-10-02 09:18:16]

On 2010/09/29 17:51:34, Mark Polesky wrote:
> On 2010/09/28 09:20:48, perpeduumimmobile wrote:
> > lines 211 and 219: change "baseline" to "top"?
> 
> I am not in favor of making that change.  I
> strongly consider this a code bug, not a doc
> bug.  The height of titles varies tremendously,
> and I think it makes little sense for something
> called "after-title-spacing" to include the
> height of the title itself in its measurement.

+1.

[perpeduumimmobile, 2010-10-02 09:19:00]

Sorry, hit the button early.

Be sure to check <http://codereview.appspot.com/2316042/> by Mark before
applying this patch.

[Graham Percival (old account), 2010-11-13 17:33:47]

This patch cannot be applied to current master.

Please withdraw the patch, or rewrite it to match the post-renaming spacing
docs.  If you opt to rewrite it, then Mark Polesky can help you.

[Mark Polesky, 2010-11-14 06:59:41]

On 2010/11/13 17:33:47, Graham Percival wrote:
> This patch cannot be applied to current master.
> 
> Please withdraw the patch, or rewrite it to match the
> post-renaming spacing docs.  If you opt to rewrite it,
> then Mark Polesky can help you.

Graham,

I don't want to be a spoilsport, but I have a patch of my
own that I'm pretty sure takes care of everything involved
here (and more): http://codereview.appspot.com/2758042/

I seem to recall that Alexander has been aware of this for a
while and is fine with it.  There are already 17 comments on
my patch, and I'll probably get around to a revised patch
set soon-ish.  Are we under a time crunch or anything?

The two NR sections I'd like to finish before the next stable
release are:

  NR 4.1   Paper and pages
  NR 4.4.1 Flexible vertical spacing within systems

Of these two, NR 4.4.1 is almost done (if you want to
comment on the last bit, see
http://codereview.appspot.com/3089042/).

But NR 4.1 still needs some work, so let me know what the
time pressure is here.  Except for renaming 'space to
'basic-distance (which I think Joe will take care of), all
of the other syntax changes are done.  But the doc patches
mentioned above are still pending revision and/or approval.

- Mark

[Graham Percival (old account), 2010-11-14 07:11:55]

On 2010/11/14 06:59:41, Mark Polesky wrote:
> I don't want to be a spoilsport, but I have a patch of my
> own that I'm pretty sure takes care of everything involved
> here (and more): http://codereview.appspot.com/2758042/

I have absolutely no problem with that.  I just wanted some clarity about the
remaining "patch" issues.  If we all agree that your patches supercedes this
one, then let's close the issue.  Having month-old patches floating around makes
me jittery.

> The two NR sections I'd like to finish before the next stable
> release are:

I'd like to have a Documentation-Critical issue for this.
 
> But NR 4.1 still needs some work, so let me know what the
> time pressure is here.

Nothing specific.

- Graham

[perpeduumimmobile, 2010-11-14 09:17:30]

On 2010/11/14 06:59:41, Mark Polesky wrote:
> On 2010/11/13 17:33:47, Graham Percival wrote:
> > This patch cannot be applied to current master. [...]
> 
> I don't want to be a spoilsport, but I have a patch of my
> own that I'm pretty sure takes care of everything involved
> here (and more): http://codereview.appspot.com/2758042/
> 
> I seem to recall that Alexander has been aware of this for a
> while and is fine with it.

Of course.  Consider my patch as closed and discarded, and go on with yours!


Cheers,
Alexander