Add support for env variable TEXTUAL_ANIMATIONS #4062
Conversation
Go over 'all' (as far as I can tell) animations in Textual. Most of them should only run when the env variable TEXTUAL_ANIMATIONS is set to FULL. A few animations may run on the level BASIC, which are animations that don't delay content appearing: - indeterminate progress bars - loading indicators - button presses - tab underlines - switch toggles - all (?) types of scrolling. These animations are completely disabled when the env var is NONE. The indeterminate progress bar displays a full, static bar and the loading indicator displays a string 'Loading...'. Many animation-related methods also grew a keyword parameter 'animate_on_level' that establishes the minimum level for said animation to take place.
rodrigogiraoserrao
changed the title
By using 'constants.SHOW_ANIMATIONS' instead of importing the constant directly we make it easier to patch for testing. See: https://mathspp.com/blog/til/patching-module-globals-with-pytest
The original issue (#3992) asked for a property on 'App' that allows controlling whether that app's animations should be played or not.
Check that animations that should happen on the BASIC level do happen at that level and don't happen on the NONE level.
There was a problem hiding this comment.
When you animate something, you only really need to tell the animation system if it is a basic animation that doesn't slow down the UI, or a more gratuitous animation that is just because it looks cool.
This makes having a bool with 3 values seem superfluous.
I would think that when calling animate, you would specify "basic" or "full".
Something like.
self.animate("x", 50, level="basic")Where the default would be "full".
Note that I don't think we should use Enums in the public interface (see comments in the code).
|
|
||
| def __call__(self, time: float) -> bool: | ||
| if self.duration == 0: | ||
| if ( |
There was a problem hiding this comment.
This is where we need the app.show_animations. Could we pass it to __call__ ?
Alternatively, do we even need to do this logic here? The animator calls this method. Could we not perform this logic from the animator? The animator has a reference to the app.
There was a problem hiding this comment.
We could perform the logic there but I think it's cleaner here.
If we do it on the animator, then we also need to set the attribute to its value in the animator and I think it'll look clunky there.
There was a problem hiding this comment.
You may need to elaborate on "cleaner" and "clunky" which are quite subjective terms.
It does feel like it is the animators job to decide if to animate something. Perhaps an Animation object should grow a finalize method which sets the attribute to its final stage.
There was a problem hiding this comment.
While addressing your other remarks I think this stopped being a problem.
| factor = min(1.0, (time - self.start_time) / self.duration) | ||
| eased_factor = self.easing(factor) | ||
|
|
||
| if eased_factor >= 1: | ||
| if ( |
There was a problem hiding this comment.
I guess this sets the value to its final position, but what is to prevent the animator calling it multiple times? Anything?
There was a problem hiding this comment.
Yeah. The return True tells the animator the animation was completed, which means that the animation will have its "on complete callback" called (if there's any) and then the animator gets rid of the animation:
textual/src/textual/_animator.py
Lines 528 to 532 in 16656e6
See review comment: #4062 (comment).
@willmcgugan addressed 👌 |
Adds support for the environment variable
TEXTUAL_ANIMATIONS, which dictates which animations take place in Textual apps.(Kind of like a log level but for animations.)
Fixes #3992.