Automatic LyricExtenders

Automatic LyricExtenders

Fixes https://sourceforge.net/p/testlilyissues/issues/4509

As written and communicated by Knut Peterson.
For reference, see the threads
  http://lists.gnu.org/archive/html/lilypond-devel/2016-12/msg00223.html
  http://lists.gnu.org/archive/html/lilypond-devel/2016-12/msg00121.html
  http://lists.gnu.org/archive/html/lilypond-user/2016-12/msg00523.html
  http://lists.gnu.org/archive/html/lilypond-user/2016-12/msg00303.html
(sorry, this discussion grew quite long in the meantime).

[akobel, 2016-12-23 21:34:49]

For the sake of completeness: deserves an entry in changes.tely (TBD when the
syntax discussion is over that I expect to start some time soon...)

[akobel, 2016-12-24 00:11:09]

Lyric autoextenders: Silence some warnings

[Trevor Daniels, 2016-12-25 10:52:13]

LGTM, with the trivial suggestions indicated below to the NR, but I'm not
competent to check the code sections.

The use of "__" has been expunged completely:  should we not retain a brief
mention perhaps for use when the user has turned auto extenders off?

Trevor

https://codereview.appspot.com/313240043/diff/20001/Documentation/notation/vo...
File Documentation/notation/vocal.itely (right):

https://codereview.appspot.com/313240043/diff/20001/Documentation/notation/vo...
Documentation/notation/vocal.itely:639: syllable to the last note of the
melisma. Lilypond detects places
The convention we use is two spaces between sentences.

https://codereview.appspot.com/313240043/diff/20001/Documentation/notation/vo...
Documentation/notation/vocal.itely:853: to num staffspaces. No lyric extenders
shorter than this will be automatically
@var{num}

https://codereview.appspot.com/313240043/diff/20001/Documentation/notation/vo...
Documentation/notation/vocal.itely:857: @code{\earlyExtender #num} generates a
lyric extender that starts num
@var{num}

https://codereview.appspot.com/313240043/diff/20001/Documentation/notation/vo...
Documentation/notation/vocal.itely:860: @code{\forceExtender} forces a lyric
extenders where none would be generated
"extender"

https://codereview.appspot.com/313240043/diff/20001/Documentation/notation/vo...
Documentation/notation/vocal.itely:863: @code{\forceExtenderTo #num} forces a
lyric extenders of length num
"extender"; @var{num}

https://codereview.appspot.com/313240043/diff/20001/Documentation/notation/vo...
Documentation/notation/vocal.itely:870: num staffspaces left from the normal
endpoint.
@var{num}

https://codereview.appspot.com/313240043/diff/20001/Documentation/notation/vo...
Documentation/notation/vocal.itely:1285: \set associatedVoice = "melody"
This \set associatedVoice is also no longer necessary

https://codereview.appspot.com/313240043/diff/20001/Documentation/notation/vo...
Documentation/notation/vocal.itely:1445: @funindex \repeatTie
@funindex \earlyExtender
@cindex extender, early
@funindex \shortenExtender
@cindex extender, shorten

https://codereview.appspot.com/313240043/diff/20001/Documentation/notation/vo...
Documentation/notation/vocal.itely:1497: around the lyrics and @code{\skip}
commands need to be inserted manually, as
Replace \skip command

[akobel, 2016-12-25 14:03:09]

Tell parser to ignore "__" token

[akobel, 2016-12-25 14:27:25]

https://codereview.appspot.com/313240043/diff/40001/Documentation/notation/vo...
File Documentation/notation/vocal.itely (right):

https://codereview.appspot.com/313240043/diff/40001/Documentation/notation/vo...
Documentation/notation/vocal.itely:842: @samp{ \markup\null } in these cases.
I'm only 90% happy about "" and \markup\null...
For dynamic spanners and hairpins, we have \! to end them prematurely before a
"natural" end event appears.  Is this similar enough to warrant the use of the
same token (i.e., \! would be translated to empty lyrics with a
CompletizeExtenderEvent), and is it possible at all?  Otherwise, should we have
a special token for this (\. comes to my mind)?

[Knut_Petersen_t-online.de, 2016-12-25 15:43:05]

Hi everybody!

> I'm only 90% happy about "" and \markup\null...
> For dynamic spanners and hairpins, we have \! to end them prematurely
> before a "natural" end event appears.  Is this similar enough to warrant
> the use of the same token (i.e., \! would be translated to empty lyrics
> with a CompletizeExtenderEvent), and is it possible at all? Otherwise,
> should we have a special token for this (\. comes to my mind)?
>
The use of "" and  \markup\null is nothing new, so it needs to be handled
correctly anyway.
It's possible to move a \markup\null to the left or right, therefore it is
useful for \earlyExtender.
If this property is not needed "" will be used as it is much easier to type. If
\! or something similar
would be provided for the same purpose almost everybody probably still would use
the faster "".

__ meant "add an extender event", this is meaningless with the proposed patch as
an extender
event is added after every syllable. To increase compatibility I feel that it
_never_ should be reused
for anything.



Cheers,
  Knut

[david.nalesnik, 2016-12-25 17:25:27]

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

https://codereview.appspot.com/313240043/diff/40001/lily/self-alignment-inter...
lily/self-alignment-interface.cc:61: return scm_from_double (-
ext.linear_combination (scm_to_double (align)));
Grobs are recognized by their interfaces not their names.  So here you would
write !has_interface<Lyric_syllable_interface> (me)

(I confess I'm unclear when internal_has_interface is used, though.)

https://codereview.appspot.com/313240043/diff/40001/lily/self-alignment-inter...
lily/self-alignment-interface.cc:143: x -= ext.linear_combination (scm_to_double
(self_align));
See above.

https://codereview.appspot.com/313240043/diff/40001/lily/self-alignment-inter...
lily/self-alignment-interface.cc:152: x += he.linear_combination (scm_to_double
(par_align));
See above.

https://codereview.appspot.com/313240043/diff/40001/scm/music-functions.scm
File scm/music-functions.scm (right):

https://codereview.appspot.com/313240043/diff/40001/scm/music-functions.scm#n...
scm/music-functions.scm:733: (append (ly:music-property event 'articulations)
Why not 'ExtenderEvent instead of (quote ExtenderEvent) ?

[Dan Eble, 2016-12-25 19:13:32]

On 2016/12/25 14:03:09, akobel wrote:
> Tell parser to ignore "__" token

I apologize if I am misjudging this change based on the headline alone, but if a
token serves no purpose (my assumption), then rather than ignoring it, shouldn’t
the parser treat it as it would any other unexpected input (i.e. complain)?

[akobel, 2016-12-25 21:53:55]

On 2016/12/25 15:43:05, Knut_Petersen_t-online.de wrote:
> Hi everybody!
> 
> > I'm only 90% happy about "" and \markup\null...
> > For dynamic spanners and hairpins, we have \! to end them prematurely
> > before a "natural" end event appears.  Is this similar enough to warrant
> > the use of the same token (i.e., \! would be translated to empty lyrics
> > with a CompletizeExtenderEvent), and is it possible at all? Otherwise,
> > should we have a special token for this (\. comes to my mind)?
> >
> The use of "" and  \markup\null is nothing new, so it needs to be handled
correctly anyway.
> It's possible to move a \markup\null to the left or right, therefore it is
useful for \earlyExtender.
> If this property is not needed "" will be used as it is much easier to type.
If \! or something similar
> would be provided for the same purpose almost everybody probably still would
use the faster "".

Granted. Didn't think of that, probably because I can't remember having ever
used either.

> __ meant "add an extender event", this is meaningless with the proposed patch
as
> an extender event is added after every syllable. To increase compatibility I
feel that it
> _never_ should be reused for anything.

Okay. I was considering that in case we could make \autoExtenderOff to revert to
the previous behavior.
But on second though, there is hardly any use for that, and it creates more
confusion than it's worth.


Bottom line: I withdraw both proposals.

[pkx166h, 2016-12-26 19:14:00]

On 2016/12/25 21:53:55, akobel wrote:
> On 2016/12/25 15:43:05, Knut_Petersen_t-online.de wrote:
> > Hi everybody!
> > 
> > > I'm only 90% happy about "" and \markup\null...
> > > For dynamic spanners and hairpins, we have \! to end them prematurely
> > > before a "natural" end event appears.  Is this similar enough to warrant
> > > the use of the same token (i.e., \! would be translated to empty lyrics
> > > with a CompletizeExtenderEvent), and is it possible at all? Otherwise,
> > > should we have a special token for this (\. comes to my mind)?
> > >
> > The use of "" and  \markup\null is nothing new, so it needs to be handled
> correctly anyway.
> > It's possible to move a \markup\null to the left or right, therefore it is
> useful for \earlyExtender.
> > If this property is not needed "" will be used as it is much easier to type.
> If \! or something similar
> > would be provided for the same purpose almost everybody probably still would
> use the faster "".
> 
> Granted. Didn't think of that, probably because I can't remember having ever
> used either.
> 
> > __ meant "add an extender event", this is meaningless with the proposed
patch
> as
> > an extender event is added after every syllable. To increase compatibility I
> feel that it
> > _never_ should be reused for anything.
> 
> Okay. I was considering that in case we could make \autoExtenderOff to revert
to
> the previous behavior.
> But on second though, there is hardly any use for that, and it creates more
> confusion than it's worth.
> 
> 
> Bottom line: I withdraw both proposals.

Can you then re-submit a new patch or delete the one(s) that are invalid?

I don't know what has and what has not been tested for sure, as far as I can
tell only patch set 1 has been tested which had a slew of warning messages
during the reg test output that I don't know are expected, desirable or
something else.

Thanks

James

[akobel, 2016-12-27 02:01:18]

On 2016/12/26 19:14:00, pkx166h wrote:
> On 2016/12/25 21:53:55, akobel wrote:
> > Bottom line: I withdraw both proposals.
> 
> Can you then re-submit a new patch or delete the one(s) that are invalid?

Sorry, I'm a bit drawn up between my position as the Rietveld-proxy of Knut and
my own Alexander-role as a commenter.
I forwarded all patches by Knut, and I withdrew my two comments from #5 and the
one from the mailing list that Knut answered in #6.

The patch set is up to date.

> I don't know what has and what has not been tested for sure, as far as I can
> tell only patch set 1 has been tested which had a slew of warning messages
> during the reg test output that I don't know are expected, desirable or
> something else.

W.r.t. the regtest differences, as far as I (Alexander) can tell:

The warnings should be gone with Knut's parser modification in patch set 3.

The image differences are expected. A few short extenders are removed (due to
the different handling of collapse-length vs. the old minimum-length). The exact
setting of default collapse-length could be discussed, but IMHO the setting is
fine.
A few extenders appear that have not been written explicitly before, but that is
expected, too.
I guess for page-spacing-nonstaff-lines-independent.ly, the extender should be
forbidden, to have the intended effect of this regtest shown more pronounced.

However, I propose that we first discuss the patch itself, and settle on syntax
and choice of default parameters (collapse-length, force-length). AFAICS, once
this is done, we will need a pass over the entire documentation and regtests
anyway that removes all __ in lyrics - not for functionality, but to purge
outdated and possibly confusing syntax. That's basically a `sed -e 's/ __//g'`,
but we will have to do a quick review of at least the affected docs to check for
errors.

Also, a convert-ly rule still has to be written that will, e.g., also solve the
difference in the page-spacing-nonstaff-lines-independent.ly regtest.


Cheers,
Alexander

[Knut_Petersen_t-online.de, 2016-12-27 20:55:21]

Am 27.12.2016 um 03:01 schrieb perpeduumimmobile@gmail.com:
> On 2016/12/26 19:14:00, pkx166h wrote:
>> On 2016/12/25 21:53:55, akobel wrote:
>> > Bottom line: I withdraw both proposals.
>
>> Can you then re-submit a new patch or delete the one(s) that are
> invalid?
>
> Sorry, I'm a bit drawn up between my position as the Rietveld-proxy of
> Knut and my own Alexander-role as a commenter.
> I forwarded all patches by Knut, and I withdrew my two comments from #5
> and the one from the mailing list that Knut answered in #6. 

I'll prepare a new patchset in a few days, thanks for the comments that were
posted up to now.

Cheers,
  Knut

[Knut_Petersen_t-online.de, 2017-01-01 15:39:39]

Hi everybody!

Attached find a new version of the autoextender patch set.

@Alexander:

I cannot think of anything easier to type than "", so it's still there ;-)

@David:

has_interface<Lyric_syllable_interface> (me) does not work, internal_... is
needed in lily/self-alignment-interface.cc.
music-functions.scm changed according to you comment.

@Trevor:

vocal.itely changed according to your comments.

Cheers,
  Knut

[akobel, 2017-01-02 09:45:45]

Version 2017-01-01

[akobel, 2017-01-02 10:10:15]

Version 2017-01-01

[a-kobel_a-kobel.de, 2017-01-02 10:13:39]

A happy new one, everybody!

On 2017-01-01 16:39, Knut Petersen wrote:
> Hi everybody!
>
> Attached find a new version of the autoextender patch set.

Thanks Knut. Just uploaded to Rietveld.

(In fact, sorry for double-posting it; I deleted my first upload a 
minute ago after I recognized that I based the patch on a more recent 
master than I should have, with unrelated changes in some files.)

> @Alexander:
>
> I cannot think of anything easier to type than "", so it's still there ;-)

No worries, I'm fine with that.


LGTM!


Cheers,
Alexander

[akobel, 2017-01-15 22:28:25]

Automatic lyric extenders - patchset 2017-01-15

[akobel, 2017-01-15 22:52:44]

remove __ from regtestsx

[akobel, 2017-01-17 17:55:52]

hide deprecated token for ExtenderEvents in \displayLilyMusic

[akobel, 2017-01-17 23:34:40]

purge superfluous __ from Lilypond code

[dak, 2017-01-18 08:28:16]

https://codereview.appspot.com/313240043/diff/160001/lily/parser.yy
File lily/parser.yy (right):

https://codereview.appspot.com/313240043/diff/160001/lily/parser.yy#newcode3107
lily/parser.yy:3107: $$ = $1;
That looks like strange fallback behavior.  EXTENDER likely has a value of
SCM_UNSPECIFIED (at least I think that is the default for tokens without
attached value).  Can the containing expression deal with that?

If it can, it would make more sense to explicitly use SCM_UNSPECIFIED, or
otherwise whatever other value is accepted by those using the containing
syntactical expression.

[Knut_Petersen_t-online.de, 2017-01-18 16:06:39]

Am 18.01.2017 um 09:28 schrieb dak@gnu.org:
>
> https://codereview.appspot.com/313240043/diff/160001/lily/parser.yy
> File lily/parser.yy (right):
>
>
https://codereview.appspot.com/313240043/diff/160001/lily/parser.yy#newcode3107
> lily/parser.yy:3107: $$ = $1;
> That looks like strange fallback behavior.  EXTENDER likely has a value
> of SCM_UNSPECIFIED (at least I think that is the default for tokens
> without attached value).  Can the containing expression deal with that?

I actually had SCM_UNSPECIFIED without the warning in patchset 4 ... don't know
why I changed it. Surprisingly both work identically. But yes, it should be
SCM_UNSPECIFIED.

Knut

[akobel, 2017-01-23 21:21:22]

let __ return SCM_UNSPECIFIED; minor change in deprecation warning

[dak, 2017-01-30 15:11:22]

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

https://codereview.appspot.com/313240043/diff/40001/lily/self-alignment-inter...
lily/self-alignment-interface.cc:61: return scm_from_double (-
ext.linear_combination (scm_to_double (align)));
On 2016/12/25 17:25:26, david.nalesnik wrote:
> Grobs are recognized by their interfaces not their names.  So here you would
> write !has_interface<Lyric_syllable_interface> (me)
> 
> (I confess I'm unclear when internal_has_interface is used, though.)

When has_interface does not work because the type cannot be determined at
runtime?

But this case appears icky.  Why would we have a non-empty glyph with empty
extents here in the first place?  Shouldn't it be removed earlier?

[dak, 2017-01-31 09:36:44]

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

https://codereview.appspot.com/313240043/diff/180001/lily/lyric-extender.cc#n...
lily/lyric-extender.cc:62: while (hs && robust_scm2double
(heads[hs-1]->get_property
This appears intended to get rid of grace notes, but wouldn't that also affect
ossia staves as a whole and alternative syllables for some stanzas?  Wouldn't it
be more reliable to base this on timing (grace time?) rather than notehead size?
 Or am I completely off the wall here?

[akobel, 2017-02-06 11:02:56]

Version 2017-02-04 by David

[dak, 2017-02-06 18:38:37]

On 2017/02/06 11:02:56, akobel wrote:
> Version 2017-02-04 by David

Not "by David" but "by Kurt".  David merely rebased Kurt's new version.

[dak, 2017-02-06 18:38:57]

On 2017/02/06 18:38:37, dak wrote:
> On 2017/02/06 11:02:56, akobel wrote:
> > Version 2017-02-04 by David
> 
> Not "by David" but "by Kurt".  David merely rebased Kurt's new version.

"by Knut" of course.

[dak, 2017-02-06 19:08:18]

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.c...
lily/extender-engraver.cc:33: #include "iostream"
Why?

https://codereview.appspot.com/313240043/diff/200001/lily/extender-engraver.c...
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.c...
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.c...
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#n...
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#n...
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-inte...
File lily/self-alignment-interface.cc (right):

https://codereview.appspot.com/313240043/diff/200001/lily/self-alignment-inte...
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-inte...
lily/self-alignment-interface.cc:146: ly_symbol2scm
("lyric-syllable-interface"))))
And this?

https://codereview.appspot.com/313240043/diff/200001/lily/self-alignment-inte...
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-properti...
File scm/define-grob-properties.scm (right):

https://codereview.appspot.com/313240043/diff/200001/scm/define-grob-properti...
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-properti...
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.

[dak, 2017-02-16 13:01:37]

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#n...
lily/lyric-extender.cc:63: while (hs && robust_scm2double
(heads[hs-1]->get_property
On 2017/02/06 19:08:18, dak wrote:
> Same as previously I see.

Which begs the question: what are the exact rules governing grace notes? 
Wouldn't it be sufficient to not even add any grace note to the "heads" array? 
Because at the time we add stuff there, the grace time of the time step is still
perfectly available.  Or are there situations where a grace note should be
present in "heads"?