Re: Do not allow page breaks on the first column of a score (issue 581720054 by address@hidden)

[hanwenn]

Reviewers: thomasmorley651,

Message:
On 2020/03/01 14:37:19, thomasmorley651 wrote:
> I've applied your patch and did some testings.
> 
> It indeed solves the code given with boom.ly at
> https://sourceforge.net/p/testlilyissues/issues/5801/
> 
> Though, I posted two minimals there.
> While the first one now compiles fine, the second still crashes with:
> 
> GNU LilyPond 2.21.0
> Processing `atest-99.ly'
> Parsing...
> Interpreting music...[8][16][24][24]
> Preprocessing graphical objects...
> Calculating page and line breaks (1 possible page breaks)...[1]
> Drawing systems...
> Layout output to `/tmp/lilypond-n0vRDz'...
> Converting to `atest-99.pdf'...
> Deleting `/tmp/lilypond-n0vRDz'...
> Interpreting music...
> Preprocessing graphical objects...lilypond:
> /home/hermann/lilypond-git-guile-2.2/lily/constrained-breaking.cc:481:
void
> Constrained_breaking::initialize(Paper_score*, const std::vector<long
unsigned
> int>&): Assertion `pagebreak_col_indices[i] > pagebreak_col_indices[i
- 1]'
> failed.
> Aborted (core dumped)
> 
> Probably
> https://sourceforge.net/p/testlilyissues/issues/5767/
> is related as well.

thanks, that is useful to know, but I don't think it should be a blocker
for this patch to go in.

Description:
Do not allow page breaks on the first column of a score

This duplicates a column index in the vector of allowed break points,
causing a crash.

Do some more sanity checking on the input of Constrained_breaking.

Fixes #5801

Please review this at https://codereview.appspot.com/581720054/

Affected files (+56, -11 lines):
  A input/regression/page-turn-page-breaking-first-column.ly
  M lily/constrained-breaking.cc
  M lily/include/constrained-breaking.hh
  M lily/page-breaking.cc


Index: input/regression/page-turn-page-breaking-first-column.ly
diff --git a/input/regression/page-turn-page-breaking-first-column.ly 
b/input/regression/page-turn-page-breaking-first-column.ly
new file mode 100644
index 
0000000000000000000000000000000000000000..a46d06c0446792e407f90b1ab3d2e3bf5b65f780
--- /dev/null
+++ b/input/regression/page-turn-page-breaking-first-column.ly
@@ -0,0 +1,34 @@
+\version "2.21.0"
+\header {
+  texidoc = "Allowing the first command column to be breakable caused a crash 
in
+Page_turn_page_breaking."
+}
+
+\book {
+  \score {
+    \new Staff
+      \new Voice <<
+       {
+         \time 4/4
+         \bar "||"                     % trick Page_turn_engraver into 
allowing page break
+         s1*5 |
+       }
+       {
+         R1
+         d4
+       }
+      >>
+
+    \layout {
+      \set Score.skipBars = ##t
+      \context {
+       \Staff
+       \consists "Page_turn_engraver"
+      }
+    }
+  }
+
+  \paper {
+    page-breaking = #ly:page-turn-breaking
+  }
+}
Index: lily/constrained-breaking.cc
diff --git a/lily/constrained-breaking.cc b/lily/constrained-breaking.cc
index 
eba3b76390d6a83ff1ab607c31234d1a1c28bf93..8bddbbb539283164b4c755c133da11eada24730f
 100644
--- a/lily/constrained-breaking.cc
+++ b/lily/constrained-breaking.cc
@@ -352,14 +352,14 @@ Constrained_breaking::prepare_solution (vsize start, 
vsize end, vsize sys_count)
 
 Constrained_breaking::Constrained_breaking (Paper_score *ps)
 {
-  start_.push_back (0);
-  initialize (ps);
+  vector<vsize> start;
+  start.push_back (0);
+  initialize (ps, start);
 }
 
 Constrained_breaking::Constrained_breaking (Paper_score *ps, vector<vsize> 
const &start)
-  : start_ (start)
 {
-  initialize (ps);
+  initialize (ps, start);
 }
 
 static SCM
@@ -373,9 +373,11 @@ min_permission (SCM perm1, SCM perm2)
   return SCM_EOL;
 }
 
-/* find the forces for all possible lines and cache ragged_ and ragged_right_ 
*/
+/* find the forces for all possible lines and cache ragged_ and ragged_right_
+ */
 void
-Constrained_breaking::initialize (Paper_score *ps)
+Constrained_breaking::initialize (Paper_score *ps,
+                                  vector<vsize> const &pagebreak_col_indices)
 {
   valid_systems_ = systems_ = 0;
   pscore_ = ps;
@@ -471,13 +473,20 @@ Constrained_breaking::initialize (Paper_score *ps)
     }
 
   /* work out all the starting indices */
-  for (vsize i = 0; i < start_.size (); i++)
+  start_.reserve (pagebreak_col_indices.size ());
+  for (vsize i = 0; i < pagebreak_col_indices.size (); i++)
     {
+      if (i)
+        {
+          assert (pagebreak_col_indices[i] > pagebreak_col_indices[i - 1]);
+        }
       vsize j;
-      for (j = 0; j + 1 < breaks_.size () && breaks_[j] < start_[i]; j++)
+      for (j = 0;
+           j + 1 < breaks_.size () && breaks_[j] < pagebreak_col_indices[i];
+           j++)
         ;
       starting_breakpoints_.push_back (j);
-      start_[i] = breaks_[j];
+      start_.push_back (breaks_[j]);
     }
   state_.resize (start_.size ());
 }
Index: lily/include/constrained-breaking.hh
diff --git a/lily/include/constrained-breaking.hh 
b/lily/include/constrained-breaking.hh
index 
f4eccddfd498f18b606eedd41b8cea92dc590652..693c6880f103b7c04cf86a272fdeadb25af20652
 100644
--- a/lily/include/constrained-breaking.hh
+++ b/lily/include/constrained-breaking.hh
@@ -196,7 +196,7 @@ private:
   std::vector<Paper_column *> all_;
   std::vector<vsize> breaks_;
 
-  void initialize (Paper_score *);
+  void initialize (Paper_score *, std::vector<vsize> const &break_col_indices);
   void resize (vsize systems);
 
   Column_x_positions space_line (vsize start_col, vsize end_col);
Index: lily/page-breaking.cc
diff --git a/lily/page-breaking.cc b/lily/page-breaking.cc
index 
7fda30b7dc09604550440102dd00dde584150d1b..5ba3c1021056a937fa94ccadd7357419a1972ae0
 100644
--- a/lily/page-breaking.cc
+++ b/lily/page-breaking.cc
@@ -730,6 +730,8 @@ Page_breaking::create_system_list ()
    with page-break-permission = 'force.
 
    By using a grob predicate, we can accommodate both of these uses.
+
+   is_break indicates if the column is an allowed page turn.
 */
 void
 Page_breaking::find_chunks_and_breaks (Break_predicate is_break, 
Prob_break_predicate prob_is_break)
@@ -776,7 +778,7 @@ Page_breaking::find_chunks_and_breaks (Break_predicate 
is_break, Prob_break_pred
                 }
 
               bool last = (j == cols.size () - 1);
-              bool break_point = is_break && is_break (cols[j]);
+              bool break_point = is_break && j > 0 && is_break (cols[j]);
               bool chunk_end = scm_is_eq (cols[j]->get_property 
("page-break-permission"), force_sym);
               Break_position cur_pos = Break_position (i,
                                                        
line_breaker_columns.size (),