From a1f3730c60650ad5b9dc76f979adca0578bbd3f3 Mon Sep 17 00:00:00 2001 From: wyzula-jan Date: Sat, 3 Oct 2026 16:20:40 +0200 Subject: [PATCH] fix(plots): drop deferred timers of closed plot widgets --- bec_widgets/utils/crosshair.py | 8 ++++ bec_widgets/widgets/plots/image/image.py | 5 +-- .../widgets/plots/motor_map/motor_map.py | 14 +++++++ bec_widgets/widgets/plots/plot_base.py | 25 ++++++++++- .../scatter_waveform/scatter_waveform.py | 7 ++-- .../widgets/plots/waveform/waveform.py | 5 ++- .../services/device_browser/device_browser.py | 2 + tests/unit_tests/test_crosshair.py | 37 +++++++++++++++++ tests/unit_tests/test_device_browser.py | 13 ++++++ tests/unit_tests/test_image_view_next_gen.py | 24 +++++++++++ tests/unit_tests/test_motor_map_next_gen.py | 41 +++++++++++++++++++ tests/unit_tests/test_plot_base_next_gen.py | 17 ++++++++ tests/unit_tests/test_scatter_waveform.py | 34 +++++++++++++++ tests/unit_tests/test_waveform.py | 28 +++++++++++++ 14 files changed, 250 insertions(+), 10 deletions(-) diff --git a/bec_widgets/utils/crosshair.py b/bec_widgets/utils/crosshair.py index 2ddfed4e..c02b7ce9 100644 --- a/bec_widgets/utils/crosshair.py +++ b/bec_widgets/utils/crosshair.py @@ -977,6 +977,14 @@ class Crosshair(QObject): self.clear_markers() def cleanup(self): + # The plot's scene outlives the crosshair: detach from its mouse signals first, so + # no mouse move queued in the rate-limiting proxy, and no click, reaches it afterwards. + if self.proxy is not None: # None once an earlier cleanup() detached it + self.proxy.disconnect() + self.proxy = None + scene = self.plot_item.scene() + if scene is not None: + scene.sigMouseClicked.disconnect(self.mouse_clicked) self.reset() self.clear_pin() self.plot_item.removeItem(self.v_line) diff --git a/bec_widgets/widgets/plots/image/image.py b/bec_widgets/widgets/plots/image/image.py index 2b5ad820..bc606dd3 100644 --- a/bec_widgets/widgets/plots/image/image.py +++ b/bec_widgets/widgets/plots/image/image.py @@ -7,7 +7,6 @@ import numpy as np from bec_lib import bec_logger from bec_lib.endpoints import MessageEndpoints from pydantic import BaseModel, Field, field_validator -from qtpy.QtCore import QTimer from qtpy.QtWidgets import QWidget from bec_widgets.utils.bec_connector import ConnectionConfig @@ -172,12 +171,12 @@ class Image(ImageBase): ] ) - QTimer.singleShot(0, self._adjust_and_connect) + self._call_later(0, self._adjust_and_connect) def _adjust_and_connect(self): """ Sync the device selection toolbar with current properties. - Has to be done with QTimer.singleShot to ensure the UI is fully initialized, needed for testing. + Has to be deferred to the event loop to ensure the UI is fully initialized, needed for testing. Note: DeviceComboBox and SignalComboBox auto-populate themselves, no manual population needed. """ diff --git a/bec_widgets/widgets/plots/motor_map/motor_map.py b/bec_widgets/widgets/plots/motor_map/motor_map.py index 300b2e54..1efa16fd 100644 --- a/bec_widgets/widgets/plots/motor_map/motor_map.py +++ b/bec_widgets/widgets/plots/motor_map/motor_map.py @@ -854,6 +854,20 @@ class MotorMap(PlotBase): data = {"x": self._buffer["x"], "y": self._buffer["y"]} return data + ################################################################################ + # Cleanup + ################################################################################ + def cleanup(self): + """ + Cleanup the widget by detaching the plot update proxy and closing the settings popup. + """ + # Detach the update proxy first, so no queued update reaches the closed plot. + self.proxy_update_plot.disconnect() + if self.motor_map_settings is not None: + self.motor_map_settings.reject() + self.motor_map_settings = None + super().cleanup() + class DemoApp(QMainWindow): # pragma: no cover def __init__(self): diff --git a/bec_widgets/widgets/plots/plot_base.py b/bec_widgets/widgets/plots/plot_base.py index 6c026073..e5a12692 100644 --- a/bec_widgets/widgets/plots/plot_base.py +++ b/bec_widgets/widgets/plots/plot_base.py @@ -1,6 +1,6 @@ from __future__ import annotations -from collections.abc import Iterable, Mapping +from collections.abc import Callable, Iterable, Mapping from enum import Enum from typing import Literal, TypeAlias @@ -9,7 +9,7 @@ import pyqtgraph as pg from bec_lib import bec_logger from bec_lib.scan_data_container import ScanDataContainer from bec_lib.scan_items import ScanItem -from qtpy.QtCore import QPoint, QPointF, Qt, Signal +from qtpy.QtCore import QPoint, QPointF, Qt, QTimer, Signal from qtpy.QtWidgets import QHBoxLayout, QLabel, QMainWindow, QVBoxLayout, QWidget from bec_widgets.utils.bec_connector import ConnectionConfig @@ -1375,6 +1375,27 @@ class PlotBase(BECWidget, QWidget): self.crosshair.clear_markers() self.crosshair.update_markers() + def _call_later(self, msec: int, callback: Callable[[], object]) -> None: + """ + Call ``callback`` once after ``msec`` milliseconds, but only while the widget is in use. + + Use this instead of a bare ``QTimer.singleShot`` for deferred work that touches the + widget: the timer is bound to the widget, so the call is dropped if the widget is + deleted first, and it is skipped if the widget has been closed (cleaned up) but not + yet deleted. + + Args: + msec(int): Delay in milliseconds. + callback(Callable[[], object]): Called without arguments. + """ + + def _call_unless_cleaned_up(): + if self._destroyed: + return + callback() + + QTimer.singleShot(msec, self, _call_unless_cleaned_up) + def cleanup(self): self.toolbar.cleanup() self.unhook_crosshair(preserve_pin=False) diff --git a/bec_widgets/widgets/plots/scatter_waveform/scatter_waveform.py b/bec_widgets/widgets/plots/scatter_waveform/scatter_waveform.py index 5f1a60d9..9038e663 100644 --- a/bec_widgets/widgets/plots/scatter_waveform/scatter_waveform.py +++ b/bec_widgets/widgets/plots/scatter_waveform/scatter_waveform.py @@ -5,7 +5,7 @@ import pyqtgraph as pg from bec_lib import bec_logger from bec_lib.endpoints import MessageEndpoints from pydantic import Field, ValidationError, field_validator -from qtpy.QtCore import QTimer, Signal +from qtpy.QtCore import Signal from qtpy.QtWidgets import QHBoxLayout, QMainWindow, QWidget from bec_widgets.utils.bec_connector import ConnectionConfig @@ -335,8 +335,9 @@ class ScatterWaveform(PlotBase): self.sync_signal_update.emit() status = msg.get("done") if status: - QTimer.singleShot(100, self.update_sync_curves) - QTimer.singleShot(300, self.update_sync_curves) + # Final refreshes for late scan data; skipped if the widget is closed meanwhile. + self._call_later(100, self.update_sync_curves) + self._call_later(300, self.update_sync_curves) @SafeSlot() def update_sync_curves(self, _=None): diff --git a/bec_widgets/widgets/plots/waveform/waveform.py b/bec_widgets/widgets/plots/waveform/waveform.py index 68439ddd..b6aef85b 100644 --- a/bec_widgets/widgets/plots/waveform/waveform.py +++ b/bec_widgets/widgets/plots/waveform/waveform.py @@ -1629,8 +1629,9 @@ class Waveform(PlotBase): self.sync_signal_update.emit() self._scan_done = msg.get("done") if self._scan_done: - QTimer.singleShot(100, self.update_sync_curves) - QTimer.singleShot(300, self.update_sync_curves) + # Final refreshes for late scan data; skipped if the widget is closed meanwhile. + self._call_later(100, self.update_sync_curves) + self._call_later(300, self.update_sync_curves) def _fetch_scan_data_and_access(self) -> tuple[dict, str] | tuple[None, None]: """ diff --git a/bec_widgets/widgets/services/device_browser/device_browser.py b/bec_widgets/widgets/services/device_browser/device_browser.py index d0fc5af9..83508380 100644 --- a/bec_widgets/widgets/services/device_browser/device_browser.py +++ b/bec_widgets/widgets/services/device_browser/device_browser.py @@ -199,6 +199,8 @@ class DeviceBrowser(BECWidget, QWidget): self._config_helper.save_current_session(file_path) def cleanup(self): + # Detach the filter proxy first, so no queued filter change reaches the closed browser. + self.proxy_device_update.disconnect() super().cleanup() self.bec_dispatcher.client.callbacks.remove(self._scan_status_callback_id) self.bec_dispatcher.client.callbacks.remove(self._device_update_callback_id) diff --git a/tests/unit_tests/test_crosshair.py b/tests/unit_tests/test_crosshair.py index 02d164a5..f229c7ed 100644 --- a/tests/unit_tests/test_crosshair.py +++ b/tests/unit_tests/test_crosshair.py @@ -1,3 +1,5 @@ +import warnings + import numpy as np import pyqtgraph as pg import pytest @@ -643,3 +645,38 @@ def test_reset_preserves_pin_cleanup_removes_it(image_widget_with_crosshair): crosshair.cleanup() assert crosshair.pinned_point is None assert crosshair.pinned_pos is None + + +def test_cleanup_detaches_crosshair_from_scene_mouse_signals(qtbot, plot_widget_with_crosshair): + """ + The plot's scene outlives the crosshair. After cleanup() neither mouse moves (through + the rate-limited SignalProxy) nor clicks on the scene may reach the crosshair, e.g. + re-creating a pin on the plot after teardown. + """ + crosshair, plot_item = plot_widget_with_crosshair + scene = plot_item.scene() + scene_pos = plot_item.vb.mapViewToScene(QPointF(2, 5)) + moved, clicked = [], [] + crosshair.positionChanged.connect(moved.append) + crosshair.positionClicked.connect(clicked.append) + + # Sanity check: while active, both scene signals reach the crosshair. + scene.sigMouseMoved.emit(scene_pos) + scene.sigMouseClicked.emit(_FakeClickEvent(scene_pos)) + qtbot.waitUntil(lambda: bool(moved and clicked), timeout=1000) + assert crosshair.pinned_point is not None + + crosshair.cleanup() + moved.clear() + clicked.clear() + scene.sigMouseMoved.emit(scene_pos) + scene.sigMouseClicked.emit(_FakeClickEvent(scene_pos)) + qtbot.wait(100) # well past the proxy's 60 Hz rate limit + + assert moved == [] + assert clicked == [] + assert crosshair.pinned_point is None + + with warnings.catch_warnings(): + warnings.simplefilter("error", RuntimeWarning) # e.g. libpyside "Failed to disconnect" + crosshair.cleanup() # a second cleanup() leaves the scene connections alone diff --git a/tests/unit_tests/test_device_browser.py b/tests/unit_tests/test_device_browser.py index c3d81b8e..82e707e0 100644 --- a/tests/unit_tests/test_device_browser.py +++ b/tests/unit_tests/test_device_browser.py @@ -181,3 +181,16 @@ def test_signal_display_omitted_not_added(mocked_client, qtbot): qtbot.addWidget(signal_display) assert signal_display._content_layout.itemAt(1).widget() is None + + +def test_device_browser_cleanup_detaches_filter_proxy(device_browser: DeviceBrowser, qtbot): + """ + After cleanup() the rate-limited filter proxy must no longer forward filter_input + changes to update_device_list of the closed browser. + """ + with mock.patch.object(device_browser.dev_list, "update_filter") as update_filter: + device_browser.close() # closeEvent -> cleanup() + device_browser.ui.filter_input.setText("samx") + + qtbot.wait(600) + update_filter.assert_not_called() diff --git a/tests/unit_tests/test_image_view_next_gen.py b/tests/unit_tests/test_image_view_next_gen.py index 4a41648a..1d4757cb 100644 --- a/tests/unit_tests/test_image_view_next_gen.py +++ b/tests/unit_tests/test_image_view_next_gen.py @@ -1,3 +1,5 @@ +from unittest import mock + import numpy as np import pyqtgraph as pg import pytest @@ -1676,3 +1678,25 @@ def test_layer_accessors_safe_after_teardown(qtbot, mocked_client): view.autorange_mode = "max" view._sync_autorange_switch() view._sync_colorbar_levels() + + +def test_deferred_toolbar_sync_skipped_after_close(qtbot, mocked_client): + """ + The toolbar sync that __init__ defers to the event loop must not run into an Image + that was closed (cleaned up) before the event loop got to it. + """ + with mock.patch.object(Image, "_sync_device_selection") as sync_device_selection: + view = Image(client=mocked_client) + qtbot.addWidget(view) + view.close() # closeEvent -> cleanup() before the deferred sync ran + + qtbot.wait(100) + sync_device_selection.assert_not_called() + + +def test_deferred_toolbar_sync_runs_while_open(qtbot, mocked_client): + """The toolbar sync deferred from __init__ still runs once for an open Image.""" + with mock.patch.object(Image, "_sync_device_selection") as sync_device_selection: + create_widget(qtbot, Image, client=mocked_client) + + qtbot.waitUntil(lambda: sync_device_selection.call_count == 1, timeout=1000) diff --git a/tests/unit_tests/test_motor_map_next_gen.py b/tests/unit_tests/test_motor_map_next_gen.py index ac8a75b2..5a9957ea 100644 --- a/tests/unit_tests/test_motor_map_next_gen.py +++ b/tests/unit_tests/test_motor_map_next_gen.py @@ -1,3 +1,6 @@ +from unittest import mock + +import shiboken6 from qtpy.QtTest import QSignalSpy from bec_widgets.tests.client_mocks import mocked_client @@ -380,3 +383,41 @@ def test_motor_map_settings_dialog(qtbot, mocked_client): mm.motor_map_settings.close() qtbot.wait(200) assert mm.motor_map_settings is None + + +def test_motor_map_cleanup_detaches_update_proxy(qtbot, mocked_client): + """ + A plot update still queued in proxy_update_plot when the MotorMap is cleaned up, or + requested afterwards, must not reach _update_plot of the closed widget. + """ + with mock.patch.object(MotorMap, "_update_plot") as update_plot: + mm = create_widget(qtbot, MotorMap, client=mocked_client) + qtbot.wait(100) # let anything queued during construction drain + update_plot.reset_mock() + + mm.update_signal.emit() # queued in the proxy + mm.close() # closeEvent -> cleanup() + mm.update_signal.emit() # requested after cleanup + + qtbot.wait(200) + update_plot.assert_not_called() + + +def test_motor_map_cleanup_closes_settings_popup(qtbot, mocked_client): + """ + Closing the MotorMap must also close its settings popup instead of leaving it open + on top of the removed widget. + """ + mm = create_widget(qtbot, MotorMap, client=mocked_client, popups=True) + mm.map(device_x="samx", device_y="samy") + mm.show_motor_map_settings() + dialog = mm.motor_map_settings + assert dialog.isVisible() + + mm.close() # closeEvent -> cleanup() + try: + assert mm.motor_map_settings is None + assert not dialog.isVisible() + finally: + if shiboken6.isValid(dialog) and dialog.isVisible(): + dialog.reject() # keep a failure on main from leaking the popup into later tests diff --git a/tests/unit_tests/test_plot_base_next_gen.py b/tests/unit_tests/test_plot_base_next_gen.py index 886e674a..2089028c 100644 --- a/tests/unit_tests/test_plot_base_next_gen.py +++ b/tests/unit_tests/test_plot_base_next_gen.py @@ -792,3 +792,20 @@ def test_limits_accept_fractional_values(qtbot, mocked_client): pb.y_limits = (-1.75, 3.5) assert (pb.x_limits.x(), pb.x_limits.y()) == (0.5, 9.25) assert (pb.y_limits.x(), pb.y_limits.y()) == (-1.75, 3.5) + + +def test_call_later_skips_callback_once_widget_is_closed(qtbot, mocked_client): + """ + PlotBase._call_later runs the deferred callback while the widget is open and skips it + when the widget has been closed (cleaned up) before the delay elapsed. + """ + open_pb = create_widget(qtbot, PlotBase, client=mocked_client) + closed_pb = create_widget(qtbot, PlotBase, client=mocked_client) + calls = [] + + open_pb._call_later(10, lambda: calls.append("open")) + closed_pb._call_later(10, lambda: calls.append("closed")) + closed_pb.close() # closeEvent -> cleanup() before the callback is due + + qtbot.wait(100) + assert calls == ["open"] diff --git a/tests/unit_tests/test_scatter_waveform.py b/tests/unit_tests/test_scatter_waveform.py index e1098e29..4f57f245 100644 --- a/tests/unit_tests/test_scatter_waveform.py +++ b/tests/unit_tests/test_scatter_waveform.py @@ -682,3 +682,37 @@ def test_scatter_waveform_cleanup_detaches_update_proxy(qtbot, mocked_client): qtbot.wait(200) update_sync_curves.assert_not_called() + + +def test_scatter_waveform_scan_done_runs_delayed_sync_refreshes(qtbot, mocked_client): + """ + A finished scan triggers one proxied sync update plus the two delayed final refreshes + (100 ms and 300 ms) while the ScatterWaveform is open. + """ + with patch.object(ScatterWaveform, "update_sync_curves") as update_sync_curves: + swf = create_widget(qtbot, ScatterWaveform, client=mocked_client) + qtbot.wait(100) # let anything queued during construction drain + update_sync_curves.reset_mock() + + swf.on_scan_progress({"done": True}, {}) + + qtbot.waitUntil(lambda: update_sync_curves.call_count == 3, timeout=2000) + qtbot.wait(100) + assert update_sync_curves.call_count == 3 + + +def test_scatter_waveform_scan_done_refreshes_skipped_after_cleanup(qtbot, mocked_client): + """ + The delayed final refreshes scheduled when a scan is done must not run into a + ScatterWaveform that has been closed (cleaned up) before they fire. + """ + with patch.object(ScatterWaveform, "update_sync_curves") as update_sync_curves: + swf = create_widget(qtbot, ScatterWaveform, client=mocked_client) + qtbot.wait(100) # let anything queued during construction drain + update_sync_curves.reset_mock() + + swf.on_scan_progress({"done": True}, {}) # schedules the refreshes in 100 ms and 300 ms + swf.close() # closeEvent -> cleanup() before they fire + + qtbot.wait(450) + update_sync_curves.assert_not_called() diff --git a/tests/unit_tests/test_waveform.py b/tests/unit_tests/test_waveform.py index ca6bf9ef..2dca9dcb 100644 --- a/tests/unit_tests/test_waveform.py +++ b/tests/unit_tests/test_waveform.py @@ -678,6 +678,34 @@ def test_waveform_cleanup_detaches_proxies_from_update_signals( } +def test_waveform_scan_done_runs_delayed_sync_refreshes(qtbot, waveform_with_proxy_slot_spies): + """ + A finished scan triggers one proxied sync update plus the two delayed final refreshes + (100 ms and 300 ms) while the Waveform is open. + """ + wf, slots = waveform_with_proxy_slot_spies + + wf.on_scan_progress({"done": True}, {}) + + qtbot.waitUntil(lambda: slots["sync"].call_count == 3, timeout=2000) + qtbot.wait(100) + assert slots["sync"].call_count == 3 + + +def test_waveform_scan_done_refreshes_skipped_after_cleanup(qtbot, waveform_with_proxy_slot_spies): + """ + The delayed final refreshes scheduled when a scan is done must not run into a Waveform + that has been closed (cleaned up) before they fire. + """ + wf, slots = waveform_with_proxy_slot_spies + + wf.on_scan_progress({"done": True}, {}) # schedules the refreshes in 100 ms and 300 ms + wf.close() # closeEvent -> cleanup() before they fire + + qtbot.wait(450) + assert slots["sync"].call_count == 0 + + def test_request_dap_always_resubmits_device_parent(qtbot, mocked_client_with_dap, monkeypatch): """ DAP curves attached to device curves keep the resubmit-on-every-update behavior,