which-page

which-page

It's possible to add a markup to a footer/header of a specified page,
using (which-page <page-number>).
Multiple settings are possible.

[thomasmorley65, 2012-06-28 14:35:25]

It's possible to add a markup to a footer/header of a specified page,
using #(which-page <page-number>).
Multiple settings are possible.

Please review as

http://codereview.appspot.com/6352049

-Harm

[dak, 2012-06-28 14:36:43]

A feature should be documented, or it will not be discoverable.

If feasible, a regtest should be added in input/regression.  Whether this is
possible for essentially multi-page features, I don't know, so it is possible
that this particular feature does not really lend itself to a regtest.

http://codereview.appspot.com/6352049/diff/1/ly/titling-init.ly
File ly/titling-init.ly (right):

http://codereview.appspot.com/6352049/diff/1/ly/titling-init.ly#newcode104
ly/titling-init.ly:104: #(define ((which-page nmbr) layout props arg)
"which-page" seems like a less than optimal name for the intended use patterns:

\on-the-fly #(which-page 3) ...

reads less natural than

\on-the-fly #(on-page 3) ...

or

\on-the-fly #(at-page 3) ...

http://codereview.appspot.com/6352049/diff/1/ly/titling-init.ly#newcode105
ly/titling-init.ly:105: (if (= (chain-assoc-get 'page:page-number props 0) nmbr)
The functions below seem to prefer "-1" for something coming without page
number.  Perhaps it would make sense to conform with that kind of usage.

[thomasmorley65, 2012-06-29 09:32:54]

2012/6/28  <dak@gnu.org>:
> A feature should be documented, or it will not be discoverable.

Of course all features (old and new) should be documented.
But, AFAICT, non of the possibilities offered in /ly/titling-init.ly
(i.e. first-page, last-page, not-first-page, part-last-page) is
documented anywhere in LM or NR.
The only place they are used in the docs is:
Documentation/snippets/book-parts.ly
Well, they should be documented, but I propose to open a new issue to do so.

> If feasible, a regtest should be added in input/regression.  Whether
> this is possible for essentially multi-page features, I don't know, so
> it is possible that this particular feature does not really lend itself
> to a regtest.

I found only one multi-page-file in the regression-tests containing a
feature of titling-init.ly (i.e. `not-first-page'):
input/regression/page-breaks.ly
But I think this file tests something else.
So I'd suggest to open a new issue for that, too.

>
> http://codereview.appspot.com/6352049/diff/1/ly/titling-init.ly
> File ly/titling-init.ly (right):
>
> http://codereview.appspot.com/6352049/diff/1/ly/titling-init.ly#newcode104
> ly/titling-init.ly:104: #(define ((which-page nmbr) layout props arg)
> "which-page" seems like a less than optimal name for the intended use
> patterns:
>
> \on-the-fly #(which-page 3) ...
>
> reads less natural than
>
> \on-the-fly #(on-page 3) ...
>
> or
>
> \on-the-fly #(at-page 3) ...

Done

> http://codereview.appspot.com/6352049/diff/1/ly/titling-init.ly#newcode105
> ly/titling-init.ly:105: (if (= (chain-assoc-get 'page:page-number props
> 0) nmbr)
> The functions below seem to prefer "-1" for something coming without
> page number.  Perhaps it would make sense to conform with that kind of
> usage.

Done

> http://codereview.appspot.com/6352049/

[Graham Percival, 2012-06-29 10:01:49]

On Fri, Jun 29, 2012 at 11:32:53AM +0200, Thomas Morley wrote:
> 2012/6/28  <dak@gnu.org>:
> > A feature should be documented, or it will not be discoverable.
> 
> Well, they should be documented, but I propose to open a new issue to do so.

That sounds sensible.  In the past we've said that new features do
not need to be documented in the same patch that adds it.

> > If feasible, a regtest should be added in input/regression.  Whether
> > this is possible for essentially multi-page features, I don't know, so
> > it is possible that this particular feature does not really lend itself
> > to a regtest.
> 
> I found only one multi-page-file in the regression-tests containing a
> feature of titling-init.ly (i.e. `not-first-page'):
> input/regression/page-breaks.ly
> But I think this file tests something else.
> So I'd suggest to open a new issue for that, too.

Here I disagree slightly; it would be really good if new features
were covered by the regtests as soon as they were added, so that
we have a chance of noticing when they break.  It doesn't need to
be in a new regtest; adding it to an exising regtest is fine.

- Graham

[t.daniels_treda.co.uk, 2012-06-29 10:13:42]

Thomas Morley wrote Friday, June 29, 2012 10:32 AM

> Of course all features (old and new) should be documented.
> But, AFAICT, non of the possibilities offered in /ly/titling-init.ly
> (i.e. first-page, last-page, not-first-page, part-last-page) is
> documented anywhere in LM or NR.
> Well, they should be documented, but I propose to open a new issue to do so.

Eluze already opened an issue for this - 2579 - and I'm
part-way through a fix to the docs.  I'll upload it for review
shortly, and I'd appreciate your comments and help.

> I found only one multi-page-file in the regression-tests containing a
> feature of titling-init.ly (i.e. `not-first-page'):
> input/regression/page-breaks.ly
> But I think this file tests something else.
> So I'd suggest to open a new issue for that, too.

Fine - please do.

Trevor

[thomasmorley65, 2012-06-30 13:28:26]

On 2012/06/29 10:13:42, t.daniels_treda.co.uk wrote:
> Thomas Morley wrote Friday, June 29, 2012 10:32 AM
> 
> > Of course all features (old and new) should be documented.
> > But, AFAICT, non of the possibilities offered in /ly/titling-init.ly
> > (i.e. first-page, last-page, not-first-page, part-last-page) is
> > documented anywhere in LM or NR.
> > Well, they should be documented, but I propose to open a new issue to do so.
> 
> Eluze already opened an issue for this - 2579 - and I'm
> part-way through a fix to the docs.  I'll upload it for review
> shortly, and I'd appreciate your comments and help.

Will do.

> > I found only one multi-page-file in the regression-tests containing a
> > feature of titling-init.ly (i.e. `not-first-page'):
> > input/regression/page-breaks.ly
> > But I think this file tests something else.
> > So I'd suggest to open a new issue for that, too.
> 
> Fine - please do.

Regtest added now.

-Harm

[Graham Percival, 2012-07-02 11:21:04]

LGTM