textual icon indicating copy to clipboard operation
textual copied to clipboard

fix: scalar animation with percentages

Open TomJGooding opened this issue 2 years ago • 6 comments

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

  • #2940

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 avatar Jul 24 '23 19:07 TomJGooding

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?

https://github.com/Textualize/textual/blob/33d775ab823f4c01d819a29e7581dfe8453c7e9a/src/textual/css/scalar_animation.py#L38

TomJGooding avatar Jul 25 '23 16:07 TomJGooding

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…

willmcgugan avatar Jul 25 '23 17:07 willmcgugan

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)...!

TomJGooding avatar Jul 25 '23 18:07 TomJGooding

@TomJGooding Is this one still relevant?

willmcgugan avatar Oct 09 '23 11:10 willmcgugan

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?

https://github.com/Textualize/textual/blob/0ffa82a13ec28c242ea13e2424863e09d819ed82/src/textual/css/scalar_animation.py#L38-L42

TomJGooding avatar Oct 09 '23 12:10 TomJGooding

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 avatar Jan 30 '24 18:01 TomJGooding

Closed as stale.

TomJGooding avatar Mar 12 '24 19:03 TomJGooding