-
-
Notifications
You must be signed in to change notification settings - Fork 669
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
Fixed widget IDs to be reusable after window closes #2517
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.
I believe you've correctly identified the source of the bug here; but both the fix and the test miss a lot of details.
core/src/toga/window.py
Outdated
@@ -272,6 +272,9 @@ def close(self) -> None: | |||
undefined, except for :attr:`closed` which can be used to check if the window | |||
was closed. | |||
""" | |||
if getattr(self, "content", None) is not None: |
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.
In what conditions can content not exist as an attribute? It's one of the first attributes set on Window
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.
sorry, I meant to do:
if self.content:
core/src/toga/window.py
Outdated
@@ -272,6 +272,9 @@ def close(self) -> None: | |||
undefined, except for :attr:`closed` which can be used to check if the window | |||
was closed. | |||
""" | |||
if getattr(self, "content", None) is not None: | |||
self.content.clear() |
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.
Why is this needed? AFAICT, setting the window to None should be sufficient, and will propagate to all children. Clearing the children from content means the ownership hierarchy of content
will be destroyed.
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, I hadn't noticed _remove()
also propagates to the children as well. Also, does ownership hierarchy matter when we are clearing all the widgets?
core/tests/test_window.py
Outdated
"""Widget IDs can be reused after the associated widget's window is closed.""" | ||
|
||
# Common widget IDs | ||
common_widget_ids = {"content": "window_content", "label": "sample_label"} |
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 don't see why this is a dictionary containing 2 constants, instead of just using 2 constants. The "dictionary" features of this are completely unused - we just need 2 constant strings.
core/tests/test_window.py
Outdated
id=common_widget_ids["content"], | ||
children=[toga.Label(text="Sample Label", id=common_widget_ids["label"])], | ||
) | ||
# Show the second window and check that it is in the app's windows registry. |
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.
There seems to be some confusion here. We aren't concerned about the window registry at all (not that there even is one. There's a list of windows).
What we care about is the widget registry. The tests you've got here verify the window has been created, but that's not the thing that is the source of the bugs here.
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 is a lot closer. The implementation is simpler, and the tests are actually validating the thing that has been changed.
However, the tests are only verifying the "good" path through the code - there's no validation of the "bad" path. Consider the cases of:
- Adding the a widget with a re-used ID to an existing (and visible) window
- Constructing a new window that re-uses an ID
It should be legal to create a widget with a duplicated ID; it's only when the widget is added to a window that a problem will occur (as widgets don't appear in the registry until they're part of a layout).
There's also a difference between a parent widget with a duplicated ID and a child widget with a duplicated ID. Both will raise an error - but the paths to generating that error may be different.
I also noticed another bug while testing: windows can have the same IDs, which defeats the purpose of ensuring uniqueness. Additionally, I didn't find any apparent way to retrieve a window by its ID, other than by iterating through all windows with the following code:
Is this the correct approach, or am I missing something?
|
I can't see any reason we shouldn't allow lookup-by-ID for windows; in fact, most (if not all) of the implementation of WidgetRegistry should be reusable as well. However, that's new feature, not a bug, and it should be handled independently of this PR. |
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.
A couple of corrections on how you're using (or not using) pytest test mechanisms.
core/tests/test_window.py
Outdated
try: | ||
third_window_content = toga.Box(id=CONTENT_WIDGET_ID) | ||
except KeyError: | ||
raise AssertionError( |
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 would usually be pytest.fail()
, not a manual raise of AssertionError.
core/tests/test_window.py
Outdated
except KeyError: | ||
assert True | ||
else: | ||
raise AssertionError( |
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 whole block should be with pytest.raises
.
core/tests/test_window.py
Outdated
try: | ||
new_label_widget = toga.Label(text="Sample Label", id=LABEL_WIDGET_ID) | ||
except KeyError: | ||
raise AssertionError( |
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.
As above, this is pytest.fail()
core/tests/test_window.py
Outdated
except KeyError: | ||
assert True | ||
else: | ||
raise AssertionError( |
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.
As above, this is with pytest.raises
Thanks for the tips. The |
The |
a6d9301
to
7b33e70
Compare
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've made a couple more tweaks to the test to protect against some remaining edge cases, and make the exception tests a little more robust; but otherwise this looks good. Thanks for the fix!
While I don't want to dampen your enthusiasm, I would be wary of having too many open PRs at once. We definitely appreciate both bug fixes and feature improvements, but the PRs you're currently producing consistently require multiple passes and improvements before they get to a final form. We're happy to provide the feedback needed to land these PRs; but when you have multiple PRs in flight at once, it means we (the maintainers/reviewers) end up spending a lot of time providing the same sort of feedback on multiple PRs, rather than having getting one PR to completion, then incorporating the experience you've gained in landing one PR into making the next PR better.
By my count, you currently have 7 open PRs; I'd vastly prefer to see these land before you start opening new PRs - especially if you're proposing adding a feature.
I agree with you. I will complete the remaining PRs first and during this time, I will only open a new PR if any of them will be necessary for landing the existing open PRs. Thank you helping as always 😄 |
Fixes #2514
Fixed widget IDs to be reusable after window closes. Also added a new test to check the behavior.
PR Checklist: