Code review - Issue 553760043: Add dynamic-interface to keepAliveInterfaceshttps://codereview.appspot.com/2020-05-03T20:03:56+00:00rietveld
Message from unknown
2020-03-24T19:29:02+00:00jeanurn:md5:4ad5d9b5820b8539c430d0a5efa2f600
Message from unknown
2020-03-25T21:51:12+00:00jeanurn:md5:3a32ff22bbc65733df14e572bb108b8f
Message from jean@abou-samra.fr
2020-03-25T21:51:14+00:00jeanurn:md5:a050ee074715e5497cabf81f4365673e
Add dynamic-interface to keepAliveInterfaces
Message from unknown
2020-03-25T21:52:12+00:00jeanurn:md5:4b9ccd9bc4edca4b1d1141eb58ee153f
Message from jean@abou-samra.fr
2020-03-25T21:52:13+00:00jeanurn:md5:1d49c5a01c04b6c5be5aa7fca0346eee
Add dynamic-interface to keepAliveInterfaces
Message from unknown
2020-03-26T10:33:07+00:00jeanurn:md5:c75cf2d07a1a8f024e60cca8cfac48a1
Message from jean@abou-samra.fr
2020-03-26T10:33:10+00:00jeanurn:md5:d4473c5e90085d3c350cd34cb4cb9f92
Add dynamic-interface to keepAliveInterfaces
Message from unknown
2020-03-26T10:33:43+00:00jeanurn:md5:57f906473c79c78fc7fafd87af4de36b
Message from jean@abou-samra.fr
2020-03-26T10:33:44+00:00jeanurn:md5:871db08886d47ba65fee940a71f73481
Add dynamic-interface to keepAliveInterfaces
Message from unknown
2020-03-26T10:39:14+00:00jeanurn:md5:1e80bec5f7bd021974981c4bbc9b445f
Message from jean@abou-samra.fr
2020-03-26T10:39:15+00:00jeanurn:md5:c253d63e820ca81702c13860e41b2fb2
Add dynamic-interface to keepAliveInterfaces
Message from unknown
2020-03-26T11:26:10+00:00jeanurn:md5:6d53cfb0b89e956a2bcca0764b79e0a1
Message from jean@abou-samra.fr
2020-03-26T11:26:12+00:00jeanurn:md5:9e1f67fdf443b6cd12ee21d2c394ad38
Add dynamic-interface to keepAliveInterfaces
Message from unknown
2020-03-26T11:35:58+00:00jeanurn:md5:506277f03ab8a2f40140dc3fe9fea823
Message from jean@abou-samra.fr
2020-03-26T11:35:59+00:00jeanurn:md5:c4ace82b8823a49edcb1a483625208e5
Add dynamic-interface to keepAliveInterfaces
Message from dak@gnu.org
2020-03-26T12:01:40+00:00dakurn:md5:b7b03407058ab709eb2b465606b546cf
What I am worried about is a partitura that puts common dynamics on every staff, including staves without notes. That would keep those from being kept alive.
So the question is whether we should not possibly create a different keepAliveInterfaces for the Dynamics context?
Opinions?
Message from unknown
2020-03-26T12:24:32+00:00jeanurn:md5:4326e1cd07c7eef85f38a3d2ecc829e7
Message from jean@abou-samra.fr
2020-03-26T12:24:33+00:00jeanurn:md5:afbe63d82d0c290e2d4d3327521695ea
Add dynamic-interface to keepAliveInterfaces
Message from lemzwerg@googlemail.com
2020-03-26T12:50:11+00:00lemzwergurn:md5:bb932c21db7fff2a0507e209674b2d05
LGTM
https://codereview.appspot.com/553760043/diff/565830044/Documentation/notation/staff.itely
File Documentation/notation/staff.itely (right):
https://codereview.appspot.com/553760043/diff/565830044/Documentation/notation/staff.itely#newcode801
Documentation/notation/staff.itely:801: rests, skips, or a combination of these elements. This behavior can be
Please use two spaces after a full stop.
https://codereview.appspot.com/553760043/diff/565830044/Documentation/notation/staff.itely#newcode807
Documentation/notation/staff.itely:807: solely skips will then be removed.
Maybe s/solely skips/skips only/
Message from wl@gnu.org
2020-03-26T12:52:29+00:00wl_gnu.orgurn:md5:463f52f5dde8f6084f52befdbb53c592
> What I am worried about is a partitura that puts common dynamics on
> every staff, including staves without notes.
Never seen that, AFAIK. Can you provide a link to such a score?
Werner
Message from v.villenave@gmail.com
2020-03-26T16:31:17+00:00Valentin Villenaveurn:md5:eafb97278d933ed24828488dc5730831
On 2020/03/26 12:52:29, wl_gnu.org wrote:
> Never seen that, AFAIK. Can you provide a link to such a score?
I can certainly imagine that sort of things, in an orchestral score simple enough that all instruments share the same dynamics (as was common until the late 18th century), and where the LilyPonder may want to store all that in a shared variable, including for systems where for example all the wind instruments are tacet.
OTOH, we may also decide that users advanced enough to typeset a full partitura are knowledgeable enough to know how to set their own keepAliveInterfaces.
BTW (on an unrelated note), I’ve just noticed that there’s something weird happening with some stem columns when dynamics are attached to spacer rests:
https://sourceforge.net/p/testlilyissues/issues/4691/#cc82
V.
Message from wl@gnu.org
2020-03-26T16:41:41+00:00wl_gnu.orgurn:md5:a83bffc32bab0f4be2f49c229c0c7f2a
>> Never seen that, AFAIK. Can you provide a link to such a score?
>
> I can certainly imagine that sort of things, in an orchestral score
> simple enough that all instruments share the same dynamics (as was
> common until the late 18th century), and where the LilyPonder may
> want to store all that in a shared variable, including for systems
> where for example all the wind instruments are tacet.
Well, sharing is OK, but displaying? Are you really saying there
exist scores that have staff lines only with rests, and which also
have dynamics like 'f' or 'p'? Besides not making any sense, it would
completely confuse the player IMHO.
Werner
Message from unknown
2020-03-27T18:39:33+00:00jeanurn:md5:55c3882906f46edc57fe1ad846d3ff45
Message from jean@abou-samra.fr
2020-03-27T18:39:35+00:00jeanurn:md5:e02a84e9eadfd3bd3eb762caead1b34c
Adress review comments on the documentation
Message from jean@abou-samra.fr
2020-03-27T19:00:08+00:00jeanurn:md5:b6cf0eeba0cfee3499c447220a25f064
This is a relevant point, but I think Werner is right.
You can put your dynamics in a Dynamics context. Currently, they would be
removed completely, while with this patch, they would be kept entirely.
Whatever the value of keepAliveInterfaces, this is no good solution for the
kind of partitura that you describe (althought I tend to think that the latter
would be less cryptic to the user). But with this change, the default behavior will
be correct with a usual Dynamics, that is, if you just write spacer rests when
you want no dynamics.
What we're debating here is the case where you put the dynamics together with
the music in the same Staff context.
Without this change, staves with rests only are removed whatever their dynamics.
You could think this is useful, but what if the winds start tacet in the middle
of a system? You will then have rests with dynamics on them. With this patch, all the
staves will be kept as soon as they contain dynamics, and you will also have rests with
dynamics on them. I don't think there is much better to do. Whatever the state of
keepAliveInterfaces, there is no 'easy' solution: it's up to the user to split their
dynamics variable, or use \quoteDuring, or write a very tricky function. As a result,
I can see no situation where the current value of keepAliveInterfaces does a better
job than the one with dynamic-interface.
If someone does and can provide a concrete example, I'll gladly create a different
value for Dynamics contexts, but if there is no real reason to do this, let's keep it simple.
Message from v.villenave@gmail.com
2020-03-27T23:08:23+00:00Valentin Villenaveurn:md5:e7ed2536c2eb3828553947ad15b6f4ba
On 2020/03/27 19:00:08, jean wrote:
> I can see no situation where the current value of keepAliveInterfaces does a
> better
> job than the one with dynamic-interface.
OK, you both make a valid point. Then it LGTM as well.
V.
Message from nine.fierce.ballads@gmail.com
2020-03-27T23:16:40+00:00Dan Ebleurn:md5:1a407f088c42aa642f466c118e852d66
Is the impact (if any) on existing scores important? (cases that we might not have in regression tests but that might irk users?)
Message from nine.fierce.ballads@gmail.com
2020-03-27T23:29:56+00:00Dan Ebleurn:md5:8d30c298b4388f5c1e5a4d70c96b5f4c
On 2020/03/27 23:16:40, Dan Eble wrote:
> Is the impact (if any) on existing scores important? (cases that we might not
> have in regression tests but that might irk users?)
... and speaking of regression tests, if you don't want someone to break your work later, it would be a good idea to add one.
Message from jean@abou-samra.fr
2020-03-31T20:14:44+00:00jeanurn:md5:a0edbf803971b027221f6cd2343d12ec
On 2020/03/27 23:16:40, Dan Eble wrote:
> Is the impact (if any) on existing scores important? (cases that we might not
> have in regression tests but that might irk users?)
It should be close to zero.
In fact, I'm stupid. I just realised a thing: up to now, users have been told to put \RemoveEmptyStaves in a \context { \Staff ... }. So it won't change anything in the overwhelming majority of scores. But, it will allow you to do this in a \context { \Score ... } (also applying to TabStaff etc.) or, by the by, in a \context { \PianoStaff ... } (it will apply to the whole piano part, but no longer remove the Dynamics context between the staves).
I'll update the documentation with this.
By the way, why do we say \RemoveEmptyStaves instead of \removeEmptyStaves?
Message from jean@abou-samra.fr
2020-03-31T20:26:51+00:00jeanurn:md5:f7cce366f61506d7b1e4b28ea1b99444
On 2020/03/31 20:14:44, jean wrote:
> I'll update the documentation with this.
Hum, while exploring the regression tests, I just discovered that we have a Keep_alive_together_engraver out there, undocumented, except in the internals.
I'll take a look at it, and try to write appropriate documentation (or find the authors and ask them to document it!). Setting the issue to needs_work.
Message from v.villenave@gmail.com
2020-03-31T20:43:03+00:00Valentin Villenaveurn:md5:bfb95a3c23dc98e9bc6430ffcff6a84b
On 3/31/20, jean@abou-samra.fr <jean@abou-samra.fr> wrote:
> By the way, why do we say \RemoveEmptyStaves instead of
> \removeEmptyStaves?
Because it’s a context property that you need to set for your whole
context (it’s actually a \with block), not on-the-fly like \cadenzaOn;
see also \RemoveEmptyStaffContext, which is capitalized like all
context names (Staff, RhythmicStaff etc.). It may (may!) become
clearer to you if you have a look at ly/context-mods-init.ly and
ly/engraver-init.ly (unlike non-capitalized properties and commands
defined in ly/property-init.ly and ly/music-functions-init.ly).
… And, yes, I do realize that’s way too convoluted of an explanation.
If someone else can do it more straightforwardly, have at it! :-)
Cheers,
- V.
Message from v.villenave@gmail.com
2020-03-31T20:51:05+00:00Valentin Villenaveurn:md5:5e96d2361eb1bdbf39dc39c62d8a1d5f
On 3/31/20, jean@abou-samra.fr <jean@abou-samra.fr> wrote:
> I'll take a look at it, and try to write appropriate documentation (or
> find the authors and ask them to document it!). Setting the issue to
> needs_work.
I believe it was added by Joe Neeman in order to fix issue #442:
https://sourceforge.net/p/testlilyissues/issues/442/
You can’t say it’s undocumented, its documentation is just not meant
for regular users:
http://lilypond.org/doc/latest/Documentation/internals/keep_005falive_005ftogether_005fengraver
There are also several comments in the source code:
http://git.savannah.gnu.org/cgit/lilypond.git/tree/lily/keep-alive-together-engraver.cc#n72
Cheers,
- V.
Message from unknown
2020-05-03T19:19:40+00:00jeanurn:md5:1ac274a28268fc2b4cb8083cbdd29773
Message from jean@abou-samra.fr
2020-05-03T19:19:42+00:00jeanurn:md5:e7b62f37fff469863b5eca0fa06195cf
Documentation improvements
Message from unknown
2020-05-03T19:37:14+00:00jeanurn:md5:25cfd1d54efb92cf5b1b8a07d3d60b7c
Message from jean@abou-samra.fr
2020-05-03T19:37:17+00:00jeanurn:md5:9ce2af0952fccc9a195f4954e864fbe4
Add a regression test
Message from jean@abou-samra.fr
2020-05-03T20:03:56+00:00jeanurn:md5:2aff0ae25de2ed4bd71c07c05bec71fd
Thanks for you support, Valentin.
> … And, yes, I do realize that’s way too convoluted of an explanation.
If someone else can do it more straightforwardly, have at it! :-)
Hey, don't make me feel bad about my own more-than-lengthy explanations!
On 2020/03/31 20:51:05, Valentin Villenave wrote:
> You can’t say it’s undocumented, its documentation is just not meant
> for regular users:
True, I meant that this isn't just useful for internal use, you may need it as a regular user, I think, because a conductor would think the choir or the strings as a whole, so it can sound weird to have a system with only violin I without violin II (hope I'm clear).
There is now a regression test as asked for, and a bit more documentation. I added an example about Keep_alive_together_engraver, though I'm not completely satified with it--feel free to improve. It is taken from https://www.universaledition.com/kurt-weill-764/works/der-jasager-1900 (Kurt Weill, Der Jasager). This score isn't actually demonstrating the need for this (it could have been if the breaks were placed another way) but if this is wanted, here is an example showing that it happens in printed music:
http://imslp.eu/files/imglnks/euimg/5/54/IMSLP522307-PMLP3653-NBE_-_Symphonie_Fantastique_-_I._Reveries,_Passions_(etc).pdf
(the first page).
Yet, there is still an issue. What's happening in this example?
\version "2.21.0"
keepRests =
\applyContext
#(lambda (context)
(ly:context-set-property! context 'keepAliveInterfaces
(cons 'rest-interface (ly:context-property context 'keepAliveInterfaces))))
\layout {
\context {
\Score
\RemoveAllEmptyStaves
\keepRests
}
}
<<
{ c'1 c'1 c'1 }
{ R1 \break s1 \break R1 }
>>
I expect the second staff in the second system to disappear, but this is not the case. Yet if you replace the surrounding R1 with r1, then it works. Did I misunderstand the way \RemoveEmptyStaves works? How can it depend on the other systems? It can't be put in the documentation if it doesn't work...
Thanks!