Re: Automatic LyricExtenders (issue 313240043 by address@hidden)

[dak]

It's not really clear to me where I am supposed to go from here with
what I proposed.  I lean towards going back to creating my own version
of this again since this one contains so much stuff that does not ring a
bell with me.


https://codereview.appspot.com/313240043/diff/200001/lily/extender-engraver.cc
File lily/extender-engraver.cc (right):

https://codereview.appspot.com/313240043/diff/200001/lily/extender-engraver.cc#newcode33
lily/extender-engraver.cc:33: #include "iostream"
Why?

https://codereview.appspot.com/313240043/diff/200001/lily/extender-engraver.cc#newcode87
lily/extender-engraver.cc:87: ASSIGN_EVENT_ONCE (hy_, ev);
If we are not the primary engraver for an item, we should not complain
about how often an event is assigned I think.

https://codereview.appspot.com/313240043/diff/200001/lily/extender-engraver.cc#newcode109
lily/extender-engraver.cc:109: if (ex_ && !hy_)
Why check for hy_ here?  If there is an explicit extender event, why
would we mess with it?

https://codereview.appspot.com/313240043/diff/200001/lily/extender-engraver.cc#newcode112
lily/extender-engraver.cc:112: extender_->set_property
("force-extender", SCM_BOOL_T);
I think it might make more sense to have, if at all, a property for
implicitly generated extenders so that any extender generated by a
user-created engraver sounds as forced.  We could probably could just
look up the event_cause and see whether it is a lyric-event, but when
user-created extenders are expected at all, it might probably be
advisable to have a more explicit property one can set to switch between
explicit and implicit behavior.

https://codereview.appspot.com/313240043/diff/200001/lily/lyric-extender.cc
File lily/lyric-extender.cc (right):

https://codereview.appspot.com/313240043/diff/200001/lily/lyric-extender.cc#newcode54
lily/lyric-extender.cc:54: bool no_auto_extenders = to_boolean
(me->get_property ("no-auto-extenders"));
That is awful.  Creating extenders with properties like "no-extender" or
"no-auto-extenders" in order to decide whether one did not want an
extender in the first place is nonsensical.  If we want to suppress
extenders on anything but visual appearance, we should have context
properties for that and not even generate them.

Because we might want to be able to create similar behavior in Midi or
other backends where no grobs are available.

https://codereview.appspot.com/313240043/diff/200001/lily/lyric-extender.cc#newcode63
lily/lyric-extender.cc:63: while (hs && robust_scm2double
(heads[hs-1]->get_property
Same as previously I see.

https://codereview.appspot.com/313240043/diff/200001/lily/self-alignment-interface.cc
File lily/self-alignment-interface.cc (right):

https://codereview.appspot.com/313240043/diff/200001/lily/self-alignment-interface.cc#newcode63
lily/self-alignment-interface.cc:63: ly_symbol2scm
("lyric-syllable-interface"))))
Why is this necessary?

https://codereview.appspot.com/313240043/diff/200001/lily/self-alignment-interface.cc#newcode146
lily/self-alignment-interface.cc:146: ly_symbol2scm
("lyric-syllable-interface"))))
And this?

https://codereview.appspot.com/313240043/diff/200001/lily/self-alignment-interface.cc#newcode156
lily/self-alignment-interface.cc:156: ly_symbol2scm
("lyric-syllable-interface"))))
And this?

Shouldn't we rather try to fix the condition rather than remove the
warning?

https://codereview.appspot.com/313240043/diff/200001/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

https://codereview.appspot.com/313240043/diff/200001/scm/define-grob-properties.scm#newcode188
scm/define-grob-properties.scm:188: (collapse-length ,ly:dimension? "An
automatically generated
collapse-width maybe?  Length is more like a list length?

https://codereview.appspot.com/313240043/diff/200001/scm/define-grob-properties.scm#newcode684
scm/define-grob-properties.scm:684: (no-auto-extenders ,boolean?
"Inhibits automatic generation of lyric
Why do we need this also?

There are just too many properties.

https://codereview.appspot.com/313240043/