Re: Issue 3560: Completion_heads_engraver with \scaleDurations (issue 14268043)

[dak]

On 2013/10/26 06:37:13, Keith wrote:
> The simplicity is just lovely.

Always hard to tell sarcasm from enthusiasm.


https://codereview.appspot.com/14268043/diff/9001/lily/completion-note-heads-engraver.cc
> File lily/completion-note-heads-engraver.cc (right):


https://codereview.appspot.com/14268043/diff/9001/lily/completion-note-heads-engraver.cc#newcode205
> lily/completion-note-heads-engraver.cc:205: if (nb.main_part_ && nb <
> note_dur.get_length ())
> The logic is a bit convoluted (probably a leftover from an earlier
design).
> Now the test  factor.sign==1  could go into the if()

I'll see what to do there.

> https://codereview.appspot.com/14268043/diff/9001/ly/engraver-init.ly
> File ly/engraver-init.ly (right):


https://codereview.appspot.com/14268043/diff/9001/ly/engraver-init.ly#newcode637
> ly/engraver-init.ly:637: completionFactor = #ly:duration-scale
> We do need to keep issue 1532 fixed, since several people depend on
and expect
> that behavior.

> %% Historically, completion_heads_engraver has split integer-scaled
> %% notes into notes with no scaling   c1*3  ==>  {c1~ c1~ c1~}
>    completionFactor = #
>    (lambda (dur)
>      (let ((scale (ly:duration-scale dur)))
>           (if (integer? scale)
>                1
>                scale)))

> If you can make the test
>     (if (> (ly:duration-length dur) measureLength)
> then issue 1772 works as several people[*] expected.

> [*] people including Neil, who had recently reviewed and pushed Páls
patch to
> make completion_*_engraver split tuplet-notes into tuplet-notes.

I violently disagree here.  People "expect" LilyPond to do the right
thing (TM), but it is not possible for it to know what "the right
thing" would be.  So we need to have it configurable, and the next
best thing is to do something _consistent_.  Anything that interprets
split notes different from unsplit notes is inconsistent.

Now that does not change that having to crank out some anonymous
lambda function for some common behavior is a bad idea.  At least the
previous default behavior should be available as a stock function, and
one can conceivably teach convert-ly to continue providing the old
inconsistent behavior via an appropriate assignment whenever one of
the completion engravers is being used.  Please remember that this old
behavior is _also_ not what "people" expected, or we would not be
having this issue.


https://codereview.appspot.com/14268043/