toga icon indicating copy to clipboard operation
toga copied to clipboard

Set Content When Initializing Window

Open cosmastech opened this issue 1 year ago • 4 comments

This change allows for initializing a window with content, rather than setting it after the window has been created.

main_box = toga.Box()

# was
main_window = toga.MainWindow(title="Window title!")
main_window.content = main_box

# now
main_window = toga.MainWindow(
	title="Window title!",
	content=main_box
)

Fixes #2307

PR Checklist:

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

cosmastech avatar Jan 01 '24 18:01 cosmastech

@freakboy3742 thanks for tip!

Any advice on where the tests for this should live? And maybe what you would normally expect for these test cases?

Thanks!

cosmastech avatar Jan 11 '24 20:01 cosmastech

Any advice on where the tests for this should live? And maybe what you would normally expect for these test cases?

I'd expect to see 2 sets of tests. The first is in core/tests/window/test_window.py, validating the public API behaves as expected; the second is in testbed/tests/test_window.py, validating that each backend behaves as expected.

What we're looking for is coverage - every line of code being exercised. We enforce 100% branch coverage as part of the test suite - even if the code works (and you fix the towncrier error), the test suite will fail if every line of code in the app isn't exercised.

So - we need an example of instantiating Window with content, validating that the content has been set, in both places.

freakboy3742 avatar Jan 11 '24 23:01 freakboy3742

@freakboy3742 I don't feel great about the testbed test that I added, as it doesn't feel like it's really testing anything of significance. Let me know if you have any guidance there.

cosmastech avatar Jan 15 '24 14:01 cosmastech

@freakboy3742 I don't feel great about the testbed test that I added, as it doesn't feel like it's really testing anything of significance. Let me know if you have any guidance there.

I'd say it's not far off. I'd suggest two improvements, both of which will help confirm that it actually working:

  1. Put something in the box. At the moment it will be a white box; when you run the test manually in slow mode, it's going to be difficult to tell the difference between "this window has default content" and "this window has the specified content".
  2. Validate some layout properties of the box. One of the ways to verify that content is in a layout is to check its size - if you use a box as window content, it will stretch to fill the window. Verifying that the box is the same size as the window (allowing for window chrome etc) - or, at least, that it's much bigger than a "default" size of 100x100 - will provide some verification that the content is actually in the window, not just returning as the current value of window._content.

I'd also suggest that the window creation needs to be in a try/finally block. It's not likely, but if the test fails after the window is created, the test will leave a a stray window open. This might alter the results of other tests that count the number of open windows. Each test should clean up after itself, and ensure that it leaves the app in as close to a "clean" state as possible.

freakboy3742 avatar Jan 15 '24 22:01 freakboy3742