Re: Carry multi-measure rests across voices in \partcombine (issue 265410043 by address@hidden)

[dak]

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterator.cc
File lily/part-combine-iterator.cc (right):

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterator.cc#newcode53
lily/part-combine-iterator.cc:53: void start_mmrest (Context *c, const
Moment &length);
We don't pass Moment by reference.  It's a simple scalar type and
references reached via unsmob are rarely guaranteed to survive for long.

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterator.cc#newcode132
lily/part-combine-iterator.cc:132: ev->set_property ("length",
length.smobbed_copy());
A multi-measure rest event having a length (which is a cache of the
"duration" converted to a Moment) but not a duration is very, very,
weird.

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterator.cc#newcode156
lily/part-combine-iterator.cc:156: {
Setting "duration" to SCM_EOL is just wrong.  Instead of setting lengths
(assuming that a Moment is all you have available), you should use
make-duration-of-length or similar in order to get a useful "duration"
value.

https://codereview.appspot.com/265410043/diff/60001/lily/part-combine-iterator.cc#newcode172
lily/part-combine-iterator.cc:172: Moment *rem = unsmob<Moment>
(c->get_property ("multiMeasureRestBusy"));
Please use robust_scm2moment here.

https://codereview.appspot.com/265410043/diff/60001/ly/engraver-init.ly
File ly/engraver-init.ly (right):

https://codereview.appspot.com/265410043/diff/60001/ly/engraver-init.ly#newcode841
ly/engraver-init.ly:841: %% by the part combiner.
Ugh.  It would appear that multi-measure rest splitting (or other rest
splitting) is only needed in the part combiner so why does it make sense
to add all this complication to the Multi_measure_rest_engraver and let
it idle in NullVoice?

Why not keep the code for dealing with this in the part combiner and
make the Multi_measure_rest_engraver just do what is expected from it?

https://codereview.appspot.com/265410043/diff/60001/scm/define-context-properties.scm
File scm/define-context-properties.scm (right):

https://codereview.appspot.com/265410043/diff/60001/scm/define-context-properties.scm#newcode480
scm/define-context-properties.scm:480: (multiMeasureRestBusy ,ly:moment?
"Remaining multi-measure rest duration.")
It seems like a strange deal to maintain an engraver-specific property
in a context property.  Most of the time, such variables are kept inside
the engraver.

Stupid question: would the partcombiner work better by having a context
PartCombinePart accepted by Voice, and then we use \change Voice =
"soloII" and similar in the two original parts?  Or would it make more
sense to put something like PartCombinePart between Staff and Voice and
run the original stuff in Voice, using \change PartCombinePart =
"soloII" and so on?  One of those might lead to a reasonably natural way
of dealing with slurs and stuff.

The reason I am asking in this context is that I think that you use
multiMeasureRestBusy for transplanting the internal state of a
Multi_measure_rest_engraver, and it may be expedient to instead
transplant the whole engraver by an appropriate use of context
hierarchy.

Just some food for thought.

https://codereview.appspot.com/265410043/