From 9c8800a8fa8215832d3c094ec07b5091b7154ec9 Mon Sep 17 00:00:00 2001 From: wyzula-jan Date: Fri, 16 Oct 2026 14:07:54 +0000 Subject: [PATCH] refactor(device_manager): share one SortTableItem with an optional sort key --- .../upload_redis_dialog.py | 40 ++++-------------- .../components/device_table/device_table.py | 42 ++++++++++++++----- .../test_device_manager_components.py | 39 +++++++++++++++++ tests/unit_tests/test_device_manager_view.py | 16 +++++++ 4 files changed, 94 insertions(+), 43 deletions(-) diff --git a/bec_widgets/applications/views/device_manager_view/device_manager_dialogs/upload_redis_dialog.py b/bec_widgets/applications/views/device_manager_view/device_manager_dialogs/upload_redis_dialog.py index a192a748..959cb740 100644 --- a/bec_widgets/applications/views/device_manager_view/device_manager_dialogs/upload_redis_dialog.py +++ b/bec_widgets/applications/views/device_manager_view/device_manager_dialogs/upload_redis_dialog.py @@ -12,6 +12,9 @@ from qtpy import QtCore, QtGui, QtWidgets from bec_widgets.utils.colors import get_accent_colors from bec_widgets.utils.error_popups import SafeSlot +from bec_widgets.widgets.control.device_manager.components.device_table.device_table import ( + SortTableItem, +) from bec_widgets.widgets.control.device_manager.components.ophyd_validation import ( ConfigStatus, ConnectionStatus, @@ -95,38 +98,9 @@ class DeviceStatusItem(QtWidgets.QWidget): self._update_display() -class SortTableItem(QtWidgets.QTableWidgetItem): - """Custom TableWidgetItem with hidden __column_data attribute for sorting.""" - - def __lt__(self, other: QtWidgets.QTableWidgetItem) -> bool: - """Override less-than operator for sorting.""" - if not isinstance(other, QtWidgets.QTableWidgetItem): - return NotImplemented - self_data = self.data(QtCore.Qt.ItemDataRole.UserRole) - other_data = other.data(QtCore.Qt.ItemDataRole.UserRole) - if self_data is not None and other_data is not None: - self_data: DeviceStatusItem - other_data: DeviceStatusItem - if self_data.config_status != other_data.config_status: - return self_data.config_status < other_data.config_status - else: - return self_data.connection_status < other_data.connection_status - return super().__lt__(other) - - def __gt__(self, other: QtWidgets.QTableWidgetItem) -> bool: - """Override less-than operator for sorting.""" - if not isinstance(other, QtWidgets.QTableWidgetItem): - return NotImplemented - self_data = self.data(QtCore.Qt.ItemDataRole.UserRole) - other_data = other.data(QtCore.Qt.ItemDataRole.UserRole) - if self_data is not None and other_data is not None: - self_data: DeviceStatusItem - other_data: DeviceStatusItem - if self_data.config_status != other_data.config_status: - return self_data.config_status > other_data.config_status - else: - return self_data.connection_status > other_data.connection_status - return super().__gt__(other) +def _status_sort_key(item: DeviceStatusItem) -> tuple[ConfigStatus, ConnectionStatus]: + """Sort device status items by config status first, then by connection status.""" + return item.config_status, item.connection_status class ValidationSection(QtWidgets.QGroupBox): @@ -180,7 +154,7 @@ class ValidationSection(QtWidgets.QGroupBox): else: row_position = self.table.rowCount() self.table.insertRow(row_position) - sort_item = SortTableItem(device_name) + sort_item = SortTableItem(device_name, sort_key=_status_sort_key) sort_item.setText("") self.table.setItem(row_position, 0, sort_item) device_item = DeviceStatusItem(device_config, config_status, connection_status) diff --git a/bec_widgets/widgets/control/device_manager/components/device_table/device_table.py b/bec_widgets/widgets/control/device_manager/components/device_table/device_table.py index 9d3b9a27..9cf4ac56 100644 --- a/bec_widgets/widgets/control/device_manager/components/device_table/device_table.py +++ b/bec_widgets/widgets/control/device_manager/components/device_table/device_table.py @@ -145,26 +145,48 @@ class CheckBoxDelegate(QtWidgets.QStyledItemDelegate): class SortTableItem(QtWidgets.QTableWidgetItem): - """Custom TableWidgetItem with hidden __column_data attribute for sorting.""" + """ + Table item that sorts by the value stored under the UserRole data role. + + Items without UserRole data fall back to the QTableWidgetItem comparison. + + Args: + *args: Positional arguments forwarded to QTableWidgetItem. + sort_key (Callable[[Any], Any] | None): Optional callable that maps the UserRole data + to the value that is compared. If None, the UserRole data is compared directly. + **kwargs: Keyword arguments forwarded to QTableWidgetItem. + """ + + def __init__(self, *args, sort_key: Callable[[Any], Any] | None = None, **kwargs) -> None: + super().__init__(*args, **kwargs) + self._sort_key = sort_key + + def _sort_values(self, other: QtWidgets.QTableWidgetItem) -> tuple[Any, Any] | None: + """Return the values to compare, or None if either item has no UserRole data.""" + self_data = self.data(QtCore.Qt.ItemDataRole.UserRole) + other_data = other.data(QtCore.Qt.ItemDataRole.UserRole) + if self_data is None or other_data is None: + return None + if self._sort_key is None: + return self_data, other_data + return self._sort_key(self_data), self._sort_key(other_data) def __lt__(self, other: QtWidgets.QTableWidgetItem) -> bool: """Override less-than operator for sorting.""" if not isinstance(other, QtWidgets.QTableWidgetItem): return NotImplemented - self_data = self.data(QtCore.Qt.ItemDataRole.UserRole) - other_data = other.data(QtCore.Qt.ItemDataRole.UserRole) - if self_data is not None and other_data is not None: - return self_data < other_data + values = self._sort_values(other) + if values is not None: + return values[0] < values[1] return super().__lt__(other) def __gt__(self, other: QtWidgets.QTableWidgetItem) -> bool: - """Override less-than operator for sorting.""" + """Override greater-than operator for sorting.""" if not isinstance(other, QtWidgets.QTableWidgetItem): return NotImplemented - self_data = self.data(QtCore.Qt.ItemDataRole.UserRole) - other_data = other.data(QtCore.Qt.ItemDataRole.UserRole) - if self_data is not None and other_data is not None: - return self_data > other_data + values = self._sort_values(other) + if values is not None: + return values[0] > values[1] return super().__gt__(other) diff --git a/tests/unit_tests/test_device_manager_components.py b/tests/unit_tests/test_device_manager_components.py index c2d04998..f7ddd3ac 100644 --- a/tests/unit_tests/test_device_manager_components.py +++ b/tests/unit_tests/test_device_manager_components.py @@ -37,6 +37,9 @@ from bec_widgets.widgets.control.device_manager.components.device_config_templat ReadoutPriorityComboBox, _try_literal_eval, ) +from bec_widgets.widgets.control.device_manager.components.device_table.device_table import ( + SortTableItem, +) from bec_widgets.widgets.control.device_manager.components.device_table.device_table_row import ( DeviceTableRow, ) @@ -295,6 +298,42 @@ class TestDeviceTableRow: ) +class TestSortTableItem: + """Test class for SortTableItem.""" + + @staticmethod + def _item(data=None, text: str = "", sort_key=None) -> SortTableItem: + item = SortTableItem(text, sort_key=sort_key) + if data is not None: + item.setData(QtCore.Qt.ItemDataRole.UserRole, data) + return item + + def test_compares_user_role_data(self): + """Without a sort key, the UserRole data is compared directly.""" + low, high = self._item(1, text="b"), self._item(2, text="a") + assert low < high + assert high > low + assert not high < low + + def test_sort_key(self): + """A sort key maps the UserRole data before comparing.""" + + def key(data: dict) -> tuple[int, int]: + return data["first"], data["second"] + + a = self._item({"first": 0, "second": 2}, sort_key=key) + b = self._item({"first": 1, "second": 0}, sort_key=key) + c = self._item({"first": 0, "second": 1}, sort_key=key) + assert a < b and b > a + assert c < a and a > c + + def test_non_item_comparison(self): + """Comparing with a non-item returns NotImplemented.""" + item = self._item(1) + assert item.__lt__(1) is NotImplemented + assert item.__gt__(1) is NotImplemented + + class TestDeviceTable: """Test class for DeviceTable component.""" diff --git a/tests/unit_tests/test_device_manager_view.py b/tests/unit_tests/test_device_manager_view.py index be934951..429eb024 100644 --- a/tests/unit_tests/test_device_manager_view.py +++ b/tests/unit_tests/test_device_manager_view.py @@ -372,6 +372,22 @@ class TestDeviceManagerViewDialogs: section.update_summary("2 devices validated, 1 failed.") assert section.summary_label.text() == "2 devices validated, 1 failed." + def test_validation_section_sorts_by_status(self, device_config: dict, qtbot): + """Rows sort by config status first, then by connection status.""" + section = ValidationSection(title="Validation Results") + qtbot.addWidget(section) + for name, config_status, connection_status in ( + ("dev_a", 1, 0), + ("dev_b", 0, 2), + ("dev_c", 0, 1), + ): + config = device_config.copy() + config["name"] = name + section.add_device(config, config_status, connection_status) + + names = [section.table.cellWidget(row, 0).device_name for row in range(3)] + assert names == ["dev_c", "dev_b", "dev_a"] + @pytest.fixture def device_configs_valid(self, device_config: dict): """Fixture for multiple device configurations."""