KeyError when re-creating a sub-window which contains widgets with ids

[t-arn]

Describe the bug

On desktops, when you close and then re-create a sub-window which contains widgets that have an id, you get following error:

Error in handler: "There is already a widget with the id 'label1'"
Traceback (most recent call last):
  File "C:\Projects\Python\Toga\.venv39\lib\site-packages\toga\handlers.py", line 79, in _handler
    result = handler(interface, *args, **kwargs)
  File "C:\Projects\Python\Toga\examples\window\window\app.py", line 46, in do_new_windows
    non_resize_window.content = toga.Box(
  File "C:\Projects\Python\Toga\.venv39\lib\site-packages\toga\window.py", line 214, in content
    widget.app = self.app
  File "C:\Projects\Python\Toga\.venv39\lib\site-packages\toga\widgets\base.py", line 234, in app
    child.app = app
  File "C:\Projects\Python\Toga\.venv39\lib\site-packages\toga\widgets\base.py", line 238, in app
    app.widgets.add(self)
  File "C:\Projects\Python\Toga\.venv39\lib\site-packages\toga\widgets\base.py", line 40, in add
    raise KeyError(f"There is already a widget with the id {widget.id!r}")
KeyError: "There is already a widget with the id 'label1'"

Steps to reproduce

1. Go to the window example
2. Add a label to the non-resizable window: non_resize_window.content = toga.Box(children=[toga.Label("This window is not resizable", id="label1")])
3. Run the example
4. Click on "Create Window" button
5. Close the non-sizable window
6. Click on "Create Window" button again
7. You'll see following error: KeyError: "There is already a widget with the id 'label1'"

Expected behavior

It should be possible to re-create the sub-window without getting this KeyError.

Screenshots

No response

Environment

• Operating System: Windows
• Python version: 3.9
• Software versions:
  • Briefcase: 0.3.15
  • toga: 0.3.2.dev1367+g9021f9106

Logs

Additional context

No response

[freakboy3742]

Thanks for the report. I can confirm I can reproduce this problem; it appears that when the window is being closed, the content of that window isn't being deregistered from the app registry.

That shouldn't require an additional step - the app registry uses weakrefs, so as soon as there's no object referencing it, the widget should be cleaned up. This appears to be a garbage collection issue. If you put a call to import gc; gc.collect() on a button somewhere (say, the change title) button, so you can force a single garbage collection run, the widget is purged from the app registry, and you can re-create a widget with the given label.

The naive solution would be to invoke the garbage collector on a window close... but I'm not sure if that's the right permanent solution.

[freakboy3742]

A suggestion from @mhsmith about a possible fix - instead of treating the app registry as an index of every widget in existence (which becomes subject to garbage collection), it should only store widgets that are "in the DOM" - that is - widgets that are in a layout. That way, we can reliably say a widget is removed from the app registry when the window is closed (because it has been removed from the app's "DOM"), even though there may still be a reference to the widget somewhere in code. When it is re-added to a layout, it gets re-added to the app registry.

This would theoretically allow for 2 widgets with the same ID to exist, but you wouldn't be able to look up a widget by instance unless it was in a layout, and only one of them would be able to be in a layout at any given time.

One possible way to implement this would be to replace the app widget registry with an implementation that is a proxy around the window registries .That is, app.widgets.__getitem__() would effectively become:

def __getitem__(self, widget_id):
    for window in self.app.windows:
        try:
            return window.widgets[widget_id]
        except KeyError:
            pass
    raise KeyError()

That is - you look up a widget on the app by looking up the widget on each window in the app. This would also require that the __setitem__ mechanism on the window registry would check all other registries on all other windows before allowing an ID to be added. Widgets would no longer be explicitly added to the app registry, just the window registry.

[hyuri]

Same issue here.

This part works great (pseudo code):

page1 = ScrollContainer(...) # Contains widgets with IDs
page2 = ScrollContainer(...) # Contains widgets with IDs of the same name

page_container = Box(
    ...
    children=[
        page1
    ]
)

main_window.content = page_container

Error happens on the last line below:

page_container.remove(page1)
page_container.add(page2)
main_window.content = page_container

[hyuri]

I found a workaround:
Right before re-creating/re-adding your widgets, add this:
app.widgets.clear()

Solved all my issues related to this.

Let me know if it's a bad idea, or if it might cause a crash.

[freakboy3742]

@hyuri I'd consider it at least a little risky. Calling clear() like that will mean that all widgets (even ones still in use on other pages, or other parts of the app) will be purged from the widget cache. Toga doesn't use this widget cache internally, except when cleaning up; so if you purge the cache, then move/repartent a widget that is still visible, you could get some interesting behavior. It might work for your specific circumstance, but I personally wouldn't rely on it.

It's also not a public API, so I wouldn't consider it a long term solution.