I've started going through with the code review here but at some point decided that I was missing the elephant in the room. So don't really bother with the code-related commment.
The main problem I see with the current code is that it is ad-hoc and basically unmaintainable. Spanner_engraver is a class that really serves no clear purpose apart from carrying a few helper functions. The collection of helper functions does not create a coherent design. All the actual management of multiple spanners still happens in the spanner classes themselves. One indicator of a lack of coherence is that you did not touch Slur_engraver (which has a working multiple-spanner implementation of its own, without the multiple-context part). Having a working generic multiple-spanner implementation should have _simplified_ the code in Slur_engraver by moving the stuff dealing with multiple events out. So the question is how would we _want_ multiple-spanner management to be reflected in the code? It's complex and mostly orthogonal to the actual work (except when multiple spanners or likely rather their grobs have to notice each other in order to avoid collisions), so the answer is "as little as possible". The ideal implementation would be to change ASSIGN_EVENT_ONCE to ASSIGN_EVENT_SPANNER_ID and not change anything else. However, the amount of trickery that would need to get done then would be considerable. However, the principle underlying this somewhat unrealistic example is still valid: the bulk of the code should only cater for a single spanner. If we can arrive there, we will also be in a good position to let users create Scheme engravers that can deal with multiple spanners without having to delve into multiple-spanner management themselves. So let's get back to a good API here: the ASSIGN_EVENT_ONCE proposal would be too tricky to pull off, but your idea of using a Spanner_engraver base class has merit. What should this Spanner_engraver do? Well, if the instance is supposed to deal only with a subset of the arriving events, it is not going to be a true Engraver but merely something behaving like one. It will have a pointer to the actual Engraver class (assuming that it needs it) and implement most of the function calls from an Engraver class by reflecting them to the actual engraver. The add_acknowledger/listener functions will probably get a companion add_multi_acknowledger/listener that will only admit events/grobs matching the respective spanner id. That's the main filtering that appears to be necessary. So basically Spanner_engraver would entertain additional acknowledger/listener arrays routed through a hash table routed via spanner_id. So that's the basic hand-wavy sketch of how to abstract the multiple-spanner management into a class of its own _without_ significantly complicating the spanner classes themselves. Basically the key metric would be that lily/slur-engraver.cc should become simpler to work on and with than it is now by having the multiple-spanner management funneled off into a separate class and retaining only management for a single spanner. If the additional code/classes does not manage to _simplify_ reading lily/slur-engraver.cc once it makes it in there, it is going to make writing maintainable multi-spanner code harder, not easier. If that abstraction is successful, adding the multiple-context management on top of the multiple-spanner-id management should require almost no additional code in slur-engraver.cc itself rather than in the spanner management class. So the question is how much of a chance we have to get where reusing the same multiple-id/multiple-context code for both dynamic spanners and for Slur_engraver (in a way where the ad-hoc multi-spanner management of Slur_engraver is mostly removed from the Slur_engraver class itself) becomes feasible. That's basically my take on this, focused more on the matter of how LilyPond can better be extended and maintained in future rather than on whether the proposed code will work for the intended purpose correctly right now. I trust that you tested it to do that, but for it to be of best use in future, its operation should be better separated from the code it affects. https://codereview.appspot.com/304160043/diff/20001/input/regression/dynamics-alignment-autobreak.ly File input/regression/dynamics-alignment-autobreak.ly (right): https://codereview.appspot.com/304160043/diff/20001/input/regression/dynamics-alignment-autobreak.ly#newcode19 input/regression/dynamics-alignment-autobreak.ly:19: << This code is not really seminal to the topic of this regtest. You should rather create one or more separate regtests for the new functionality and have a matching description. The same for the other regtests. https://codereview.appspot.com/304160043/diff/20001/lily/dynamic-align-engraver.cc File lily/dynamic-align-engraver.cc (right): https://codereview.appspot.com/304160043/diff/20001/lily/dynamic-align-engraver.cc#newcode124 lily/dynamic-align-engraver.cc:124: started_.insert (started_.begin (), info); Inserting at the beginning of the vector is an O(n) operation, leading to an O(n^2) overall behavior. Wouldn't it make more sense to push into a separate structure here like it was originally done? What is the problem you are trying to solve by using only one structure? https://codereview.appspot.com/304160043/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel