Nested book parts

Book parts aim at splitting a book into several parts, in order to be able to use eg. different page breaking functions, or to make the page breaking problem less difficult and more likely to finish.

- Book and Paper_book instances respectively are nestable: children
  book or paper_book are added to the bookparts_ slot;

- the paper_ slot of a child Book (or Book_paper) is created empty,
  and has its parent set to the paper object of the parent Book (or
  Paper_book), so that default paper properties are got from the
  higher level paper object, and child objects only store part-wide
  overrides. This way, we ensure that fonts are loaded in the higher
  level paper object, so that the output framework can get all the
  loaded fonts from the top level book;

- a Paper_book::top_paper() method is added to access the higher level
  paper object, to access properties that are book-wide, for instance
  the table used to store labels and page numbers;

- in the parser, \bookpart blocks are introduced, which can be used at
  toplevel, or inside a \book block. It can contain the same things as
  \book blocks (except \bookpart blocks, though that would be
  possible). The associated handlers are added.

[joeneeman, 2008-10-15 01:06:06]

I'm really sorry I took so long to do this. I honestly thought it had already
been committed (probably because it has been merged into my working copy for
such a long time).

http://codereview.appspot.com/4848/diff/1/4
File lily/book.cc (right):

http://codereview.appspot.com/4848/diff/1/4#newcode139
Line 139: Book::add_bookpart ()
This method confused me for a while (because it has quite a different purpose
from add_bookpart(SCM)). I suggest renaming this method and adding a comment
along the lines of "all scores before an explicit \bookpart are implicitly
lumped into their own separate bookpart".

http://codereview.appspot.com/4848/diff/1/4#newcode190
Line 190: Book::process (Output_def *default_paper,
This method is becoming very long. Can it be split up into multiple functions
like:

if (there are bookparts)
  process_bookparts();
else
  for (s in scores)
    process_score(s);

http://codereview.appspot.com/4848/diff/1/5
File lily/include/book.hh (right):

http://codereview.appspot.com/4848/diff/1/5#newcode35
Line 35: void add_bookpart ();
Can this be private? Yeah, I know everything else here is public, but that's no
reason to perpetuate the problem.

http://codereview.appspot.com/4848/diff/1/5#newcode38
Line 38: bool error_found ();
also private?

http://codereview.appspot.com/4848/diff/1/8
File lily/minimal-page-breaking.cc (right):

http://codereview.appspot.com/4848/diff/1/8#newcode43
Line 43: vsize first_page_num = robust_scm2int (book_->paper_->c_variable
("part-first-page-number"), 1);
Do we really need new variables for properties in book parts (as opposed to
using some cascading to allow bookparts to override properties in the parent)? I
could see this leading to an explosion of properties.

http://codereview.appspot.com/4848/diff/1/10
File lily/page-breaking.cc (right):

http://codereview.appspot.com/4848/diff/1/10#newcode266
Line 266: bool last_from_part = (i == lines_per_page.size () - 1);
I'm not sure this is the logic we want here. I think we want to treat the last
page in a bookpart the same as the last page in a book. This would give the
per-score ragged-last-bottom feature that some people have been asking for.

http://codereview.appspot.com/4848/diff/1/13
File lily/paper-book.cc (right):

http://codereview.appspot.com/4848/diff/1/13#newcode112
Line 112: Paper_book::output_aux (SCM output_channel,
This gets simpler if you no longer care about is_first and is_last...

http://codereview.appspot.com/4848/diff/1/13#newcode549
Line 549: /* backwards compatibility for the old page breaker */
We can probably scrap this soon.

[hanwenn, 2008-10-15 01:29:22]

http://codereview.appspot.com/4848/diff/1/5
File lily/include/book.hh (right):

http://codereview.appspot.com/4848/diff/1/5#newcode35
Line 35: void add_bookpart ();
On 2008/10/15 01:06:06, joeneeman wrote:
> Can this be private? Yeah, I know everything else here is public, but that's
no
> reason to perpetuate the problem.

actually, my preference is for protected, unless the method does something
really internal (like breaking invariants.)

[nicolas.sceaux, 2008-10-18 15:10:28]

http://codereview.appspot.com/4848/diff/1/4
File lily/book.cc (right):

http://codereview.appspot.com/4848/diff/1/4#newcode139
Line 139: Book::add_bookpart ()
On 2008/10/15 01:06:06, joeneeman wrote:
> This method confused me for a while (because it has quite a different purpose
> from add_bookpart(SCM)). I suggest renaming this method and adding a comment
> along the lines of "all scores before an explicit \bookpart are implicitly
> lumped into their own separate bookpart".

Done.
/* Before an explicit \bookpart is encountered, scores are added to the book.
 * But once a bookpart is added, the previous scores shall be collected into
 * a new bookpart.
 */
void
Book::add_scores_to_bookpart ()

http://codereview.appspot.com/4848/diff/1/4#newcode190
Line 190: Book::process (Output_def *default_paper,
On 2008/10/15 01:06:06, joeneeman wrote:
> This method is becoming very long.

Done.

http://codereview.appspot.com/4848/diff/1/5
File lily/include/book.hh (right):

http://codereview.appspot.com/4848/diff/1/5#newcode35
Line 35: void add_bookpart ();
On 2008/10/15 01:29:22, hanwenn wrote:
> On 2008/10/15 01:06:06, joeneeman wrote:
> > Can this be private? Yeah, I know everything else here is public, but that's
> no
> > reason to perpetuate the problem.
> 
> actually, my preference is for protected, unless the method does something
> really internal (like breaking invariants.)

Done.

http://codereview.appspot.com/4848/diff/1/5#newcode38
Line 38: bool error_found ();
On 2008/10/15 01:06:06, joeneeman wrote:
> also private?

Done.

http://codereview.appspot.com/4848/diff/1/8
File lily/minimal-page-breaking.cc (right):

http://codereview.appspot.com/4848/diff/1/8#newcode43
Line 43: vsize first_page_num = robust_scm2int (book_->paper_->c_variable
("part-first-page-number"), 1);
On 2008/10/15 01:06:06, joeneeman wrote:
> Do we really need new variables for properties in book parts (as opposed to
> using some cascading to allow bookparts to override properties in the parent)?
I
> could see this leading to an explosion of properties.

indeed the first-page-number property can be used in children parts, instead of
a new property.
Done.

http://codereview.appspot.com/4848/diff/1/10
File lily/page-breaking.cc (right):

http://codereview.appspot.com/4848/diff/1/10#newcode266
Line 266: bool last_from_part = (i == lines_per_page.size () - 1);
On 2008/10/15 01:06:06, joeneeman wrote:
> I'm not sure this is the logic we want here. I think we want to treat the last
> page in a bookpart the same as the last page in a book. This would give the
> per-score ragged-last-bottom feature that some people have been asking for.

`last_from_part' is used to achieve what you propose: treat the part last page
as the one that gets special spacing with ragged-last-bottom.
`last_from_book' is used to know where to put the tagline. We do not want  a
tagline at every last page of book parts.

http://codereview.appspot.com/4848/diff/1/13
File lily/paper-book.cc (right):

http://codereview.appspot.com/4848/diff/1/13#newcode112
Line 112: Paper_book::output_aux (SCM output_channel,
On 2008/10/15 01:06:06, joeneeman wrote:
> This gets simpler if you no longer care about is_first and is_last...

`is_first' is useless indeed (remainer of previous implementation)
but `is_last' is useful, not to put tagline on every part last page.

[pas, 2008-10-29 06:11:17]

http://codereview.appspot.com/4848/diff/1/4
File lily/book.cc (right):

http://codereview.appspot.com/4848/diff/1/4#newcode139
Line 139: Book::add_bookpart ()
On 2008/10/18 15:10:29, nicolas.sceaux wrote:
> On 2008/10/15 01:06:06, joeneeman wrote:
> > This method confused me for a while (because it has quite a different
purpose
> > from add_bookpart(SCM)). I suggest renaming this method and adding a comment
> > along the lines of "all scores before an explicit \bookpart are implicitly
> > lumped into their own separate bookpart".
> 
> Done.
> /* Before an explicit \bookpart is encountered, scores are added to the book.
>  * But once a bookpart is added, the previous scores shall be collected into
>  * a new bookpart.
>  */
> void
> Book::add_scores_to_bookpart ()
> 

Done.

http://codereview.appspot.com/4848/diff/1/4#newcode190
Line 190: Book::process (Output_def *default_paper,
On 2008/10/18 15:10:29, nicolas.sceaux wrote:
> On 2008/10/15 01:06:06, joeneeman wrote:
> > This method is becoming very long.
> 
> Done.

Done.

[joeneeman, 2008-11-03 00:27:51]

Look good, just one nitpick.

http://codereview.appspot.com/4848/diff/806/615
File lily/page-breaking.cc (right):

http://codereview.appspot.com/4848/diff/806/615#newcode227
Line 227: ly_symbol2scm ("is-part-last"), scm_from_bool (last),
A really minor naming nitpick, but you have is-last and is-part-last in the page
properties, part-is-last in the paper block and is-part-last-page as a titling
predicate. Of these, is-part-last and is-part-last-page mean the same thing
(AFAICT), but is-last and part-is-last are completely different from each other
and from the other 2. Could you make the names more clear (I have no objection
to verbose names for things with broad visibility, maybe something like
is-last-page-from-bookpart and is-last-bookpart)?

[nicolas.sceaux, 2008-11-16 12:16:58]

http://codereview.appspot.com/4848/diff/806/615
File lily/page-breaking.cc (right):

http://codereview.appspot.com/4848/diff/806/615#newcode227
Line 227: ly_symbol2scm ("is-part-last"), scm_from_bool (last),
On 2008/11/03 00:27:51, joeneeman wrote:
> A really minor naming nitpick, but you have is-last and is-part-last in the
page
> properties, part-is-last in the paper block and is-part-last-page as a titling
> predicate. Of these, is-part-last and is-part-last-page mean the same thing
> (AFAICT), but is-last and part-is-last are completely different from each
other
> and from the other 2. Could you make the names more clear (I have no objection
> to verbose names for things with broad visibility, maybe something like
> is-last-page-from-bookpart and is-last-bookpart)?

Done: use is-bookpart-last-page and is-last-bookpart everywhere