Skip to content
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

Ensure that widget IDs can be reused. #2320

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Jan 4, 2024

Reworks the widget registry so that widget IDs can be re-used.

Instead of maintaining both an app-level and window-level registry, a single registry is maintained on the app, with widgets being added or removed as a result of assigning a the widget to the window. The window-level registry is then a filtered proxy over the main app registry. This avoids the problem reported as #2190 where the WeakRefDict at the App level wouldn't be cleared until a widget was garbage collected. Since there is no step where a widget is explicitly removed from the app, there is essentially no trigger (other than garbage collection) that could be used to purge from the app registry.

This means that a widget is not available for ID lookup until it has been assigned to a window as content. This is a backwards incompatible change, but one that aligns ID-based lookup with the DOM getElementByID() method.

The implementation involves some minor changes to the order in which window and app are modified:

  • A widget's app must be assigned before window. This ensures the window assignment can find the app registry to register the widget.
  • A widget's window must be removed before app. This ensures the window can find the app registry to deregister the widget.
  • A widget's app and window must be assigned before any DOM-related operations (e.g., setting parents) are performed. This ensures that in the case of a duplicate ID, the widget isn't left in a partially valid state (e.g., where a parent has been assigned, but it's not part of a layout or part of the registry)

These shouldn't affect anything in normal operation, as assigning window and app shouldn't generally be something the end-user is doing.

Fixes #2190.

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

@mhsmith mhsmith merged commit 98deebf into beeware:main Jan 9, 2024
35 checks passed
@freakboy3742 freakboy3742 deleted the widget-lookup branch January 9, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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