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

Don't allow screen instances in App.SCREENS #4893

Closed
willmcgugan opened this issue Aug 18, 2024 · 5 comments · Fixed by #4894
Closed

Don't allow screen instances in App.SCREENS #4893

willmcgugan opened this issue Aug 18, 2024 · 5 comments · Fixed by #4894
Assignees
Labels
good first issue Good for newcomers

Comments

@willmcgugan
Copy link
Collaborator

Textual currently allows a screen instance to be specified in the SCREENS classvar. This works fine when launching an app, until a second instance of the app runs. Re-using a previous instance breaks the app.

See the example here: https://textual.textualize.io/guide/screens/#creating-a-screen

Note how the BSOD is given as an instance in a classvar, i.e. SCREENS = {"bsod": BSOD()} This would cause the instance to be shared if there is ever more than a single App.

The easiest workaround is to disallow passing instances, and require a string or callable. So the above example should be SCREENS = {"bsod": BSOD}

Requirements:

  • Adjust typing to not allow a screen instance in SCREENS and MODES
  • Reject screen instances at runtime, by throwing a TypeError.
  • Update docs and examples accordingly.
  • Tests to ensure behavior when not using a string or callable.
@willmcgugan willmcgugan added the good first issue Good for newcomers label Aug 18, 2024
@ZeroIntensity
Copy link
Contributor

I can give this a go!

@willmcgugan
Copy link
Collaborator Author

Good catch re modifying the MODES classvar. We should not be doing that. Suggest we copy the classvar to an instance var.

If that looks straightforward. You could do it in this PR. Otherwise, it could be a new issue.

@ZeroIntensity
Copy link
Contributor

Sounds easy enough, I'll implement it in this PR.

@ZeroIntensity
Copy link
Contributor

Added it under a new modes attribute!

Copy link

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
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants