Skip to content

Ensure Window always has a content widget #3478

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

Closed
wants to merge 1 commit into from

Conversation

jgao8
Copy link
Contributor

@jgao8 jgao8 commented May 20, 2025

Resolves #2818, which details a bug in which the app crashes if there is no window content. This PR updates the code so that a Window is now guaranteed to have a content widget.

Prior to this change, an error would be generated when running TOGA_BACKEND=toga_textual python -c 'import toga; toga.App(formal_name="MyApp", app_id="MyApp").main_loop()' (as mentioned in #2818). This error now no longer occurs. The output now looks like this:
image

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@mhsmith mhsmith self-requested a review May 20, 2025 18:25
Comment on lines +248 to +249
# Otherwise, set content to an empty Box widget
self.content = content if content else toga.Box()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has resulted in a few branch coverage failures. They could be fixed by removing the if check at each branch and assuming that content is never None.

If we're committing to that invariant, then we should also update the content setter to interpret None in the same way as the constructor does. However, we're not sure whether this invariant is actually a good idea, as it prevents content=None from being round-tripped.

The presence of a default Box in the app's main window has obviously also complicated many of the tests, even if they didn't involve the main window directly.

Let's leave this PR open to discuss this question, and consider alternative approaches to the original issue in #2818.

Copy link
Contributor Author

@jgao8 jgao8 May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative approach in #3479, which is now merged.

@jgao8
Copy link
Contributor Author

jgao8 commented May 21, 2025

Alternative approach in #3479 is now merged.

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.

Textual apps require window content to run
2 participants