Initialize Window event handlers before impl construction (#4347)#4359
Conversation
Closes beeware#4347 (and the duplicate beeware#4357). When 'toga.Window.__init__' creates 'self._impl', some platform layers fire resize / focus callbacks before construction returns: - Cocoa: 'windowDidResize_' fires when the requested size has to be clamped to fit the screen or the contained widgets. - WinForms .NET Framework 4.x: 'Activated' fires during 'Form' creation when the window becomes the active form. Both callbacks dispatch through the interface property getters, which read 'self._on_resize' / 'self._on_gain_focus' / etc. directly. Those attributes are only assigned at the *end* of '__init__' via the 'self.on_X = on_X' lines, so the in-construction callback raises 'AttributeError: "Window" object has no attribute "_on_resize"' (see issue beeware#4347 traceback). Fix: install wrapped no-op handlers ('wrapped_handler(self, None)') for all six on_X attributes before constructing 'self._impl'. The explicit 'self.on_X = on_X' assignments at the end of '__init__' still run and override these defaults with the user-supplied handlers. Adds a regression test in 'tests/window/test_window.py' that monkey- patches the dummy Window impl to fire all six callbacks from inside its '__init__'. The new test passes on this branch and fails on main with AttributeError.
freakboy3742
left a comment
There was a problem hiding this comment.
Thanks for the PR! I've cleaned up a couple of pieces in the implementation (details inline); but this was really helpful as a starting point (especially the test case).
| self._on_close = wrapped_handler(self, None) | ||
| self._on_gain_focus = wrapped_handler(self, None) | ||
| self._on_lose_focus = wrapped_handler(self, None) | ||
| self._on_show = wrapped_handler(self, None) | ||
| self._on_hide = wrapped_handler(self, None) | ||
| self._on_resize = wrapped_handler(self, None) |
There was a problem hiding this comment.
The wrapping can be done automatically by assigning None as the actual handler (and that also ensures that cleanup behaviour is installed correctly - e.g., in the case of on_close)
| self._on_close = wrapped_handler(self, None) | |
| self._on_gain_focus = wrapped_handler(self, None) | |
| self._on_lose_focus = wrapped_handler(self, None) | |
| self._on_show = wrapped_handler(self, None) | |
| self._on_hide = wrapped_handler(self, None) | |
| self._on_resize = wrapped_handler(self, None) | |
| self.on_close = None | |
| self.on_gain_focus = None | |
| self.on_lose_focus = None | |
| self.on_show = None | |
| self.on_hide = None | |
| self.on_resize = None |
| "on_show", | ||
| "on_hide", | ||
| "on_resize", | ||
| ): |
There was a problem hiding this comment.
A loop through a number of different test assertions is a likely indicator that a test should be parameterised. That way we'll get an independent failure signal if one of the handler is modified unexpectedly.
| @@ -0,0 +1 @@ | |||
| Window event handlers are now initialized to no-op defaults before the platform implementation is created, so resize and focus callbacks that fire during impl construction (Cocoa ``windowDidResize_``, WinForms .NET Framework 4.x ``Activated``) no longer raise ``AttributeError`` on ``_on_resize`` / ``_on_gain_focus`` / etc. | |||
There was a problem hiding this comment.
We can be a bit more terse and user-focused here. The final release note will reference the ticket number, so someone wanting more detail can find out more; so we only indicate the nature of the problem at a high level in the release note.
| Window event handlers are now initialized to no-op defaults before the platform implementation is created, so resize and focus callbacks that fire during impl construction (Cocoa ``windowDidResize_``, WinForms .NET Framework 4.x ``Activated``) no longer raise ``AttributeError`` on ``_on_resize`` / ``_on_gain_focus`` / etc. | |
| Runtime errors caused by Window event handlers firing as part of the window initialization process have now been silenced. |
Closes #4347 (and the duplicate #4357).
Summary
When
toga.Window.__init__createsself._impl, some platform layers fire resize / focus callbacks before construction returns:windowDidResize_fires when the requested size has to be clamped to fit the screen or the contained widget's natural size (the(128, 128)window with aLabel(width=256)reproducer in the issue).Activatedfires duringFormcreation when the window becomes the active form (per documentapp sample: exception 'DocumentWindow' object has no attribute '_on_gain_focus' on Windows #4357).Both callbacks dispatch through the interface property getters, which read
self._on_resize/self._on_gain_focus/ etc. directly. Those attributes are only assigned at the end of__init__via theself.on_X = on_Xlines, so the in-construction callback raises:Fix
Install wrapped no-op handlers (
wrapped_handler(self, None)) for all six on_X attributes before constructingself._impl. The explicitself.on_X = on_Xassignments at the end of__init__still run and override these defaults with the user-supplied handlers, so behavior is unchanged for the normal flow.This matches the maintainer's analysis on #4347 and #4357: "the on_gain_focus handler needs to exist before the window is created."
Tests
Adds
test_window_handler_attrs_initialized_before_implincore/tests/window/test_window.pythat monkey-patches the dummyWindow.__init__to fire all six interface callbacks from inside its constructor. The test passes on this branch and fails on main withAttributeError: 'Window' object has no attribute '_on_resize'-- exactly the bug from the issue.PR Checklist
changes/4347.bugfix.md)