-
-
Notifications
You must be signed in to change notification settings - Fork 698
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
Prime out essential interface
attributes before impl
initialization
#3071
Prime out essential interface
attributes before impl
initialization
#3071
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the report; Your first attempt got the right general idea for the fix (in the implementation layer); the subsequent fix on the GTK backend isn't correct.
The old implementation set on_scroll = None
for exactly the same reason as you've identified - because the widget fired the event handler when the widget is created, so a handler needs to be primed first. However, in #2942, the order of widget instantiation was slightly modified, and as a result, the priming needs to occur before super().__init__()
is invoked.
The assignment of _content
was historically for the same reason.
From an inspection of the code, I think there is a similar potential bug in NumberInput, OptionContainer, Selection, and maybe Canvas and SplitContainer. These might not be triggered in practice, but the potential is there.
I agree, the fix on the interface is correct. But, somehow the dummy backend is triggering the |
No, that's my entire point. The on_scroll handler needs to be primed before the impl is created. Previously, it was, because the call to create the impl was explicit, and it occurred after on_scroll was set to None. Now, the impl is created as part of the call to I'm more intrigued as to why the current code passes both the GTK testbed and the core tests. |
Yes, I meant that we would need to prime the attributes that might be used during the impl initialization, before calling
I am not sure, but the async def test_scrollcontainer_error():
_ = toga.ScrollContainer(content=toga.Box()) But just running the example triggers the |
Ok, so correctly priming the attributes fixes the bug. But, I am still not sure why pytest is not reporting the |
on_scroll
in ScrollContainer
interface
attributes before impl
initialization
faa2dbb
to
68a6111
Compare
68a6111
to
9a70f7b
Compare
interface
attributes before impl
initializationinterface
attributes before impl
initialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ordering for canvas is slightly off, which I've corrected; otherwise, this looks great. Thanks for the fix!
@@ -99,6 +97,8 @@ def __init__( | |||
self.on_alt_release = on_alt_release | |||
self.on_alt_drag = on_alt_drag | |||
|
|||
super().__init__(id=id, style=style) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This placement isn't correct. The live event handlers shouldn't be in place before the widget is created. The comparison should be with the old implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Also, did you find anything regarding pytest not reporting the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't; I can only assume that because the test is running with no delays, the signal propagation order is slightly different, and as a result, the error doesn't surface.
Thank you for catching this and cleaning up after me! Definitely perplexing about pytest not being able to catch it. Even though these are now fixed, it is a little worrying having a class of potential errors that isn't (as far as we know so far) testable. |
Running the following script:
The following error occurs:
Context
It seems like the steps for initialization of widgets has recently been changed. For example, on testing by debug logging, the reported initialization steps of
ScrollContainer()
is:The error occurs in:
The gtk event
changed
gets triggered by the backend early on, and since theScrollContainer
interface(core) initializer calls the Impl initializer while not being completely initialized itself(interface), the gtk callback event throws an error. To reduce code churn, I have gone with the simplest fix.PR Checklist: