Re: [GSoC] Implement cross-voice dynamic spanners (issue 304160043 by address@hidden)

[dak]

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/