Issue 5771: simplify \partial (issue 557440043 by nine.fierce.ballads@gmail.com)

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Issue 5771: simplify \partial (issue 557440043 by nine.fierce.ballads@gmail.com)

nine.fierce.ballads
Reviewers: ,


https://codereview.appspot.com/557440043/diff/547670043/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

https://codereview.appspot.com/557440043/diff/547670043/ly/music-functions-init.ly#newcode1319
ly/music-functions-init.ly:1319: 'origin (*location*)
I'm curious about when it is important to provide 'origin (*location*)
to make-music and when it is not.  Some functions do it and some don't.

Description:
https://sourceforge.net/p/testlilyissues/issues/5771/

Remove an unnecessary (descend-to-context ... 'Score).

I started looking for a reason for this code, but I lost interest once I
saw how old it is.  input/regression/partial-polymetric.ly seems to
cover the relevant case, and it still passes after removing this code.


Please review this at https://codereview.appspot.com/557440043/

Affected files (+7, -14 lines):
  M ly/music-functions-init.ly
  M scm/define-music-display-methods.scm


Index: ly/music-functions-init.ly
diff --git a/ly/music-functions-init.ly b/ly/music-functions-init.ly
index f3893fab2ffb4062d60f496e63b978195b858c7c..42c1075f13c657bf38dbbc196b3fe6dfafd83cb3 100644
--- a/ly/music-functions-init.ly
+++ b/ly/music-functions-init.ly
@@ -1315,15 +1315,10 @@ that they share a staff with stems directed downward.")
 partial =
 #(define-music-function (dur) (ly:duration?)
   (_i "Make a partial measure.")
-
-  ;; We use `descend-to-context' here instead of `context-spec-music' to
-  ;; ensure \partial still works if the Timing_translator is moved
-    (descend-to-context
-     (context-spec-music (make-music 'PartialSet
-                                     'origin (*location*)
-                                     'duration dur)
-                         'Timing)
-     'Score))
+  (context-spec-music (make-music 'PartialSet
+                                  'origin (*location*)
+                                  'duration dur)
+                      'Timing))
 
 pitchedTrill =
 #(define-music-function
Index: scm/define-music-display-methods.scm
diff --git a/scm/define-music-display-methods.scm b/scm/define-music-display-methods.scm
index 3516286c17def21193ce9074a17433e2b2171bb1..5cead5e0a1b9dfddad73d39356c47d49562391b7 100644
--- a/scm/define-music-display-methods.scm
+++ b/scm/define-music-display-methods.scm
@@ -988,12 +988,10 @@ Otherwise, return #f."
 Otherwise, return #f."
   (with-music-match (expr (music
                            'ContextSpeccedMusic
+                           context-type 'Timing
                            element (music
-                                    'ContextSpeccedMusic
-                                    context-type 'Timing
-                                    element (music
-                                             'PartialSet
-                                             duration ?duration))))
+                                    'PartialSet
+                                    duration ?duration)))
 
                     (and ?duration
                          (format #f "\\partial ~a"



Reply | Threaded
Open this post in threaded view
|

Re: Issue 5771: simplify \partial (issue 557440043 by nine.fierce.ballads@gmail.com)

David Kastrup

https://codereview.appspot.com/557440043/diff/547670043/ly/music-functions-init.ly
File ly/music-functions-init.ly (right):

https://codereview.appspot.com/557440043/diff/547670043/ly/music-functions-init.ly#newcode1319
ly/music-functions-init.ly:1319: 'origin (*location*)
On 2020/02/17 16:59:54, Dan Eble wrote:
> I'm curious about when it is important to provide 'origin (*location*)
to
> make-music and when it is not.  Some functions do it and some don't.

define-music-function tacks the origin on the top-level music expression
it returns.  In this case here, that does not help.

Are you sure this is actually a working idea?  At the beginning of
music, Score does not exist and 'Timing is only (reliably?) established
as an alias by the Timing_translator.  For polyrhythmic pieces, the
Timing context alias is moved down the hierarchy along with the
Timing_translator.

https://codereview.appspot.com/557440043/

Reply | Threaded
Open this post in threaded view
|

Re: Issue 5771: simplify \partial (issue 557440043 by nine.fierce.ballads@gmail.com)

nine.fierce.ballads
In reply to this post by nine.fierce.ballads
On 2020/02/17 18:41:58, dak wrote:
> Are you sure this is actually a working idea?  At the beginning of
music, Score
> does not exist and 'Timing is only (reliably?) established as an alias
by the
> Timing_translator.  For polyrhythmic pieces, the Timing context alias
is moved
> down the hierarchy along with the Timing_translator.

What I'm sure of is that the stated scenario for using
descend-to-context here was "the Timing_translator is moved," that
input/regression/partial-polymetric.ly moves the Timing_translator, and
this patch did not make any change in the regression tests.  I won't
claim that there is full coverage because I didn't investigate that
thoroughly.

About the alias: I see this in the Score definition in
ly/engraver-init.ly:

  \alias "Timing"

  %% An alias for Timing is established by the Timing_translator in
  %% whatever context it is initialized, and the timing variables are
  %% then copied from wherever Timing had been previously established.
  %% The alias at Score level provides a target for initializing
  %% Timing variables in layout definitions before any
  %% Timing_translator has been run.

The only state in which (descend-to-context ... 'Score) should have an
effect is when the current context is Global.  In any other state, it
should change nothing.  And if the current context is Global,
(context-spec-music ... 'Timing) should find-or-create the Score because
of the alias; descending to Score first should not be necessary.

https://codereview.appspot.com/557440043/

Reply | Threaded
Open this post in threaded view
|

Re: Issue 5771: simplify \partial (issue 557440043 by nine.fierce.ballads@gmail.com)

David Kastrup
In reply to this post by nine.fierce.ballads
On 2020/02/17 19:17:04, Dan Eble wrote:
> On 2020/02/17 18:41:58, dak wrote:
> > Are you sure this is actually a working idea?  At the beginning of
music,
> Score
> > does not exist and 'Timing is only (reliably?) established as an
alias by the
> > Timing_translator.  For polyrhythmic pieces, the Timing context
alias is moved
> > down the hierarchy along with the Timing_translator.
>
> What I'm sure of is that the stated scenario for using
descend-to-context here
> was "the Timing_translator is moved," that
> input/regression/partial-polymetric.ly moves the Timing_translator,
and this
> patch did not make any change in the regression tests.  I won't claim
that there
> is full coverage because I didn't investigate that thoroughly.
>
> About the alias: I see this in the Score definition in
ly/engraver-init.ly:

>
>   \alias "Timing"
>
>   %% An alias for Timing is established by the Timing_translator in
>   %% whatever context it is initialized, and the timing variables are
>   %% then copied from wherever Timing had been previously established.
>   %% The alias at Score level provides a target for initializing
>   %% Timing variables in layout definitions before any
>   %% Timing_translator has been run.
>
> The only state in which (descend-to-context ... 'Score) should have an
effect is
> when the current context is Global.  In any other state, it should
change
> nothing.  And if the current context is Global, (context-spec-music
... 'Timing)
> should find-or-create the Score because of the alias; descending to
Score first
> should not be necessary.

Well, with a bit of juggling around things I have not been able to
trigger a difference.  I am a bit skeptical that that means nobody else
can: we have inventive users and the startup of Timing has been one area
where combinations of things like \skip, \partial and \time have managed
to create crashes.  Since we have various other context-related changes
slated in 2.21.0, this is one of the cases where I'd likely be happier
if the final push had this change end up in 2.21.1.

https://codereview.appspot.com/557440043/