From 00f7f566b2b407e02fbcb91dc068ad3feb4a2a36 Mon Sep 17 00:00:00 2001 From: wyzula-jan Date: Sat, 3 Oct 2026 16:21:34 +0200 Subject: [PATCH] fix(curve_settings): edit curve copies and drop removed curves --- .../settings/curve_settings/curve_tree.py | 20 ++++- .../widgets/plots/waveform/waveform.py | 16 +++- tests/unit_tests/test_curve_settings.py | 74 ++++++++++++++++++- tests/unit_tests/test_waveform.py | 57 ++++++++++++++ 4 files changed, 162 insertions(+), 5 deletions(-) diff --git a/bec_widgets/widgets/plots/waveform/settings/curve_settings/curve_tree.py b/bec_widgets/widgets/plots/waveform/settings/curve_settings/curve_tree.py index a89cdc77..bb16d66b 100644 --- a/bec_widgets/widgets/plots/waveform/settings/curve_settings/curve_tree.py +++ b/bec_widgets/widgets/plots/waveform/settings/curve_settings/curve_tree.py @@ -737,7 +737,11 @@ class CurveTree(BECWidget, QWidget): self.tree.collapseItem(item) def refresh_from_waveform(self): - """Clear the tree and rebuild from the waveform's existing curves if any, else add sample rows.""" + """Clear the tree and rebuild from the waveform's existing curves if any, else add sample rows. + + Each row works on a deep copy of its curve's config, so edits stay staged in the tree + and only reach the waveform through :meth:`send_curve_json`. + """ if self.waveform is None: return self.tree.clear() @@ -746,10 +750,20 @@ class CurveTree(BECWidget, QWidget): top_curves = [c for c in self.waveform.curves if c.config.source in ("device", "history")] dap_curves = [c for c in self.waveform.curves if c.config.source == "dap"] for dev in top_curves: - dr = CurveRow(self.tree, parent_item=None, config=dev.config, device_manager=self.dev) + dr = CurveRow( + self.tree, + parent_item=None, + config=dev.config.model_copy(deep=True), + device_manager=self.dev, + ) for dap in dap_curves: if dap.config.parent_label == dev.config.label: - CurveRow(self.tree, parent_item=dr, config=dap.config, device_manager=self.dev) + CurveRow( + self.tree, + parent_item=dr, + config=dap.config.model_copy(deep=True), + device_manager=self.dev, + ) def cleanup(self): """Cleanup the widget.""" diff --git a/bec_widgets/widgets/plots/waveform/waveform.py b/bec_widgets/widgets/plots/waveform/waveform.py index a50b0050..65a38d68 100644 --- a/bec_widgets/widgets/plots/waveform/waveform.py +++ b/bec_widgets/widgets/plots/waveform/waveform.py @@ -1346,7 +1346,14 @@ class Waveform(PlotBase): return curve def _refresh_history_curves(self): - for curve in self._history_curves: + """ + Reload the data of all history curves from their scans, e.g. after an x-axis change. + + Curves whose source is no longer 'history' are dropped from the history tracking + instead of being filled with scan data they do not accept. + """ + self._history_curves = [c for c in self._history_curves if c.config.source == "history"] + for curve in list(self._history_curves): scan_item = self.get_history_scan_item( scan_id=curve.config.scan_id, scan_index=curve.config.scan_number ) @@ -1432,6 +1439,7 @@ class Waveform(PlotBase): """ curve_list = self.curves self._dap_curves = [] + self._history_curves = [] self._sync_curves = [] self._async_curves = [] for curve in curve_list: @@ -1518,6 +1526,12 @@ class Waveform(PlotBase): ) curve.rpc_register.remove_rpc(curve) + # Stop tracking the removed curve, so history refreshes and DAP requests skip it + if curve in self._history_curves: + self._history_curves.remove(curve) + if curve in self._dap_curves: + self._dap_curves.remove(curve) + # Remove itself from the DAP summary only for side panels if ( curve.config.source == "dap" diff --git a/tests/unit_tests/test_curve_settings.py b/tests/unit_tests/test_curve_settings.py index 29c832eb..0c2e1479 100644 --- a/tests/unit_tests/test_curve_settings.py +++ b/tests/unit_tests/test_curve_settings.py @@ -5,7 +5,12 @@ import pytest from bec_lib.scan_history import ScanHistory from qtpy.QtWidgets import QComboBox, QVBoxLayout -from bec_widgets.tests.client_mocks import dap_plugin_message, mocked_client, mocked_client_with_dap +from bec_widgets.tests.client_mocks import ( + dap_plugin_message, + inject_scan_history, + mocked_client, + mocked_client_with_dap, +) from bec_widgets.widgets.plots.waveform.settings.curve_settings.curve_setting import CurveSetting from bec_widgets.widgets.plots.waveform.settings.curve_settings.curve_tree import CurveTree from bec_widgets.widgets.plots.waveform.waveform import Waveform @@ -470,3 +475,70 @@ def test_export_data_history_curve(curve_tree_fixture, scan_history_factory): assert exported["scan_number"] == 2 assert exported["scan_id"] == "hid2" assert exported["label"] == "bpm4i-bpm4i-scan-2" + + +################################################## +# Staging of edits (issue #1314) +################################################## + + +@pytest.fixture +def history_curve_setting(qtbot, mocked_client, scan_history_factory): + """ + A Waveform showing one history curve of scan 1 (two scans in history) and a CurveSetting + targeting it. + """ + wf = create_widget(qtbot, Waveform, client=mocked_client) + history_msgs = inject_scan_history(wf, scan_history_factory, ("hist1", 1), ("hist2", 2)) + # The scan index combobox lists scans by number, so register those as well. + for msg in history_msgs: + wf.client.history._scan_numbers.append(msg.scan_number) + wf.x_mode = "index" + history_curve = wf.plot(device_y="bpm4i", signal_y="bpm4i", scan_id="hist1") + curve_setting = create_widget(qtbot, CurveSetting, parent=None, target_widget=wf) + curve_setting.mode_combo.setCurrentText("index") + return curve_setting, wf, history_curve + + +def test_switch_history_curve_to_live_keeps_history_refresh_working(history_curve_setting): + """ + Regression for #1314: switching a history curve to 'live' and applying must not leave a + device-source curve behind in the Waveform's history tracking. Previously the settings row + edited the live curve's config in place, so the removed history curve turned into a + device curve that the next history refresh tried to fill, raising + "Source device do not allow custom data setting." + """ + curve_setting, wf, history_curve = history_curve_setting + row = curve_setting.curve_manager.tree.topLevelItem(0) + assert row.scan_index_combo.scan_id == "hist1" + + row.scan_index_combo.set_scan_id(None) # select 'live' + curve_setting.accept_changes() + + # The next history refresh (x-axis change, Apply again, ...) must not raise. + wf._refresh_history_curves() + + assert [curve.config.source for curve in wf.curves] == ["device"] + assert wf._history_curves == [] + # The removed history curve kept its own configuration. + assert history_curve.config.source == "history" + assert history_curve.config.scan_id == "hist1" + + +def test_curve_settings_edits_do_not_touch_live_curve_config(history_curve_setting): + """ + Edits in the curve tree are staged on a copy of each curve's config: changing the color, + renormalizing colors or exporting the rows must leave the plotted curve untouched until + the Waveform applies the exported configuration (also #1304, Cancel keeps color edits). + """ + curve_setting, wf, history_curve = history_curve_setting + curve_manager = curve_setting.curve_manager + original_config = history_curve.config.model_dump() + + row = curve_manager.tree.topLevelItem(0) + curve_manager.renormalize_colors() + row.color_button.set_color("#123456") + row.scan_index_combo.set_scan_id(None) # select 'live' + curve_manager.export_all_curves() + + assert history_curve.config.model_dump() == original_config diff --git a/tests/unit_tests/test_waveform.py b/tests/unit_tests/test_waveform.py index 4466b23e..a5e99d54 100644 --- a/tests/unit_tests/test_waveform.py +++ b/tests/unit_tests/test_waveform.py @@ -1892,6 +1892,63 @@ def test_history_curve_x_modes_post_plot(qtbot, mocked_client, scan_history_fact assert c.config.current_x_mode == mode +@pytest.mark.parametrize("removal", ["remove_curve", "clear_all"]) +def test_removed_history_curve_leaves_history_tracking( + qtbot, mocked_client, scan_history_factory, removal +): + """ + Regression for #1314: a removed history curve must not stay in the list of curves + refreshed from the scan history. + """ + wf = create_widget(qtbot, Waveform, client=mocked_client) + inject_scan_history(wf, scan_history_factory, ("hist1", 1)) + wf.x_mode = "index" + c = wf.plot(device_y="bpm4i", signal_y="bpm4i", scan_id="hist1") + assert wf._history_curves == [c] + + if removal == "remove_curve": + wf.remove_curve(c.name()) + else: + wf.clear_all() + + assert wf._history_curves == [] + + +def test_history_refresh_skips_curve_that_is_no_longer_history( + qtbot, mocked_client, scan_history_factory +): + """ + Regression for #1314: a curve whose source is no longer 'history' is dropped from the + history refresh instead of raising "Source device do not allow custom data setting." + """ + wf = create_widget(qtbot, Waveform, client=mocked_client) + inject_scan_history(wf, scan_history_factory, ("hist1", 1)) + wf.x_mode = "index" + c = wf.plot(device_y="bpm4i", signal_y="bpm4i", scan_id="hist1") + c.config.source = "device" + c.config.scan_id = None + c.config.scan_number = None + + wf._refresh_history_curves() + + assert wf._history_curves == [] + + +def test_removed_dap_curve_leaves_dap_tracking(qtbot, mocked_client_with_dap): + """ + Removing a device curve also removes its DAP child; neither may stay tracked as a DAP curve. + """ + wf = create_widget(qtbot, Waveform, client=mocked_client_with_dap) + wf.plot(arg1="bpm4i", label="bpm4i-bpm4i") + dap_curve = wf.add_dap_curve(device_label="bpm4i-bpm4i", dap_name="GaussianModel") + assert wf._dap_curves == [dap_curve] + + wf.remove_curve("bpm4i-bpm4i") + + assert wf.curves == [] + assert wf._dap_curves == [] + + def test_history_curve_incompatible_x_mode_hides_curve(qtbot, mocked_client, scan_history_factory): """ Test that setting an x_mode not present in stored data hides the history curve.