Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(794)

Issue 557440043: Issue 5771: Remove unnecessary (descend-to-context ... 'Score)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 2 weeks ago by Dan Eble
Modified:
1 month, 2 weeks ago
Reviewers:
dak
CC:
lilypond-devel_gnu.org
Visibility:
Public.

Description

https://sourceforge.net/p/testlilyissues/issues/5771/ Remove an unnecessary (descend-to-context ... 'Score). From \partial: 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.

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -14 lines) Patch
M ly/music-functions-init.ly View 1 chunk +4 lines, -9 lines 2 comments Download
M scm/define-music-display-methods.scm View 1 chunk +3 lines, -5 lines 0 comments Download

Messages

Total messages: 4
Dan Eble
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 ...
1 month, 2 weeks ago (2020-02-17 16:59:54 UTC) #1
dak
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: > ...
1 month, 2 weeks ago (2020-02-17 18:41:58 UTC) #2
Dan Eble
On 2020/02/17 18:41:58, dak wrote: > Are you sure this is actually a working idea? ...
1 month, 2 weeks ago (2020-02-17 19:17:04 UTC) #3
dak
1 month, 2 weeks ago (2020-02-17 19:33:24 UTC) #4
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b