Re: Enhancement: Page breaking to avoid widow lines

[Joe Neeman]

On Tue, 2010-01-12 at 11:58 -0500, address@hidden wrote:
> Here is a proposed patch to add functionality to the Page Spacer. 
> 
> When typesetting large quantities of markup lines, Lilypond currently does not
> try to avoid widowed/orphaned lines.  Some publishers will consider this an
> unacceptable typographical error. 
> 
> I have added a few lines to the Page Spacer so it avoid having a line of 
> markup
> stand by itself.  There are two cases.  Case (1) is when the last line of a
> markup paragraph is the first line on a page, and then there goes the start of
> another paragraph (markuplines or music).  Case (2) is when there is nothing
> after that lonely last line -- in other words, the last page of the book
> consists of one line of markup. 

It's nice to see someone else touching the page-breaking code :)
The ideas in your code look fine, but it doesn't conform to the lilypond
code style. I've made some comments below, but it would be easier to
review if you could upload your patch to codereview.appspot.com.

> There are probably two things one would want to add to this functionality. 
> First: The penalty associated with the widow should be made configurable. 
> (Setting a Scheme variable in the source score, and then reading it in
> Page_spacer::calc_subproblem() where it is now hardcoded to 
> BAD_SPACING_PENALTY/2?)

You could make it a paper block variable and access it in Page_spacer in
the same way that the ragged variables are currently accessed.

> Second: This definition of widow is oversimplified.  The Chicago Manual talks
> about orphans/widows which are *too short*.  Right now, my code does not take
> line length into consideration at all.  It should look at lines_[page_start] 
> and
> compare it to some configurable threshold.  I am not sure, however, how to 
> read
> the horizontal width, as Line_details only contains the Y extent. 
> Also, if we consider line lengths, then we should address orphans as well --
> right now we are only avoiding widows.

You could check the X extent in Paper_book::get_system_specs or
Line_details(Prob*). In fact, if you checked the X extent in
Paper_book::get_system_specs then you wouldn't even need the
markup-list-id property: you could add an avoid-orphan property that
only gets set for the last line of a multi-line markup list if it is
short. That might also simplify Page_spacer::calc_subproblem.

> Here is the proposed patch (against latest git head):
> 
> diff --git a/lily/constrained-breaking.cc b/lily/constrained-breaking.cc
> index 2fead24..f5158b1 100644
> --- a/lily/constrained-breaking.cc
> +++ b/lily/constrained-breaking.cc
> @@ -525,4 +525,6 @@ Line_details::Line_details (Prob *pb, Output_def *paper)
>    title_ = to_boolean (pb->get_property ("is-title"));
>    compressed_lines_count_ = 1;
>    compressed_nontitle_lines_count_ = title_ ? 0 : 1;
> +  SCM scmId = pb->get_property("markup-list-id");

scm_id (or markup_list_id_scm)
space before opening paren

> +  markup_list_id = SCM_INUMP(scmId)? scm_num2int(scmId, 0, NULL) : 0;

robust_scm2int

>  }
> diff --git a/lily/include/constrained-breaking.hh
> b/lily/include/constrained-breaking.hh
> index bb8a1d0..9628492 100644
> --- a/lily/include/constrained-breaking.hh
> +++ b/lily/include/constrained-breaking.hh
> @@ -52,6 +52,7 @@ struct Line_details {
>       class. */
>    int compressed_lines_count_;
>    int compressed_nontitle_lines_count_;
> +  int markup_list_id;

class members get trailing underscores

>    Line_details ()
>    {
> @@ -71,6 +72,7 @@ struct Line_details {
>      title_ = false;
>      compressed_lines_count_ = 1;
>      compressed_nontitle_lines_count_ = 1;
> +    markup_list_id = 0;
>    }
>  
>    Line_details (Prob *pb, Output_def *paper);
> diff --git a/lily/page-spacing.cc b/lily/page-spacing.cc
> index 8919257..be5f0e7 100644
> --- a/lily/page-spacing.cc
> +++ b/lily/page-spacing.cc
> @@ -245,6 +245,24 @@ Page_spacer::calc_subproblem (vsize page, vsize line)
>           penalty += lines_[page_start-1].page_penalty_
>             + (page % 2 == 0) ? lines_[page_start-1].turn_penalty_ : 0;
>  
> +       /* Deal with widow lines */
> +       /* The penalty should be made configurable from the source score */
> +       Real widow_penalty = BAD_SPACING_PENALTY/2;
> +       /* Avoid having a widow before start of next paragraph */
> +       if ((page_start > 0) &&
> +           (page_start+1 < lines_.size()) &&
> +           (lines_[page_start-1].markup_list_id) &&
> +           (lines_[page_start].markup_list_id) &&
> +           // do not test for lines_[page_start+1].markup_list_id --
> +           //    next paragraph may not be markup
> +           (lines_[page_start].markup_list_id != 
> lines_[page_start+1].markup_list_id) )

no space before closing paren

> +               penalty += widow_penalty;
> +       /* Avoid having a widow as the very last line of the book */
> +       if ((page_start > 0) &&
> +               (page_start == lines_.size()-1) &&
> +               (lines_[page_start].markup_list_id) )
> +               penalty += widow_penalty;
> +
>         demerits += penalty;
>         if (demerits < cur.demerits_ || page_start == line)
>           {
> diff --git a/lily/paper-book.cc b/lily/paper-book.cc
> index 79896e6..13bd955 100644
> --- a/lily/paper-book.cc
> +++ b/lily/paper-book.cc
> @@ -430,6 +430,11 @@ Paper_book::get_score_title (SCM header)
>  }
>  
> 
> +static int current_int = 1;
> +static int get_next_int() {

static int
get_next_int ()
{

> +     return current_int++;
> +}
> +
>  SCM
>  Paper_book::get_system_specs ()
>  {
> @@ -516,6 +521,7 @@ Paper_book::get_system_specs ()
>                                 paper_->self_scm (),
>                                 page_properties,
>                                 scm_car (s));
> +       int markup_list_id = get_next_int();
>         for (SCM list = texts ; scm_is_pair (list) ; list = scm_cdr (list))
>           {
>             SCM t = scm_car (list);
> @@ -525,6 +531,8 @@ Paper_book::get_system_specs ()
>                               ly_symbol2scm ("allow"));
>             ps->set_property ("page-turn-permission",
>                               ly_symbol2scm ("allow"));
> +           ps->set_property ("markup-list-id",
> +                             scm_int2num (markup_list_id));
>  
>             paper_system_set_stencil (ps, *unsmob_stencil (t));
>             ps->set_property ("is-title", SCM_BOOL_T);
> 
> 
> 
> 
> _______________________________________________
> lilypond-devel mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/lilypond-devel