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

Add wait_for_dismiss to push_screen #3477

Merged
merged 11 commits into from
Oct 9, 2023
Merged

Add wait_for_dismiss to push_screen #3477

merged 11 commits into from
Oct 9, 2023

Conversation

willmcgugan
Copy link
Collaborator

@willmcgugan willmcgugan commented Oct 8, 2023

An addition to the screens API to avoid callbacks. Adds a wait_for_dismiss parameter which waits for the new screen to be dismissed.

It works like this:

result = await self.push_screen(MyScreen(), wait_for_dismiss=True)

This only works from within a worker. If not run within a worker it will raise a NoActiveWorker error.

Without a worker, waiting on the result would cause a deadlock. But at least we can identify that and raise a helpful error.

This allows for more natural code than callbacks. For example, here's how you might use a screen to ask the user if they want to quit:

@work
async def action_close(self) -> None:
    if await self.push_screen(QuitScreen(), wait_for_dismiss=True):
        self.app.quit()

The push_screen has overloaded signatures, so the type checker knows that there will only be a return value when wait_for_dismiss is True, and it knows the type of the return -- which is nice!

@willmcgugan willmcgugan marked this pull request as draft October 8, 2023 16:58
@willmcgugan willmcgugan changed the title docstrings Add wait_for_dismiss to push_screen Oct 8, 2023
@willmcgugan willmcgugan marked this pull request as ready for review October 8, 2023 18:53
src/textual/app.py Show resolved Hide resolved
tests/test_screens.py Outdated Show resolved Hide resolved
src/textual/app.py Show resolved Hide resolved
Co-authored-by: Rodrigo Girão Serrão <[email protected]>
@mistotebe
Copy link

Isn't it better (definitely cleaner) to just add a new method, so you don't have to add wait_for_dismiss? Also no need to keep callback in that case...

@willmcgugan
Copy link
Collaborator Author

@mistotebe Nothing prevents us for doing both. But awaiting the screen is more verbose.

screen = QuitScreen()
await self.push_screen(screen)
result = await screen.wait_for_dismiss()

Compared to:

result = await self.push_screen(QuitScreen(), wait_for_dismiss=True)

And callbacks still have their uses.

@willmcgugan willmcgugan merged commit b8ac737 into main Oct 9, 2023
23 checks passed
@willmcgugan willmcgugan deleted the wait-for-dismiss branch October 9, 2023 11:42
@mistotebe
Copy link

@mistotebe Nothing prevents us for doing both. But awaiting the screen is more verbose.

screen = QuitScreen()
await self.push_screen(screen)
result = await screen.wait_for_dismiss()

If such a method existed, yes, but it appears to be missing from the PR? And sounds race-prone unless implemented correctly.

My instinct would have been to add something like this instead:

async def push_modal_screen+bikeshed(self, screen: ModalScreen[ScreenResultType]) -> ScreenResultType:
    ...

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.

4 participants