Hi Keith, your changes look good overall, but i think that NullVoice could be simplified even further - see below.
thanks for your work! https://codereview.appspot.com/117050043/diff/40001/input/regression/lyric-combine-nullvoice.ly File input/regression/lyric-combine-nullvoice.ly (right): https://codereview.appspot.com/117050043/diff/40001/input/regression/lyric-combine-nullvoice.ly#newcode12 input/regression/lyric-combine-nullvoice.ly:12: \autoBeamOff c4 r16 b,16~ b,8 c8[ e8 g8 e8] | g4( f4) g2 } It seems that the closing slur is one note too early. https://codereview.appspot.com/117050043/diff/40001/input/regression/lyric-combine-nullvoice.ly#newcode14 input/regression/lyric-combine-nullvoice.ly:14: \new Lyrics \lyricsto "nv" { free a -- lign -- ment } Shouldn't there be a lyric extender at the end? https://codereview.appspot.com/117050043/diff/40001/input/regression/lyric-combine-nullvoice.ly#newcode15 input/regression/lyric-combine-nullvoice.ly:15: >> } This regtest is nice, but as far as i can see it doesn't check whether issue 3825 has been fixed - i.e. the output looks the same before and after the change in the code. I think we should add the example from issue 3825 verbatim (as another regtest), and have a regtest corresponding to issue 3834. https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly File ly/engraver-init.ly (right): https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly#newcode787 ly/engraver-init.ly:787: \override NoteHead.meta.interfaces = #(delete 'rhythmic-head-interface What about making NullVoice \accepted not by \Staff, but by \Score (like Devnull)? From my test it seems that it would work and we wouldn't have to care about Rhythmic_column_engraver. https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly#newcode794 ly/engraver-init.ly:794: squashedPosition = 0 This should also become unneeded if we "move" NullVoice from Staff to Score. https://codereview.appspot.com/117050043/diff/40001/ly/engraver-init.ly#newcode798 ly/engraver-init.ly:798: \omit AccidentalSuggestion I think these are unneeded anyway. https://codereview.appspot.com/117050043/ _______________________________________________ lilypond-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/lilypond-devel
