Re: Nested book parts

[nicolas . sceaux]

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.

http://codereview.appspot.com/4848