From bf793fb72dbf8426e3b26b3ca34c884177bd04bb Mon Sep 17 00:00:00 2001 From: wyzula-jan Date: Sat, 3 Oct 2026 16:28:24 +0200 Subject: [PATCH] fix(dict_backed_table): follow the Qt item model update rules --- .../widgets/editors/dict_backed_table.py | 48 +++-- .../test_dict_backed_table_model.py | 187 ++++++++++++++++++ 2 files changed, 220 insertions(+), 15 deletions(-) create mode 100644 tests/unit_tests/test_dict_backed_table_model.py diff --git a/bec_widgets/widgets/editors/dict_backed_table.py b/bec_widgets/widgets/editors/dict_backed_table.py index a9fb0644..9ab0e301 100644 --- a/bec_widgets/widgets/editors/dict_backed_table.py +++ b/bec_widgets/widgets/editors/dict_backed_table.py @@ -28,7 +28,8 @@ class DictBackedTableModel(QAbstractTableModel): Args: data (list[list[str]]): list of key-value pairs to initialise with""" super().__init__() - self._data: list[list[str]] = data + # copy, so that rows are never added or removed behind the model's back + self._data: list[list[str]] = [list(row) for row in data] self._default = _NOT_SET self._disallowed_keys: list[str] = [] @@ -70,10 +71,13 @@ class DictBackedTableModel(QAbstractTableModel): return False def replaceData(self, data: dict): - self.delete_rows(list(range(len(self._data)))) - self.resetInternalData() + """Replace the whole content of the table. + + Args: + data (dict): key-value pairs to display instead of the current rows.""" + self.beginResetModel() self._data = [[str(k), str(v)] for k, v in data.items()] - self.dataChanged.emit(self.index(0, 0), self.index(len(self._data), 1)) + self.endResetModel() def update_disallowed_keys(self, keys: list[str]): """Set the list of keys which may not be used. @@ -89,21 +93,23 @@ class DictBackedTableModel(QAbstractTableModel): def _other_keys(self, row: int): return [r[0] for r in self._data[:row] + self._data[row + 1 :]] - def flags(self, _): + def flags(self, index): + if not index.isValid(): + return Qt.ItemFlag.NoItemFlags return Qt.ItemFlag.ItemIsSelectable | Qt.ItemFlag.ItemIsEnabled | Qt.ItemFlag.ItemIsEditable - def insertRows(self, row, number, index): - """We only support adding one at a time for now""" - if row != self.rowCount() or number != 1: + def insertRows(self, row, number, index=QModelIndex()): + """We only support appending one row at a time for now""" + if index.isValid() or row != self.rowCount() or number != 1: return False - self.beginInsertRows(QModelIndex(), 0, 0) + self.beginInsertRows(QModelIndex(), row, row) self._data.append(["", ""]) self.endInsertRows() return True - def removeRows(self, row, number, index): + def removeRows(self, row, number, index=QModelIndex()): """This can only be consecutive, so instead of trying to be clever, only support removing one at a time""" - if number != 1: + if index.isValid() or number != 1 or not 0 <= row < self.rowCount(): return False self.beginRemoveRows(QModelIndex(), row, row) del self._data[row] @@ -112,14 +118,17 @@ class DictBackedTableModel(QAbstractTableModel): @SafeSlot() def add_row(self): + """Append an empty row; views are notified through rowsInserted.""" self.insertRow(self.rowCount()) - self.dataChanged.emit(self.index(self.rowCount(), 0), self.index(self.rowCount(), 1), 0) @SafeSlot(list) def delete_rows(self, rows: list[int]): + """Remove the given rows; views are notified through rowsRemoved. + + Args: + rows (list[int]): indices of the rows to remove.""" # delete from the end so indices stay correct - for row in sorted(rows, reverse=True): - self.dataChanged.emit(self.index(row, 0), self.index(row, 1), 0) + for row in sorted(set(rows), reverse=True): self.removeRows(row, 1, QModelIndex()) def set_default(self, value: dict | None): @@ -191,7 +200,16 @@ class DictBackedTable(QWidget): self._remove_button.clicked.connect(self.delete_selected_rows) self.delete_rows.connect(self._table_model.delete_rows) - self._table_model.dataChanged.connect(lambda *_: self.data_changed.emit(self.dump_dict())) + # structural changes do not emit dataChanged, so listen for them separately + self._table_model.dataChanged.connect(self._emit_data_changed) + self._table_model.rowsInserted.connect(self._emit_data_changed) + self._table_model.rowsRemoved.connect(self._emit_data_changed) + self._table_model.modelReset.connect(self._emit_data_changed) + + @SafeSlot() + def _emit_data_changed(self, *_): + """Emit data_changed with the table content after any edit, insertion, removal or reset.""" + self.data_changed.emit(self.dump_dict()) def set_default(self, value: dict | None): self._table_model.set_default(value) diff --git a/tests/unit_tests/test_dict_backed_table_model.py b/tests/unit_tests/test_dict_backed_table_model.py new file mode 100644 index 00000000..4b0cf3e4 --- /dev/null +++ b/tests/unit_tests/test_dict_backed_table_model.py @@ -0,0 +1,187 @@ +"""Regression tests for DictBackedTableModel model notifications (bec_widgets#1319). + +Views and proxies attached to a QAbstractItemModel rely on the model announcing every +structural change at its real position and never emitting dataChanged for rows that do not +exist. Qt's QAbstractItemModelTester is used in Warning mode (never Fatal, which aborts the +process); its failures are collected from the Qt log. +""" + +# pylint: disable=missing-function-docstring +# pylint: disable=redefined-outer-name +# pylint: disable=protected-access + +import pytest +from qtpy.QtCore import QItemSelectionModel, QSortFilterProxyModel, Qt +from qtpy.QtTest import QAbstractItemModelTester + +from bec_widgets.tests.utils import create_widget +from bec_widgets.widgets.editors.dict_backed_table import DictBackedTable +from bec_widgets.widgets.editors.scan_metadata.scan_metadata import ScanMetadata + +INITIAL_DATA = [["key1", "value1"], ["key2", "value2"], ["key3", "value3"]] + + +class NotificationRecorder: + """Record a model's notifications as plain tuples, evaluated at emission time.""" + + def __init__(self, model): + self.model = model + self.inserted: list[tuple[bool, int, int]] = [] + self.removed: list[tuple[bool, int, int]] = [] + self.data_changed: list[tuple[bool, int, bool, int, int]] = [] + self.resets = 0 + model.rowsAboutToBeInserted.connect( + lambda parent, first, last: self.inserted.append((parent.isValid(), first, last)) + ) + model.rowsAboutToBeRemoved.connect( + lambda parent, first, last: self.removed.append((parent.isValid(), first, last)) + ) + model.dataChanged.connect(self._on_data_changed) + model.modelReset.connect(self._on_reset) + + def _on_data_changed(self, top_left, bottom_right, *_): + self.data_changed.append( + ( + top_left.isValid(), + top_left.row(), + bottom_right.isValid(), + bottom_right.row(), + self.model.rowCount(), + ) + ) + + def _on_reset(self): + self.resets += 1 + + def invalid_data_changed(self): + """dataChanged emissions with an invalid index or a row outside the model.""" + return [ + entry + for entry in self.data_changed + if not (entry[0] and entry[2] and 0 <= entry[1] <= entry[3] < entry[4]) + ] + + +def _model_rows(model) -> list[str]: + return [model.index(row, 0).data() for row in range(model.rowCount())] + + +def _tester_failures(qtlog) -> list[str]: + return [record.message for record in qtlog.records if record.message.startswith("FAIL!")] + + +@pytest.fixture +def table(qtbot): + return create_widget(qtbot, DictBackedTable, initial_data=[list(r) for r in INITIAL_DATA]) + + +@pytest.fixture +def recorder(table): + return NotificationRecorder(table._table_model) + + +@pytest.fixture +def emitted(table): + """Dicts emitted by DictBackedTable.data_changed.""" + received: list[dict] = [] + table.data_changed.connect(received.append) + return received + + +@pytest.fixture +def model_tester(table): + tester = QAbstractItemModelTester( + table._table_model, QAbstractItemModelTester.FailureReportingMode.Warning + ) + yield tester + tester.deleteLater() + + +def test_add_row_announces_insert_at_end(table, recorder, emitted, capfd): + table._add_button.click() + + assert table._table_model.rowCount() == 4 + assert recorder.inserted == [(False, 3, 3)] + assert recorder.invalid_data_changed() == [] + # passing 0 as the roles argument of dataChanged makes shiboken print conversion errors + assert "Cannot copy-convert" not in capfd.readouterr().err + assert table.dump_dict() == {"key1": "value1", "key2": "value2", "key3": "value3", "": ""} + assert emitted[-1] == table.dump_dict() + + +def test_delete_rows_does_not_touch_removed_rows(table, recorder, emitted): + selection = table._table_view.selectionModel() + selection.select(table._table_model.index(1, 0), QItemSelectionModel.SelectionFlag.Select) + table.delete_selected_rows() + + assert recorder.removed == [(False, 1, 1)] + assert recorder.data_changed == [] + assert table.dump_dict() == {"key1": "value1", "key3": "value3"} + # listeners must see the table after the removal, not before it + assert emitted[-1] == {"key1": "value1", "key3": "value3"} + + +@pytest.mark.parametrize("new_data", [{"a": "1", "b": "2"}, {}]) +def test_replace_data_announces_structural_change(table, recorder, emitted, new_data): + table.replace_data(new_data) + + assert recorder.invalid_data_changed() == [] + assert recorder.resets == 1 + assert table.dump_dict() == new_data + assert emitted[-1] == new_data + + +def test_proxy_model_stays_in_sync(table): + proxy = QSortFilterProxyModel() + proxy.setSourceModel(table._table_model) + proxy.sort(0, Qt.SortOrder.DescendingOrder) + + table._add_button.click() + assert _model_rows(proxy) == sorted(_model_rows(table._table_model), reverse=True) + + table.replace_data({"a": "1", "b": "2"}) + assert _model_rows(proxy) == ["b", "a"] + + table.clear() + assert _model_rows(proxy) == [] + proxy.setSourceModel(None) + + +def test_model_tester_reports_no_failures(table, model_tester, qtlog): + model = table._table_model + table._add_button.click() + model.setData(model.index(3, 0), "key4", Qt.ItemDataRole.EditRole) + table.update_disallowed_keys(["key2"]) + selection = table._table_view.selectionModel() + selection.select(model.index(0, 0), QItemSelectionModel.SelectionFlag.Select) + selection.select(model.index(2, 0), QItemSelectionModel.SelectionFlag.Select) + table.delete_selected_rows() + table.replace_data({"a": "1", "b": "2"}) + table.clear() + + assert _tester_failures(qtlog) == [] + + +def test_tables_do_not_share_backing_data(qtbot): + first = create_widget(qtbot, DictBackedTable) + second = create_widget(qtbot, DictBackedTable) + first._add_button.click() + + assert first._table_model.rowCount() == 1 + assert second._table_model.rowCount() == 0 + + +def test_scan_metadata_form_drops_deleted_extra_row(qtbot): + widget = create_widget(qtbot, ScanMetadata, initial_extras=[["k1", "v1"], ["k2", "v2"]]) + received: list[dict] = [] + widget.form_data_updated.connect(received.append) + table = widget._additional_metadata + + selection = table._table_view.selectionModel() + selection.select(table._table_model.index(0, 0), QItemSelectionModel.SelectionFlag.Select) + table.delete_selected_rows() + + assert table.dump_dict() == {"k2": "v2"} + assert received, "deleting a row must re-validate the form" + assert "k1" not in received[-1] + assert received[-1]["k2"] == "v2"