diff --git a/bec_widgets/utils/bec_widget.py b/bec_widgets/utils/bec_widget.py index fb7b172a..d7beabb0 100644 --- a/bec_widgets/utils/bec_widget.py +++ b/bec_widgets/utils/bec_widget.py @@ -111,8 +111,9 @@ class BECWidget(BECConnector): def _connect_to_theme_change(self): """Connect to the theme change signal.""" qapp = QApplication.instance() + self._theme_connection = None if hasattr(qapp, "theme"): - SafeConnect(self, qapp.theme.theme_changed, self._update_theme) + self._theme_connection = SafeConnect(self, qapp.theme.theme_changed, self._update_theme) @SafeSlot(str) @SafeSlot() @@ -402,6 +403,9 @@ class BECWidget(BECConnector): def cleanup(self): """Cleanup the widget.""" + theme_connection = getattr(self, "_theme_connection", None) + if theme_connection is not None: + theme_connection.invalidate() with RPCRegister.delayed_broadcast(): # All widgets need to call super().cleanup() in their cleanup method logger.info(f"Registry cleanup for widget {self.__class__.__name__}") diff --git a/bec_widgets/utils/error_popups.py b/bec_widgets/utils/error_popups.py index 1df1f1f4..993e2f36 100644 --- a/bec_widgets/utils/error_popups.py +++ b/bec_widgets/utils/error_popups.py @@ -192,13 +192,55 @@ def SafeProperty( return decorator +class _SafeConnection: + """Weak receiver wrapper with explicit invalidation for pending deliveries.""" + + def __init__(self, instance, signal, slot): + self._active = True + self._connected = True + self._instance = safe_ref(instance) + self._slot = safe_ref(slot) + self._signal = signal + + def invalidate(self, *_args) -> None: + """Block deliveries without touching Qt during receiver destruction.""" + self._active = False + + def _disconnect(self) -> None: + if not self._connected: + return + self._connected = False + try: + self._signal.disconnect(self) + except (RuntimeError, TypeError): + pass + + def __call__(self, *args): + if not self._active: + self._disconnect() + return + instance = self._instance() + slot = self._slot() + if ( + instance is None + or slot is None + or getattr(instance, "_destroyed", False) + or not shiboken6.isValid(instance) + ): + self.invalidate() + self._disconnect() + return + slot(*args) + + def SafeConnect(instance, signal, slot): # pylint: disable=invalid-name """ Method to safely handle Qt signal-slot connections. The python object is only forwarded as a weak reference to avoid stale objects. Once the instance or slot has been garbage collected, marked as destroyed, or its C++ object deleted, the connection removes itself from the signal on the next emission, so long-lived signals (e.g. theme_changed) do not - accumulate dead wrappers. + accumulate dead wrappers. Receiver destruction first marks the wrapper inactive without + disconnecting Qt signals in destructor context. Args: instance: The instance to connect. @@ -206,38 +248,17 @@ def SafeConnect(instance, signal, slot): # pylint: disable=invalid-name slot: The slot to connect. Returns: - Callable: The wrapper connected to the signal. It can be passed to + Callable: The wrapper connected to the signal. Its ``invalidate()`` method blocks + pending deliveries at cleanup start. It can also be passed to ``signal.disconnect`` for explicit early disconnection. Example: >>> SafeConnect(self, qapp.theme.theme_changed, self._update_theme) """ - weak_instance = safe_ref(instance) - weak_slot = safe_ref(slot) - - def safe_slot(*connect_args): - instance_ = weak_instance() - slot_func = weak_slot() - - # Instance or slot garbage collected, instance cleaned up, or C++ side - # deleted: remove this stale connection from the signal. - if ( - instance_ is None - or slot_func is None - or getattr(instance_, "_destroyed", False) - or not shiboken6.isValid(instance_) - ): - try: - signal.disconnect(safe_slot) - except (RuntimeError, TypeError): - # Signal owner already gone or connection already removed. - pass - return - - slot_func(*connect_args) - + safe_slot = _SafeConnection(instance, signal, slot) signal.connect(safe_slot) + instance.destroyed.connect(safe_slot.invalidate) return safe_slot diff --git a/tests/unit_tests/test_bec_widget_lifecycle.py b/tests/unit_tests/test_bec_widget_lifecycle.py index 4fc1aef6..9f0d6df7 100644 --- a/tests/unit_tests/test_bec_widget_lifecycle.py +++ b/tests/unit_tests/test_bec_widget_lifecycle.py @@ -45,6 +45,21 @@ def test_close_runs_cleanup_once(qtbot, mocked_client): _flush_deferred_deletes(qtbot) +def test_cleanup_invalidates_theme_subscription_while_widget_is_valid(qtbot, mocked_client): + widget = LifecycleWidget(client=mocked_client) + connection = widget._theme_connection + assert connection._active + + widget.cleanup() + assert shiboken6.isValid(widget) + assert not widget._destroyed + assert not connection._active + + widget.close() + widget.deleteLater() + _flush_deferred_deletes(qtbot) + + def test_delete_later_without_close_runs_cleanup(qtbot, mocked_client): widget = LifecycleWidget(client=mocked_client) gui_id = widget.gui_id diff --git a/tests/unit_tests/test_error_utils.py b/tests/unit_tests/test_error_utils.py index bd2c6bd4..f6fb5605 100644 --- a/tests/unit_tests/test_error_utils.py +++ b/tests/unit_tests/test_error_utils.py @@ -1,8 +1,9 @@ from unittest.mock import patch import pytest +import shiboken6 from bec_lib.logger import bec_logger -from qtpy.QtCore import QObject, Signal +from qtpy.QtCore import QObject, Qt, Signal from qtpy.QtWidgets import QMessageBox from bec_widgets.utils.error_popups import ErrorPopupUtility, ExampleWidget, SafeProperty, SafeSlot @@ -243,6 +244,45 @@ def test_safe_connect_removes_stale_connection(qtbot): emitter.deleteLater() +def test_safe_connect_invalidated_before_queued_delivery(qtbot): + """An inactive subscription must not deliver an already queued signal.""" + from bec_widgets.utils.error_popups import SafeConnect + + emitter = _ArgSignalEmitter() + receiver = _SafeConnectReceiver() + wrapper = SafeConnect(receiver, emitter.str_signal, receiver.on_value) + emitter.str_signal.disconnect(wrapper) + emitter.str_signal.connect(wrapper, Qt.ConnectionType.QueuedConnection) + + emitter.str_signal.emit("queued") + assert receiver.calls == [] + wrapper.invalidate() + qtbot.wait(10) + assert shiboken6.isValid(receiver) + assert receiver.calls == [] + # Also cover direct calls retained by code outside the Qt connection. + wrapper("direct") + assert receiver.calls == [] + receiver.deleteLater() + emitter.deleteLater() + + +def test_safe_connect_destruction_only_invalidates(): + """Destruction blocks delivery without disconnecting signals in the destructor.""" + from bec_widgets.utils.error_popups import SafeConnect + + emitter = _ArgSignalEmitter() + receiver = _SafeConnectReceiver() + wrapper = SafeConnect(receiver, emitter.str_signal, receiver.on_value) + with patch.object(wrapper, "_disconnect") as disconnect: + shiboken6.delete(receiver) + assert not wrapper._active + disconnect.assert_not_called() + emitter.str_signal.emit("after-death") + assert not wrapper._connected + shiboken6.delete(emitter) + + def test_safe_slot_override_params_apply_to_single_call(): """Regression test for BW-004: _override_slot_params must not permanently mutate the decorator's defaults."""