Re: Narrowly target warnings for multiple slurs; Issue 1967 (issue 6272046)

[k-ohara5a5a]

Reviewers: dak,

Message:
Patch Set 3 shows the revert of the original patch for issue 1967, so
you can use "delta from patch set 3" to see the re-implementation of the
fix that preserves the functionality of partcombine.


http://codereview.appspot.com/6272046/diff/5/input/regression/slur-multiple.ly
File input/regression/slur-multiple.ly (right):

http://codereview.appspot.com/6272046/diff/5/input/regression/slur-multiple.ly#newcode17
input/regression/slur-multiple.ly:17: \relative c'' {
On 2012/06/03 10:08:54, dak wrote:
> If you want a different test case, maybe add it to the end?
No, I cannot both fix issue 1967 and leave the old test case in place,
because the old test case generated a warning for c4(( which is a
pattern generated by partcombine.  I want to remove that warning, but
this regtest expects warnings.

> Otherwise one can't
> judge the effect of your changes on the existing regtest.
I wiĺl put the old test output on the tracker.

http://codereview.appspot.com/6272046/diff/5/lily/slur-engraver.cc
File lily/slur-engraver.cc (right):

http://codereview.appspot.com/6272046/diff/5/lily/slur-engraver.cc#newcode185
lily/slur-engraver.cc:185: ended = true;
On 2012/06/03 10:08:54, dak wrote:
> Why is this change going to work with double slurs?  Those have
multiple start
> events and only one end event.

Double slurs have only one start event.  If the property doubleSlurs is
true, then two slurs are created from that one event (line 221ff), both
both with the same spanner-id.  Each slur is ended at their common end
event, on successive passes through this loop.

http://codereview.appspot.com/6272046/diff/5/lily/slur-engraver.cc#newcode212
lily/slur-engraver.cc:212: else
On 2012/06/03 10:08:54, dak wrote:
> What difference do you expect from this change?  it is not clear to
me.

None.  This part of the patch is just a revert of the commit that caused
the regression.  I'll repost as a series.

Description:
When \partcombine merges two streams into one voice, it can create
duplicate slurs (with the same spanner-id) in these patterns
  { <c e>(( <d f>))  <c e>(( d) c( <d f>)) }
All should show a single merged slur.  We must end these slurs at the d,
preferably without warning, but should still warn for other cases that
are more likely input errors.

This is a re-implementation of the original patch for issue 1967, which
broke partcombine.

Please review this at http://codereview.appspot.com/6272046/

Affected files:
  M input/regression/phrasing-slur-multiple.ly
  M input/regression/slur-multiple.ly
  M lily/phrasing-slur-engraver.cc
  M lily/slur-engraver.cc


Index: input/regression/phrasing-slur-multiple.ly
diff --git a/input/regression/phrasing-slur-multiple.ly  
b/input/regression/phrasing-slur-multiple.ly
index  
f9ccca61b257ba55a54807c46ee8bc026b71ad9b..9d16546fafb5903bb594acf86da7e644cd7deda9  
100644
--- a/input/regression/phrasing-slur-multiple.ly
+++ b/input/regression/phrasing-slur-multiple.ly
@@ -16,7 +16,7 @@ altPhSlurEnd =  
#(make-music 'PhrasingSlurEvent 'span-direction STOP 'spanner-id

 \relative c'' {
   % This will give warnings ("Already have phrasing slur" and "Cannot end  
phrasing slur")
-  c4\(\( d4\)\( e4\) f\) |
+  c4\( d\( e\) f\) |
   % This will give two overlapping slurs:
   d\(  d\altPhSlur e\) f\altPhSlurEnd |

Index: input/regression/slur-multiple.ly
diff --git a/input/regression/slur-multiple.ly  
b/input/regression/slur-multiple.ly
index  
f73d93a3166d29ce4c2afb5da56b300c57b542f3..57c5e8bc826a3dd8f1a6abc5fc039d65fab4d792  
100644
--- a/input/regression/slur-multiple.ly
+++ b/input/regression/slur-multiple.ly
@@ -14,10 +14,10 @@ a different spanner-id."
 altSlur = #(make-music 'SlurEvent 'span-direction START 'spanner-id "alt")
 altSlurEnd = #(make-music 'SlurEvent 'span-direction  
STOP 'spanner-id "alt")

-\relative c'' {
+\relative c'' {
   % This will give warnings ("Already have slur" and "Cannot end slur")
-  c4(( d4)( e4) f) |
+  c4( d( e) f) |
   % This will give two overlapping slurs:
   d(  d\altSlur e) f\altSlurEnd |
-
+
 }
Index: lily/phrasing-slur-engraver.cc
diff --git a/lily/phrasing-slur-engraver.cc b/lily/phrasing-slur-engraver.cc
index  
a3b10d027a158472ef1309b3bfb66aa7c00c5d0b..0875c1b03d7b4a5fb9b0a8b545abd3d0d9f7f77c  
100644
--- a/lily/phrasing-slur-engraver.cc
+++ b/lily/phrasing-slur-engraver.cc
@@ -170,13 +170,13 @@ Phrasing_slur_engraver::finalize ()
 void
 Phrasing_slur_engraver::process_music ()
 {
+  bool ended = false;
   for (vsize i = 0; i < stop_events_.size (); i++)
     {
       Stream_event *ev = stop_events_[i];
       string id = robust_scm2string (ev->get_property ("spanner-id"), "");

       // Find the slur that is ended with this event (by checking the  
spanner-id)
-      bool ended = false;
       for (vsize j = slurs_.size (); j--;)
         {
           if (id == robust_scm2string (slurs_[j]->get_property  
("spanner-id"), ""))
@@ -190,6 +190,7 @@ Phrasing_slur_engraver::process_music ()
         ev->origin ()->warning (_ ("cannot end phrasing slur"));
     }

+  vsize n_old_slurs = slurs_.size ();
   for (vsize i = 0; i < start_events_.size (); i++)
     {
       Stream_event *ev = start_events_[i];
@@ -197,7 +198,7 @@ Phrasing_slur_engraver::process_music ()
       bool have_slur = false;
       // Check if we already have a slur with the same spanner-id.
       // In that case, don't create a new slur, but print a warning
-      for (vsize i = 0; i < slurs_.size (); i++)
+      for (vsize i = 0; i < n_old_slurs; i++)
         have_slur = have_slur || (id == robust_scm2string  
(slurs_[i]->get_property ("spanner-id"), ""));

       if (have_slur)
Index: lily/slur-engraver.cc
diff --git a/lily/slur-engraver.cc b/lily/slur-engraver.cc
index  
b0ef9f90ad21d23142b7b25f5cc4c22594f16be9..519c244a987ccccf06e99e12a9a48341b3dc2c07  
100644
--- a/lily/slur-engraver.cc
+++ b/lily/slur-engraver.cc
@@ -171,34 +171,27 @@ Slur_engraver::finalize ()
 void
 Slur_engraver::process_music ()
 {
+  bool ended = false;
   for (vsize i = 0; i < stop_events_.size (); i++)
     {
       Stream_event *ev = stop_events_[i];
       string id = robust_scm2string (ev->get_property ("spanner-id"), "");

       // Find the slur that is ended with this event (by checking the  
spanner-id)
-      bool ended = false;
-      SCM starter = SCM_BOOL_F;
       for (vsize j = slurs_.size (); j--;)
         {
           if (id == robust_scm2string (slurs_[j]->get_property  
("spanner-id"), ""))
             {
-              // We end only one slur unless several ones have been
-              // caused by the same event, like with double slurs.
-              if (!ended || scm_is_eq (starter,
-                                       slurs_[j]->get_property ("cause")))
-                {
-                  ended = true;
-                  starter = slurs_[j]->get_property ("cause");
-                  end_slurs_.push_back (slurs_[j]);
-                  slurs_.erase (slurs_.begin () + j);
-                }
+              ended = true;
+              end_slurs_.push_back (slurs_[j]);
+              slurs_.erase (slurs_.begin () + j);
             }
         }
       if (!ended)
         ev->origin ()->warning (_ ("cannot end slur"));
     }

+  vsize n_old_slurs = slurs_.size ();
   for (vsize i = start_events_.size (); i--;)
     {
       Stream_event *ev = start_events_[i];
@@ -206,7 +199,7 @@ Slur_engraver::process_music ()
       bool have_slur = false;
       // Check if we already have a slur with the same spanner-id.
       // In that case, don't create a new slur, but print a warning
-      for (vsize j = 0; j < slurs_.size (); j++)
+      for (vsize j = 0; j < n_old_slurs; j++)
         have_slur = have_slur || (id == robust_scm2string  
(slurs_[j]->get_property ("spanner-id"), ""));

       if (have_slur)
@@ -216,26 +209,23 @@ Slur_engraver::process_music ()
           ev->origin ()->warning (_ ("already have slur"));
           start_events_.erase (start_events_.begin () + i);
         }
-    }
-  for (vsize i = start_events_.size (); i--;)
-    {
-      Stream_event *ev = start_events_[i];
-      string id = robust_scm2string (ev->get_property ("spanner-id"), "");
-
-      Grob *slur = make_spanner ("Slur", ev->self_scm ());
-      Direction updown = to_dir (ev->get_property ("direction"));
-      slur->set_property ("spanner-id", ly_string2scm (id));
-      if (updown)
-        set_grob_direction (slur, updown);
-      slurs_.push_back (slur);
-
-      if (to_boolean (get_property ("doubleSlurs")))
+      else
         {
-          set_grob_direction (slur, DOWN);
-          slur = make_spanner ("Slur", ev->self_scm ());
+          Grob *slur = make_spanner ("Slur", ev->self_scm ());
+          Direction updown = to_dir (ev->get_property ("direction"));
           slur->set_property ("spanner-id", ly_string2scm (id));
-          set_grob_direction (slur, UP);
+          if (updown)
+            set_grob_direction (slur, updown);
           slurs_.push_back (slur);
+
+          if (to_boolean (get_property ("doubleSlurs")))
+            {
+              set_grob_direction (slur, DOWN);
+              slur = make_spanner ("Slur", ev->self_scm ());
+              slur->set_property ("spanner-id", ly_string2scm (id));
+              set_grob_direction (slur, UP);
+              slurs_.push_back (slur);
+            }
         }
     }
   set_melisma (slurs_.size ());