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

pop_until_active causes ScreenResume to be posted even though "transitional" screens won't be visible #5086

Closed
mzebrak opened this issue Oct 3, 2024 · 7 comments

Comments

@mzebrak
Copy link

mzebrak commented Oct 3, 2024

Please consider the following test. ScreenResume should be "logged" just 2 times, but is 4 times, because ScreenResume happens when "dismissing" multiple screens via "pop_until_active".

from __future__ import annotations

from typing import TYPE_CHECKING

from textual import on
from textual.app import App, ComposeResult
from textual.events import ScreenResume
from textual.reactive import var
from textual.screen import Screen
from textual.widgets import Label

if TYPE_CHECKING:
    from textual.pilot import Pilot


async def test_screen_resume() -> None:
    class BaseScreen(Screen):
        def compose(self) -> ComposeResult:
            yield Label("BASE")

    class FooScreen(Screen):
        def compose(self) -> ComposeResult:
            yield Label()

        @on(ScreenResume)
        def log_resume(self) -> None:
            self.app.resumed.append(True)

    class BarScreen(Screen):
        BINDINGS = [("b", "app.make_base_active")]

        def compose(self) -> ComposeResult:
            yield Label()

    class PopApp(App):
        SCREENS = {"base": BaseScreen}
        
        resumed = var([])

        async def on_mount(self) -> None:
            # Push base
            await self.push_screen("base")
            # Push two screens
            await self.push_screen(FooScreen())
            await self.push_screen(FooScreen())
            await self.push_screen(BarScreen())

        def action_make_base_active(self) -> None:
            self.get_screen("base").pop_until_active()

    async with PopApp().run_test() as pilot:
        pilot: Pilot
        await pilot.press("b")
        assert len(pilot.app.resumed) == 2
Copy link

github-actions bot commented Oct 3, 2024

We found the following entries in the FAQ which you may find helpful:

Feel free to close this issue if you found an answer in the FAQ. Otherwise, please give us a little time to review.

This is an automated reply, generated by FAQtory

@willmcgugan
Copy link
Collaborator

The is the correct behavior.

Perhaps you can say why you think it should be otherwise. And please make your MREs runnable.

@mzebrak
Copy link
Author

mzebrak commented Oct 3, 2024

Why would notifying the screen that it's active when it's not active for even a fraction of a second be the correct behavior?

If there is a good reason for this, please provide it. In my opinion this is incorrect behavior because this intermediate screen can define some actions for this event and why should they occur when transitioning between screens during that multiple dismiss. Since it will be destroyed in a moment, why should it react?

What is wrong with this MRE? What problem you faced while running it? It runs on my machine and it's a regular pytest test with a failing assertion showing the issue described there. It prints out expected and actual value.

@willmcgugan
Copy link
Collaborator

The intermediate screens will briefly be the active screen. Screens can make the assumption that it will receive resume and suspend.

An MRE should run with just python.

@mzebrak We want to help you, but your issues are getting close to time-wasting. I'd suggest you first ask questions on Discord before assuming something is a bug. Additionally, if you explain why you want something or expect it to work a certain way, you will get a satisfactory response quicker.

@mzebrak
Copy link
Author

mzebrak commented Oct 3, 2024

An MRE should run with just python.

I don't believe that pytest is a problem here when Textual supports pytest and bases its tests on it as well.
In this case, instead of writing the steps for reproduction, it's probably easier and faster to understand it on the assertion.

I'm sorry you have to say that when I'm trying my best to explain to you what the problem is. I feel like I'm treated in advance here because a good example of that is also closing the #5064 without a fix or a word despite the "silent crash" that I found.

If your opinion is correct behavior, please close this issue. My opinion is that informing the screen which will be removed in a moment and taking some action on it is an unnecessary performance waste. I think, I have the right to express it, especially since we operate in an open-source environment. I'm not posting here to harm Textual...

@willmcgugan
Copy link
Collaborator

You haven't explained what problem this is causing for you. And you haven't said why you think it should behave this way. We simply have nothing to work with.

Most of your other issues are like this. I mean you never once asked how to dismiss a number of screens to get back to a desired screen.

If Textual is important to your work, and I assume it is, you will need to start cooperating with is. Have a look at the 100s of closed issues that go smoothly for inspiration.

Copy link

github-actions bot commented Oct 4, 2024

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

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

No branches or pull requests

2 participants