Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: scalar animation with percentages #3004

Conversation

TomJGooding
Copy link
Contributor

@TomJGooding TomJGooding commented Jul 24, 2023

Description

Fixes #2940 by resolving the scalar animation start and destination cell sizes based on the size of the container (parent) rather than the widget.

Related Issue

How Has This Been Tested?

The added test sets both the initial widget width and the animation value as percentages. The size is checked at specific intervals during the animation using pilot.pause, which might be why the test is a bit flaky?

This has also been tested manually, with both the animate method and the (currently undocumented) CSS transition feature.

Checklist:

  • Docstrings on all new or modified functions / classes
  • Updated documentation
  • Updated CHANGELOG.md (where appropriate)

@TomJGooding
Copy link
Contributor Author

Should a check be added here? mypy complains that "DOMNode | None" has no attribute "size", but am I right in thinking that all widgets have a parent which will be either another widget or the app?

container_size = widget.parent.size

@willmcgugan
Copy link
Collaborator

The app is a DOMnode, which doesn’t have a parent because it’s at the very top. Not sure if that’s applicable here. Haven’t checked the code yet…

@TomJGooding
Copy link
Contributor Author

TomJGooding commented Jul 25, 2023

Thanks Will, that's what I thought. I guess someone might try animating the size of the app (though pretty sure that would crash currently anyway)...!

@willmcgugan
Copy link
Collaborator

@TomJGooding Is this one still relevant?

@TomJGooding
Copy link
Contributor Author

Yes, it looks like there is still an issue with scalar animation. Here's a slightly modified version of the app from the added test. Press Space to start the animation, and you'll the see the width rubber bands as described in the linked issue #2940.

from textual.app import App, ComposeResult
from textual.containers import Container
from textual.widgets import Footer, Static


class ScalarPercentAnimApp(App):
    BINDINGS = [("space", "start_animation", "start animation")]

    # To simplify percentage calculations, the widget to be animated is placed
    # inside a container with a width of 100
    CSS = """
    #container {
        width: 100;
    }

    #foo {
        width: 20%;
        height: 3;
        background: red;
    }
    """

    def compose(self) -> ComposeResult:
        with Container(id="container"):
            yield Static(id="foo")

    def action_start_animation(self) -> None:
        static = self.query_one("#foo", Static)
        # The animation duration is set to 6 seconds, so after every
        # second the width should have increased by 10 cells
        static.styles.animate("width", "80%", duration=6.0, easing="linear")


if __name__ == "__main__":
    app = ScalarPercentAnimApp()
    app.run()

I think the problem is how the start and destination are 'resolved' here, as shouldn't this be based on the size of the container (parent) rather than the widget?

size = widget.outer_size
viewport = widget.app.size
self.start = getattr(styles, attribute).resolve(size, viewport)
self.destination = value.resolve(size, viewport)

@TomJGooding
Copy link
Contributor Author

Sorry I realise this PR has been hanging around for a while, but after some tinkering and hopefully now with a better understanding of Textual, I'm tentatively marking this as ready for review.

You'll see I've struggled with a creating a test that isn't flaky in CI (only in Windows for some reason), so I'd welcome any suggestions for improving this.

@TomJGooding TomJGooding marked this pull request as ready for review January 30, 2024 18:22
@TomJGooding
Copy link
Contributor Author

Closed as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSS Transition rubber bands when %'s are used
2 participants