Stuff is not getting better by myself procrastinating any longer, so I'm putting out the current review, lacklustre as it may seem given the time I let pass on it.
https://codereview.appspot.com/304160043/diff/80001/lily/dynamic-align-engraver.cc File lily/dynamic-align-engraver.cc (right): https://codereview.appspot.com/304160043/diff/80001/lily/dynamic-align-engraver.cc#newcode57 lily/dynamic-align-engraver.cc:57: Grob *script_; Ok, I'm somewhat confused here. The code already was dealing with multiple spanners before? While you are perfectly keeping the quota of comments intact, the previous state was programmed in a "standard" manner, and the previous state of the comments was a hurdle to readers that does not really warrant continuing in the same style: basically it is an artifact of LilyPond's two-person past and the confidence of young programmers to live as long as the code lasts. Now you are reorganizing things in a tentatively new manner, but you don't document what you are doing. That's one of the reasons for myself procrastinating on the review: I expect to be able to understand complex stuff but when it's no fun doing so, I am running away before getting anywhere. Which is the wrong reaction. Other people have their eyes glaze over and pass on, but as long as they did not promise a review... Sorry for that. At any rate, you need to state here what is organised by what, and sketch how Dynamic_engraver (which already started out organizing multiple engravers and thus is _not_ behaving like slurs and other multi-instance engravers) fits into Spanner_engraver with its particular needs. https://codereview.appspot.com/304160043/diff/80001/lily/dynamic-align-engraver.cc#newcode99 lily/dynamic-align-engraver.cc:99: if (current_dynamic_spanner_ == dynamic) What's the relation between current_spanner_, current_dynamic_spanner_, and dynamic ? That's not clear in comparison to the original code. https://codereview.appspot.com/304160043/diff/80001/lily/dynamic-engraver.cc File lily/dynamic-engraver.cc (right): https://codereview.appspot.com/304160043/diff/80001/lily/dynamic-engraver.cc#newcode148 lily/dynamic-engraver.cc:148: current_span_event_->get_property ("spanner-share-context"), Wouldn't make_multi_spanner be able to consult spanner-share-context and spanner-id on its own? https://codereview.appspot.com/304160043/diff/80001/lily/include/engraver-group.hh File lily/include/engraver-group.hh (right): https://codereview.appspot.com/304160043/diff/80001/lily/include/engraver-group.hh#newcode56 lily/include/engraver-group.hh:56: friend class Spanner_engraver; What is the friendship needed for in particular? When it's not too much effort, it is nice to get along without friends, but if one does lean on them, it makes sense keeping book on what you need them for. This is not life advice. https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engraver.hh File lily/include/spanner-engraver.hh (right): https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engraver.hh#newcode15 lily/include/spanner-engraver.hh:15: // Context property spannerEngravers is an alist: Ok, here is the principal problem with this module: you are microdocumenting it. There is no description how to use it, no description how it fits into things, no description of what it does (only details of how it does it, but not forming a coherent picture), no description of its expectations and what it can and cannot be used for. Yes, this is a bit par for the course for much of LilyPond's code base. But it's not right. For some interfacing module that attempts to do a slightly better job at sketching how things fit into place, see lily/include/smobs.hh . Still documented comparatively briefly, but the microdocumentation then has an overall _context_ sketched out and you can check how the code falls into place regarding its purpose and design. This is really missing here and it makes it hard to review because it is all a heap of code with local descriptions and no picture of what it does and what it connects with in order to abstract what task given which parameters and boundary conditions. https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engraver.hh#newcode16 lily/include/spanner-engraver.hh:16: // ( #(engraver-class-symbol share-context spanner-id) . engraver-list ) That sounds like something better maintained in internal variables of Spanner_engraver rather than a context property? https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engraver.hh#newcode102 lily/include/spanner-engraver.hh:102: template <class T, void (T::*callback) (Grob_info)> I'm not fond of large, comparatively generic template functions, and there are a lot of them. Would it make sense to abstract some of this into non-templated functions? https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engraver.hh#newcode111 lily/include/spanner-engraver.hh:111: if (is_manager_) Ok, this is_manager_ concept seems like a bad fit with regard to the overall data structures. It means that every engraver instance here has all the functions and data structures needed to manage multiple engravers. What are alternative means of doing this? The simplest would likely be to have the multiple spanner class be templated on the class it is controlling. It would then create actual engravers as needed. To get equivalent behavior, it might seem that one engraver instance should be created right away. However, since the multiple-spanner-ready engravers need to be prepared to be created later than that anyway, I think I'd just create them as needed even for the default one. The advantage would be that the hierarchy would be much cleaner: currently I feel that there is code distributed up and down, making it harder to track responsibilities and consequently also for figuring out how to work this into slurs and others. Now the question is how such a rearrangement would best be organized: we don't want the code fall dead, but the time allotment for the project also is basically over. See a perspective here time and motivation getting this simplified? https://codereview.appspot.com/304160043/diff/80001/lily/include/spanner-engraver.hh#newcode121 lily/include/spanner-engraver.hh:121: if (ly_is_equal (id, filter_id_) && ly_is_equal (share->self_scm (), filter_share_->self_scm ())) Yes, this is ugly: basically having the managing class duplicated everywhere receiving everything and sorting it out. It would be much nicer to have a single instance receiving the events and distributing them. https://codereview.appspot.com/304160043/ _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel