Re: reopened Issue 2584: please make partcombine merge slurs (issue 6432047)

[dak]

Reviewers: Trevor Daniels,

Message:
On 2012/07/19 09:30:15, Trevor Daniels wrote:
> Just nitpicking; can't comment substantively.

> Trevor


http://codereview.appspot.com/6432047/diff/1/lily/phrasing-slur-engraver.cc
> File lily/phrasing-slur-engraver.cc (right):


http://codereview.appspot.com/6432047/diff/1/lily/phrasing-slur-engraver.cc#newcode256
> lily/phrasing-slur-engraver.cc:256: slurs_[j]->suicide ();
> no tabs please


http://codereview.appspot.com/6432047/diff/1/lily/phrasing-slur-engraver.cc#newcode258
> lily/phrasing-slur-engraver.cc:258: continue;
> no tabs please

> http://codereview.appspot.com/6432047/diff/1/lily/slur-engraver.cc
> File lily/slur-engraver.cc (right):


http://codereview.appspot.com/6432047/diff/1/lily/slur-engraver.cc#newcode257
> lily/slur-engraver.cc:257: slurs_[j]->suicide ();
> no tabs please


http://codereview.appspot.com/6432047/diff/1/lily/slur-engraver.cc#newcode259
> lily/slur-engraver.cc:259: continue;
> no tabs please

Ran M-x untabify RET on both files, but won't recommit to testing for
just this spacing change.

Description:
reopened Issue 2584: please make partcombine merge slurs

This refrains from referencing a slur grob's direction field (often
inconclusive and not representative of the input), instead referencing
the direction of the causing event.

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

Affected files:
  M lily/phrasing-slur-engraver.cc
  M lily/slur-engraver.cc


Index: lily/phrasing-slur-engraver.cc
diff --git a/lily/phrasing-slur-engraver.cc b/lily/phrasing-slur-engraver.cc
index  
ff1bd4e51d5d0581b4dc5a7cad08c9d6aac70262..b87d649e015d8d3e01ada65077fb8774d99f22ec  
100644
--- a/lily/phrasing-slur-engraver.cc
+++ b/lily/phrasing-slur-engraver.cc
@@ -218,7 +218,7 @@ Phrasing_slur_engraver::process_music ()
       Direction updown = to_dir (ev->get_property ("direction"));

       bool completed;
-      for (vsize j = 0; !(completed = (j == slurs_.size ())); j++)
+      for (vsize j = slurs_.size (); !(completed = (j-- == 0));)
         {
           // Check if we already have a slur with the same spanner-id.
           if (id == robust_scm2string (slurs_[j]->get_property  
("spanner-id"), ""))
@@ -234,31 +234,28 @@ Phrasing_slur_engraver::process_music ()

               // If this slur event has no direction, it will not
               // contribute anything new to the existing slur(s), so
-              // we can ignore it.  This is not entirely accurate:
-              // tweaks or context properties like those set with
-              // \slurUp can still override a neutral direction, so
-              // when encountering a slur event with "opposite"
-              // direction first, then one with neutral direction, we
-              // only let the "opposite" direction remain, while if
-              // the order is the other way round, a double slur
-              // results since the direction of the first slur is no
-              // longer attributable to a "neutral" slur event.  A
-              // mixture of neutral and directed events is nothing
-              // that the partcombiner should crank out, and it would
-              // be decidedly strange for manual input.
+              // we can ignore it.

               if (!updown)
                 break;

-              // If the existing slur does not have a direction yet,
-              // give it ours
+	      Stream_event *c = unsmob_stream_event (slurs_[j]->get_property  
("cause"));
+
+             if (!c) {
+               slurs_[j]->programming_error ("phrasing slur without a cause");
+               continue;
+             }

-              Direction slur_dir = to_dir (slurs_[j]->get_property  
("direction"));
+              Direction slur_dir = to_dir (c->get_property ("direction"));
+
+              // If the existing slur does not have a direction yet,
+              // we'd rather take the new one.

               if (!slur_dir)
                 {
-                  set_grob_direction (slurs_[j], updown);
-                  break;
+                 slurs_[j]->suicide ();
+                  slurs_.erase (slurs_.begin () + j);
+                 continue;
                 }

               // If the existing slur has the same direction as ours, drop  
ours
Index: lily/slur-engraver.cc
diff --git a/lily/slur-engraver.cc b/lily/slur-engraver.cc
index  
fde4d9447def65123f93532dc8421a8420e9a634..d88ff4ea5776dd17ed4b32bc57ef95adb020f21f  
100644
--- a/lily/slur-engraver.cc
+++ b/lily/slur-engraver.cc
@@ -219,7 +219,7 @@ Slur_engraver::process_music ()
       Direction updown = to_dir (ev->get_property ("direction"));

       bool completed;
-      for (vsize j = 0; !(completed = (j == slurs_.size ())); j++)
+      for (vsize j = slurs_.size (); !(completed = (j-- == 0));)
         {
           // Check if we already have a slur with the same spanner-id.
           if (id == robust_scm2string (slurs_[j]->get_property  
("spanner-id"), ""))
@@ -235,31 +235,28 @@ Slur_engraver::process_music ()

               // If this slur event has no direction, it will not
               // contribute anything new to the existing slur(s), so
-              // we can ignore it.  This is not entirely accurate:
-              // tweaks or context properties like those set with
-              // \slurUp can still override a neutral direction, so
-              // when encountering a slur event with "opposite"
-              // direction first, then one with neutral direction, we
-              // only let the "opposite" direction remain, while if
-              // the order is the other way round, a double slur
-              // results since the direction of the first slur is no
-              // longer attributable to a "neutral" slur event.  A
-              // mixture of neutral and directed events is nothing
-              // that the partcombiner should crank out, and it would
-              // be decidedly strange for manual input.
+              // we can ignore it.

               if (!updown)
                 break;

-              // If the existing slur does not have a direction yet,
-              // give it ours
+	      Stream_event *c = unsmob_stream_event (slurs_[j]->get_property  
("cause"));
+
+             if (!c) {
+               slurs_[j]->programming_error ("slur without a cause");
+               continue;
+             }

-              Direction slur_dir = to_dir (slurs_[j]->get_property  
("direction"));
+              Direction slur_dir = to_dir (c->get_property ("direction"));
+
+              // If the existing slur does not have a direction yet,
+              // we'd rather take the new one.

               if (!slur_dir)
                 {
-                  set_grob_direction (slurs_[j], updown);
-                  break;
+                 slurs_[j]->suicide ();
+                  slurs_.erase (slurs_.begin () + j);
+                 continue;
                 }

               // If the existing slur has the same direction as ours, drop  
ours