Re: Colored box behind a single note

[Werner LEMBERG]

>>    if (d == STOP)
>>      {
>>        pop_count_++;
>> -      if (pop_count_ > bracket_stack_.size ())
>> +
>> +      // Since N `L' events create N HorizontalBracket grobs we need (at
>> +      // most) N `R' events to finish them.  However, at this particular
>> +      // moment, a single-moment horizontal bracket might be created also;
>> +      // this takes another `L' event (which might not have caused a new
>> +      // element on `bracket_stack' yet) and its corresponding `R' event.
>> +      if (pop_count_ > bracket_stack_.size () + 1)
>>          ev->origin ()->warning (_ ("do not have that many brackets"));
>>      }
> 
> "which might not have caused a new element"?  That sounds like this
> may or may not suppress an actually warranted warning.
> 
> Correct?

Right, thanks for noticing.  I've fixed this – to a certain extent,
see comments in the attached new version of the patch.

If someone is going to rewrite the algorithm, the order of events
should be obeyed.  To allow overlapping horizontal brackets, hopefully
the same mechanism as used for overlapping slurs can be used.


    Werner

diff --git a/lily/horizontal-bracket-engraver.cc 
b/lily/horizontal-bracket-engraver.cc
index 608af35965..4b18cc3ad7 100644
--- a/lily/horizontal-bracket-engraver.cc
+++ b/lily/horizontal-bracket-engraver.cc
@@ -28,6 +28,39 @@
 
 #include "translator.icc"
 
+// In the following (simplified) description of the algorithm, we abbreviate
+// a horizontal bracket's `START' and `STOP' span-direction with `L' and
+// `R', respectively.
+//
+// For a given moment do the following.
+//
+// (1) Count the number of `L' and `R' events; they are also pushed onto a
+//     stack so that they can be referenced later on (method
+//     `listen_note_grouping').
+//
+// (2) Create a HorizontalBracket spanner for every `L' event, link it as
+//     necessary, and push it onto `bracket_stack' (method `process_music').
+//
+// (3) Set one boundary for every element of `bracket_stack' (method
+//     `acknowledge_note_column').
+//
+// (4) Check whether there is a single `R' event within a bunch of `L' events
+//     (or a single `L' event within a bunch of `R' events) and set the
+//     other boundary with the same values to create a single-moment
+//     horizontal bracket for this case (method `acknowledge_note_column').
+//
+// (5) At a moment with a series of `R' events, `bracket_stack' normally
+//     contains elements created by `L' events from earlier moments (plus
+//     one possible single-moment horizontal bracket).  Pop an element off
+//     `bracket_stack' for every `R' event and ignore excessive `R' events
+//     (method `stop_translation_timestep').
+//
+// The above algorithm allows for nested horizontal brackets.
+//
+// TODO: Allow overlapping horizontal brackets (issue #5240); in particular,
+//       respect the order of `L' and `R' events for a given moment.  This
+//       would need a complete rewrite of the algorithm, however.
+
 class Horizontal_bracket_engraver : public Engraver
 {
 public:
@@ -56,10 +89,32 @@ Horizontal_bracket_engraver::listen_note_grouping 
(Stream_event *ev)
 {
   Direction d = to_dir (ev->get_property ("span-direction"));
 
+  // Allow one single-moment bracket.  This means that we can have
+  //
+  //   LLL...LLR
+  //
+  // or
+  //
+  //   LRRR...RR   .
+  //
+  // Note that the order of events doesn't matter; however, to avoid a false
+  // warning the single `L' event in a series of `R' events should not come
+  // last; similarly, the single `R' event in a series of `L' events should
+  // not come first.  In other words, both `RRRL' and `RLLL' are OK but make
+  // LilyPond complain without reason.  I think it is not worth fixing this,
+  // given that the algorithm should be rewritten anyway.
+
   if (d == STOP)
     {
       pop_count_++;
-      if (pop_count_ > bracket_stack_.size ())
+
+      // Since N `L' events create N HorizontalBracket grobs we need (at
+      // most) N `R' events to finish them.  However, at this particular
+      // moment, a single-moment horizontal bracket might be created also;
+      // this takes another `L' event and its corresponding `R' event.
+      int extra = (pop_count_ && push_count_) ? 1 : 0;
+
+      if (pop_count_ > bracket_stack_.size () + extra)
         ev->origin ()->warning (_ ("do not have that many brackets"));
     }
   else
@@ -68,22 +123,41 @@ Horizontal_bracket_engraver::listen_note_grouping 
(Stream_event *ev)
       events_.push_back (ev);
     }
 
-  if (pop_count_ && push_count_)
+  // The handled number of events are
+  //
+  //   LLL...LLR   =>   push_count_ >= 1 && pop_count_ == 1
+  //
+  // or
+  //
+  //   LRR...RRR   =>   pop_count_ >= 1 && push_count_ == 1   .
+  //
+  if (pop_count_ >= 2 && push_count_ >= 2)
     ev->origin ()->warning (_ ("conflicting note group events"));
 }
 
 void
 Horizontal_bracket_engraver::acknowledge_note_column (Grob_info gi)
 {
+  bool process_single_moment_bracket = pop_count_ && push_count_;
+
   for (vsize i = 0; i < bracket_stack_.size (); i++)
     {
-      Side_position_interface::add_support (bracket_stack_[i], gi.grob ());
-      Pointer_group_interface::add_grob (bracket_stack_[i],
-                                         ly_symbol2scm ("columns"), gi.grob 
());
-      add_bound_item (bracket_stack_[i],
-                      gi.grob ());
-      add_bound_item (text_stack_[i],
-                      gi.grob ());
+      // For a single-moment horizontal bracket (which is the most recently
+      // pushed element of `bracket_stack'), use the current note column for
+      // both the left and right bound of the bracket.
+      int count = (process_single_moment_bracket
+                   && i + 1 == bracket_stack_.size ()) ? 2 : 1;
+
+      for (int j = 0; j < count; j++)
+        {
+          Side_position_interface::add_support (bracket_stack_[i],
+                                                gi.grob ());
+          Pointer_group_interface::add_grob (bracket_stack_[i],
+                                             ly_symbol2scm ("columns"),
+                                             gi.grob ());
+          add_bound_item (bracket_stack_[i], gi.grob ());
+          add_bound_item (text_stack_[i], gi.grob ());
+        }
     }
 }