Fixed widget IDs to be reusable after window closes #2517

proneon267 · 2024-04-20T16:23:42Z

Fixes #2514

Fixed widget IDs to be reusable after window closes. Also added a new test to check the behavior.

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

freakboy3742

I believe you've correctly identified the source of the bug here; but both the fix and the test miss a lot of details.

freakboy3742 · 2024-04-21T23:14:33Z

core/src/toga/window.py

@@ -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:


In what conditions can content not exist as an attribute? It's one of the first attributes set on Window

sorry, I meant to do:

if self.content:

freakboy3742 · 2024-04-21T23:16:53Z

core/src/toga/window.py

@@ -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()


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.

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?

freakboy3742 · 2024-04-21T23:18:41Z

core/tests/test_window.py

+    """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"}


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.

freakboy3742 · 2024-04-21T23:25:50Z

core/tests/test_window.py

+        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 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.

freakboy3742

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.

proneon267 · 2024-04-22T08:06:04Z

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:

for window in app.windows:
    if window.id == CHOSEN_WINDOW_ID:
        # Found the window
        break

Is this the correct approach, or am I missing something?
If not, could we make the Window Registry API symmetrical to the Widget Registry? This would ensure that only windows with unique IDs are added to the windows registry, and also allow us to retrieve a window by its ID using:

app.windows[CHOSEN_WINDOW_ID]

freakboy3742 · 2024-04-24T02:18:20Z

.. could we make the Window Registry API symmetrical to the Widget Registry? This would ensure that only windows with unique IDs are added to the windows registry, and also allow us to retrieve a window by its ID using:

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.

freakboy3742

A couple of corrections on how you're using (or not using) pytest test mechanisms.

freakboy3742 · 2024-04-26T06:21:58Z

core/tests/test_window.py

+    try:
+        third_window_content = toga.Box(id=CONTENT_WIDGET_ID)
+    except KeyError:
+        raise AssertionError(


This would usually be pytest.fail(), not a manual raise of AssertionError.

freakboy3742 · 2024-04-26T06:22:48Z

core/tests/test_window.py

+    except KeyError:
+        assert True
+    else:
+        raise AssertionError(


This whole block should be with pytest.raises.

freakboy3742 · 2024-04-26T06:23:01Z

core/tests/test_window.py

+    try:
+        new_label_widget = toga.Label(text="Sample Label", id=LABEL_WIDGET_ID)
+    except KeyError:
+        raise AssertionError(


As above, this is pytest.fail()

freakboy3742 · 2024-04-26T06:23:24Z

core/tests/test_window.py

+    except KeyError:
+        assert True
+    else:
+        raise AssertionError(


As above, this is with pytest.raises

proneon267 · 2024-04-27T15:52:36Z

Thanks for the tips. The with pytest.raises is less verbose than try except else blocks, but I didn't notice any way to specify the custom error message with with pytest.raises. Is that the intended behavior or am I missing something?

freakboy3742 · 2024-04-28T23:51:07Z

Thanks for the tips. The with pytest.raises is less verbose than try except else blocks, but I didn't notice any way to specify the custom error message with with pytest.raises. Is that the intended behavior or am I missing something?

with pytest.raises(ValueError, match=r"You forgot to include .* in your arguments"`:
    ...

The match argument is a regular expression that will be compared against the message of the exception raised. There's no need to customise the message of the test - the default return value is clear enough.

proneon267 · 2024-04-29T01:40:02Z

Thanks, I have changed the test to use pytest helpers. Let me know if any other changes are needed. I am currently working on #2484 and will start a new PR for changing the window registry implementation when #2484 will near completion.

freakboy3742

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.

proneon267 · 2024-04-29T04:53:15Z

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 😄

proneon267 added 2 commits April 20, 2024 09:16

Fixed widget IDs to be reusable after window closes

1e3743c

Added changelog

01277c0

freakboy3742 requested changes Apr 21, 2024

View reviewed changes

Fixed tests and core

de77562

proneon267 requested a review from freakboy3742 April 22, 2024 01:34

Empty commit for CI

035d040

freakboy3742 requested changes Apr 22, 2024

View reviewed changes

proneon267 added 2 commits April 24, 2024 08:38

Modified test

2ed4c69

Empty commit for CI

d1d7396

proneon267 requested a review from freakboy3742 April 26, 2024 01:08

freakboy3742 requested changes Apr 26, 2024

View reviewed changes

modified to use pytest helpers

5a7f3fe

proneon267 requested a review from freakboy3742 April 27, 2024 15:52

Minor tweaks to window ID test.

7b33e70

freakboy3742 force-pushed the widget_id_reusable_fix branch from a6d9301 to 7b33e70 Compare April 29, 2024 03:44

freakboy3742 approved these changes Apr 29, 2024

View reviewed changes

freakboy3742 merged commit 5242026 into beeware:main Apr 29, 2024
34 checks passed

proneon267 deleted the widget_id_reusable_fix branch April 29, 2024 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed widget IDs to be reusable after window closes #2517

Fixed widget IDs to be reusable after window closes #2517

proneon267 commented Apr 20, 2024

freakboy3742 left a comment

freakboy3742 Apr 21, 2024

proneon267 Apr 22, 2024

freakboy3742 Apr 21, 2024

proneon267 Apr 22, 2024

freakboy3742 Apr 21, 2024

freakboy3742 Apr 21, 2024

freakboy3742 left a comment

proneon267 commented Apr 22, 2024

freakboy3742 commented Apr 24, 2024

freakboy3742 left a comment

freakboy3742 Apr 26, 2024

freakboy3742 Apr 26, 2024

freakboy3742 Apr 26, 2024

freakboy3742 Apr 26, 2024

proneon267 commented Apr 27, 2024

freakboy3742 commented Apr 28, 2024

proneon267 commented Apr 29, 2024

freakboy3742 left a comment

proneon267 commented Apr 29, 2024

Fixed widget IDs to be reusable after window closes #2517

Fixed widget IDs to be reusable after window closes #2517

Conversation

proneon267 commented Apr 20, 2024

PR Checklist:

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freakboy3742 left a comment

Choose a reason for hiding this comment

proneon267 commented Apr 22, 2024

freakboy3742 commented Apr 24, 2024

freakboy3742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proneon267 commented Apr 27, 2024

freakboy3742 commented Apr 28, 2024

proneon267 commented Apr 29, 2024

freakboy3742 left a comment

Choose a reason for hiding this comment

proneon267 commented Apr 29, 2024