From bf86a030a08b325a08e031ff71d0716a2f2f122b Mon Sep 17 00:00:00 2001 From: wakonig_k Date: Fri, 25 Jul 2025 16:06:40 +0200 Subject: [PATCH] fix(bec widgets): always call cleanup of child widgets on cleanup --- bec_widgets/utils/bec_connector.py | 20 ------------------- bec_widgets/utils/bec_widget.py | 8 ++++++++ bec_widgets/utils/compact_popup.py | 9 --------- bec_widgets/widgets/containers/dock/dock.py | 1 + tests/end-2-end/test_rpc_widgets_e2e.py | 3 ++- tests/unit_tests/test_compact_popup_widget.py | 7 ------- 6 files changed, 11 insertions(+), 37 deletions(-) diff --git a/bec_widgets/utils/bec_connector.py b/bec_widgets/utils/bec_connector.py index 12f95d2d..c04b3fd3 100644 --- a/bec_widgets/utils/bec_connector.py +++ b/bec_widgets/utils/bec_connector.py @@ -161,8 +161,6 @@ class BECConnector: # 2) Enforce unique objectName among siblings with the same BECConnector parent self.setParent(parent) - if isinstance(self.parent(), QObject) and hasattr(self, "cleanup"): - self.parent().destroyed.connect(self._run_cleanup_on_deleted_parent) # Error popups self.error_utility = ErrorPopupUtility() @@ -186,24 +184,6 @@ class BECConnector: except: logger.error(f"Error getting parent_id for {self.__class__.__name__}") - def _run_cleanup_on_deleted_parent(self) -> None: - """ - Run cleanup on the deleted parent. - This method is called when the parent is deleted. - """ - if not hasattr(self, "cleanup"): - return - try: - if not self._destroyed: - self.cleanup() - self._destroyed = True - except Exception: - content = traceback.format_exc() - logger.info( - "Failed to run cleanup on deleted parent. " - f"This is not necessarily an error as the parent may be deleted before the child and includes already a cleanup. The following exception was raised:\n{content}" - ) - def change_object_name(self, name: str) -> None: """ Change the object name of the widget. Unregister old name and register the new one. diff --git a/bec_widgets/utils/bec_widget.py b/bec_widgets/utils/bec_widget.py index b43ee769..7482e74b 100644 --- a/bec_widgets/utils/bec_widget.py +++ b/bec_widgets/utils/bec_widget.py @@ -3,6 +3,7 @@ from __future__ import annotations from typing import TYPE_CHECKING import darkdetect +import shiboken6 from bec_lib.logger import bec_logger from qtpy.QtCore import QObject, Slot from qtpy.QtWidgets import QApplication @@ -102,6 +103,13 @@ class BECWidget(BECConnector): # All widgets need to call super().cleanup() in their cleanup method logger.info(f"Registry cleanup for widget {self.__class__.__name__}") self.rpc_register.remove_rpc(self) + children = self.findChildren(BECWidget) + for child in children: + if not shiboken6.isValid(child): + # If the child is not valid, it means it has already been deleted + continue + child.close() + child.deleteLater() def closeEvent(self, event): """Wrap the close even to ensure the rpc_register is cleaned up.""" diff --git a/bec_widgets/utils/compact_popup.py b/bec_widgets/utils/compact_popup.py index 1c3e92e4..cb5203b8 100644 --- a/bec_widgets/utils/compact_popup.py +++ b/bec_widgets/utils/compact_popup.py @@ -259,12 +259,3 @@ class CompactPopupWidget(QWidget): @expand_popup.setter def expand_popup(self, popup: bool): self._expand_popup = popup - - def closeEvent(self, event): - # Called by Qt, on closing - since the children widgets can be - # BECWidgets, it is good to explicitely call 'close' on them, - # to ensure proper resources cleanup - for child in self.container.findChildren(QWidget, options=Qt.FindDirectChildrenOnly): - child.close() - - super().closeEvent(event) diff --git a/bec_widgets/widgets/containers/dock/dock.py b/bec_widgets/widgets/containers/dock/dock.py index 20318dfb..07f81a45 100644 --- a/bec_widgets/widgets/containers/dock/dock.py +++ b/bec_widgets/widgets/containers/dock/dock.py @@ -389,6 +389,7 @@ class BECDock(BECWidget, Dock): if widget in self.widgets: self.widgets.remove(widget) widget.close() + widget.deleteLater() def delete_all(self): """ diff --git a/tests/end-2-end/test_rpc_widgets_e2e.py b/tests/end-2-end/test_rpc_widgets_e2e.py index 97f54380..b513068d 100644 --- a/tests/end-2-end/test_rpc_widgets_e2e.py +++ b/tests/end-2-end/test_rpc_widgets_e2e.py @@ -81,6 +81,7 @@ def test_available_widgets(qtbot, connected_client_gui_obj): # Number of top level widgets, should be 4 top_level_widgets_count = 12 assert len(gui._server_registry) == top_level_widgets_count + names = set(list(gui._server_registry.keys())) # Number of widgets with parent_id == None, should be 2 widgets = [ widget for widget in gui._server_registry.values() if widget["config"]["parent_id"] is None @@ -151,7 +152,7 @@ def test_available_widgets(qtbot, connected_client_gui_obj): raise RuntimeError( f"Widget {object_name} was not removed properly. The number of top level widgets " f"is {len(gui._server_registry)} instead of {top_level_widgets_count}. The following " - f"widgets are still registered: {list(gui._server_registry.keys())}." + f"widgets are not cleaned up: {set(gui._server_registry.keys()) - names}" ) from exc # Number of widgets with parent_id == None, should be 2 widgets = [ diff --git a/tests/unit_tests/test_compact_popup_widget.py b/tests/unit_tests/test_compact_popup_widget.py index 21c730d1..4e75c76d 100644 --- a/tests/unit_tests/test_compact_popup_widget.py +++ b/tests/unit_tests/test_compact_popup_widget.py @@ -31,13 +31,6 @@ def compact_popup(qtbot): yield widget -def test_widget_closing(qtbot, compact_popup): - with mock.patch.object(compact_popup.contained, "close") as close_method: - compact_popup.close() - qtbot.waitUntil(lambda: not compact_popup.isVisible(), timeout=1000) - close_method.assert_called_once() - - def test_size_policy(compact_popup): csp = compact_popup.sizePolicy() assert csp.horizontalPolicy() == QSizePolicy.Expanding