diff --git a/src/instrumentserver/gui/base_instrument.py b/src/instrumentserver/gui/base_instrument.py index 2e28793..2f633e4 100644 --- a/src/instrumentserver/gui/base_instrument.py +++ b/src/instrumentserver/gui/base_instrument.py @@ -534,6 +534,15 @@ class InstrumentTreeViewBase(QtWidgets.QTreeView): #: emitted when this item got its star action triggered. itemStarToggle = QtCore.Signal(ItemBase) + #: Signal() + #: emitted when the user presses Return, Enter, F2, or Right on a parameter row to enter edit mode. + #: On a row with children (an instrument or submodule node) those keys expand/collapse the node instead. + editCurrentParameter = QtCore.Signal() + + #: Signal() + #: emitted when the user presses Backspace to clear the selected parameter's value. + clearCurrentParameter = QtCore.Signal() + def __init__( self, model: QtCore.QAbstractItemModel, @@ -586,6 +595,19 @@ def __init__( self.setContextMenuPolicy(QtCore.Qt.ContextMenuPolicy.CustomContextMenu) self.customContextMenuRequested.connect(self.onContextMenuRequested) + for key in ("Return", "Enter", "F2"): + sc = QtWidgets.QShortcut(QtGui.QKeySequence(key), self) + sc.setContext(QtCore.Qt.ShortcutContext.WidgetShortcut) + sc.activated.connect(self.onEditKeyPressed) + + sc = QtWidgets.QShortcut(QtGui.QKeySequence("Right"), self) + sc.setContext(QtCore.Qt.ShortcutContext.WidgetShortcut) + sc.activated.connect(self.onRightKeyPressed) + + sc = QtWidgets.QShortcut(QtGui.QKeySequence("Backspace"), self) + sc.setContext(QtCore.Qt.ShortcutContext.WidgetShortcut) + sc.activated.connect(self.clearCurrentParameter) + @QtCore.Slot() def fillCollapsedDict(self, parentItem: Optional[ItemBase] = None) -> None: """ @@ -749,6 +771,55 @@ def onContextMenuRequested(self, pos: QtCore.QPoint) -> None: self.contextMenu.exec_(self.mapToGlobal(pos)) + def _currentNodeIndex(self) -> Optional[QtCore.QModelIndex]: + """ + Returns the column-0 index of the current row if that row has children (i.e. it is an + instrument/submodule node rather than a parameter), otherwise None. + """ + current = self.currentIndex() + if not current.isValid(): + return None + idx0 = current.sibling(current.row(), 0) + if self.model().hasChildren(idx0): # type: ignore[union-attr] + return idx0 + return None + + @QtCore.Slot() + def onEditKeyPressed(self) -> None: + """ + Return/Enter/F2: on a node with children toggle expanded/collapsed, otherwise ask to edit the + current parameter's value. + """ + node = self._currentNodeIndex() + if node is not None: + self.setExpanded(node, not self.isExpanded(node)) + else: + self.editCurrentParameter.emit() + + @QtCore.Slot() + def onRightKeyPressed(self) -> None: + """ + Right: on a node with children behave like a normal tree (expand if collapsed, otherwise move to the + first child), otherwise ask to edit the current parameter's value. + """ + node = self._currentNodeIndex() + if node is not None: + if not self.isExpanded(node): + self.expand(node) + else: + self.setCurrentIndex(self.model().index(0, 0, node)) # type: ignore[union-attr] + else: + self.editCurrentParameter.emit() + + def focusNextPrevChild(self, next: bool) -> bool: + current = self.currentIndex() + if current.isValid(): + next_idx = self.indexBelow(current) if next else self.indexAbove(current) + if next_idx.isValid(): + self.setCurrentIndex(next_idx) + return True + return super().focusNextPrevChild(next) + @QtCore.Slot() def onStarActionTrigger(self) -> None: self.itemStarToggle.emit(self.lastSelectedItem) diff --git a/src/instrumentserver/gui/instruments.py b/src/instrumentserver/gui/instruments.py index b96805a..0df52b3 100644 --- a/src/instrumentserver/gui/instruments.py +++ b/src/instrumentserver/gui/instruments.py @@ -203,6 +203,10 @@ class MethodDisplay(QtWidgets.QWidget): #: emitted when the widget runs a function and is successful. Emits the return value as a string. runSuccessful = QtCore.Signal(str) + #: Signal() -- + #: emitted when the user commits via Return/Enter, regardless of success or failure + valueCommitted = QtCore.Signal() + def __init__( self, fun: Callable, @@ -254,6 +258,8 @@ def runFun(self) -> None: except Exception as e: self.runFailed.emit(str(e)) logger.warning(f"'{self.fullName}' Raised the following execution: {e}") + finally: + self.valueCommitted.emit() @classmethod def getTooltipFromFun(cls, fun: Callable) -> str: @@ -286,6 +292,7 @@ def __init__(self, parent: Optional[QtCore.QObject] = None) -> None: # Stores as key the name of the item and as value the widget that the delegate creates. # used to keep a reference to the widget. self.parameters: Dict[str, QtWidgets.QWidget] = {} + self.navFilter: Optional["ValueCellNavigationFilter"] = None def createEditor( # type: ignore[override] self, @@ -297,10 +304,24 @@ def createEditor( # type: ignore[override] This is the function that is supposed to create the widget. It should return it. """ item = self.getItem(index) + + if not item.showDelegate: # type: ignore[attr-defined] + return None # type: ignore[return-value] + element = item.element # type: ignore[attr-defined] ret = ParameterWidget(element, widget) self.parameters[item.name] = ret # type: ignore[attr-defined] + ret.valueCommitted.connect(self.parent().setFocus) # type: ignore[union-attr] + + if self.navFilter is not None: + if isinstance(ret.paramWidget, AnyInput): + input_widget = ret.paramWidget.input + else: + input_widget = ret.paramWidget + input_widget.installEventFilter(self.navFilter) + self.navFilter.registerWidget(input_widget, index) + # Try to fetch and display current value immediately # ---- Chao: removed because the constructor of ParameterWidget object already calls parameter get ---- # if element.gettable: @@ -312,6 +333,76 @@ def createEditor( # type: ignore[override] return ret +class ValueCellNavigationFilter(QtCore.QObject): + """Event filter installed on value input widgets to handle Escape, Tab, and Shift+Tab.""" + + def __init__(self, treeView: InstrumentTreeViewBase) -> None: + super().__init__(treeView) + self._treeView = treeView + self._widgetIndex: Dict[QtCore.QObject, QtCore.QPersistentModelIndex] = {} + + def registerWidget(self, widget: QtCore.QObject, index: QtCore.QModelIndex) -> None: + self._widgetIndex[widget] = QtCore.QPersistentModelIndex(index) + + def eventFilter(self, obj: QtCore.QObject, event: QtCore.QEvent) -> bool: # type: ignore[override] + if event.type() == QtCore.QEvent.Type.FocusIn: + if obj in self._widgetIndex: + idx = self._widgetIndex[obj] + if idx.isValid(): + self._treeView.setCurrentIndex(QtCore.QModelIndex(idx)) + return False + + if event.type() == QtCore.QEvent.Type.KeyPress: + assert isinstance(event, QtGui.QKeyEvent) + key = QtGui.QKeySequence(event.key()).toString() + + if key == "Esc": + host = self._findHostWidget(obj) + if isinstance(host, ParameterWidget): + host.setWidgetFromParameter() + elif isinstance(host, MethodDisplay): + host.anyInput.input.clear() + self._treeView.setFocus() + return True + + elif key == "Tab": + self._commitAndMove(obj, 1) + return True + + elif key == "Backtab": + self._commitAndMove(obj, -1) + return True + + return super().eventFilter(obj, event) + + def _findHostWidget( + self, obj: QtCore.QObject + ) -> Optional[Union[ParameterWidget, MethodDisplay]]: + parent = obj.parent() + while parent is not None: + if isinstance(parent, (ParameterWidget, MethodDisplay)): + return parent + parent = parent.parent() + return None + + def _commitAndMove(self, obj: QtCore.QObject, direction: int) -> None: + host = self._findHostWidget(obj) + if isinstance(host, ParameterWidget): + host.setButton.click() + elif isinstance(host, MethodDisplay): + host.runFun() + current = self._treeView.currentIndex() + if current.isValid(): + next_idx = ( + self._treeView.indexBelow(current) + if direction > 0 + else self._treeView.indexAbove(current) + ) + if next_idx.isValid(): + self._treeView.setCurrentIndex(next_idx) + self._treeView.setFocus() + + class ModelParameters(InstrumentModelBase): # : Signal(item, object) : Emitted when an item in the model has received a new value, first object is the item's # name, second object is its new value @@ -424,6 +515,7 @@ def __init__( super().__init__(model, [2], *args, **kwargs) self.delegate = ParameterDelegate(self) + self.delegate.navFilter = ValueCellNavigationFilter(self) self.setItemDelegateForColumn(2, self.delegate) self.setAllDelegatesPersistent() @@ -482,11 +574,14 @@ def __init__( def connectSignals(self) -> None: super().connectSignals() self.model.itemNewValue.connect(self.view.onItemNewValue) + + self.view.editCurrentParameter.connect(self._focusToParameterValue) + self.view.clearCurrentParameter.connect(self._clearCurrentParameter) + self.shortcutManager.register("refresh_item", self._refreshCurrentItem, self) self.shortcutManager.register( "toggle_python", self._togglePythonCurrentItem, self ) - self.shortcutManager.register("edit_value", self._focusToParameterValue, self) def _withCurrentParameter( self, callback: Callable[["ParameterWidget"], None] @@ -517,10 +612,27 @@ def _focusToParameterValue(self) -> None: lambda w: ( w.paramWidget.input.setFocus() if isinstance(w.paramWidget, AnyInput) + else None + if isinstance(w.paramWidget, QtWidgets.QLabel) else w.paramWidget.setFocus() ) ) + @QtCore.Slot() + def _clearCurrentParameter(self) -> None: + self._withCurrentParameter( + lambda w: ( + w.paramWidget.input.clear() + if isinstance(w.paramWidget, AnyInput) + # read-only parameters are shown in a QLabel; clearing it would blank the display + else None + if isinstance(w.paramWidget, QtWidgets.QLabel) + else w.paramWidget.clear() + if hasattr(w.paramWidget, "clear") + else None + ) + ) + # ----------------- Parameters Display Classes - Ending -------------------------------- @@ -539,11 +651,24 @@ def createEditor( # type: ignore[override] index: QtCore.QModelIndex, ) -> QtWidgets.QWidget: item = self.getItem(index) + + if not item.showDelegate: # type: ignore[attr-defined] + return None # type: ignore[return-value] + element = item.element # type: ignore[attr-defined] rw = self.makeRemoveWidget(item.name, widget) # type: ignore[attr-defined] ret = ParameterWidget(parameter=element, parent=widget, additionalWidgets=[rw]) self.parameters[item.name] = ret # type: ignore[attr-defined] + ret.valueCommitted.connect(self.parent().setFocus) # type: ignore[union-attr] + + if self.navFilter is not None: + if isinstance(ret.paramWidget, AnyInput): + input_widget = ret.paramWidget.input + else: + input_widget = ret.paramWidget + input_widget.installEventFilter(self.navFilter) + self.navFilter.registerWidget(input_widget, index) return ret @@ -572,6 +697,7 @@ def __init__( super().__init__(model, [2], *args, **kwargs) self.delegate = ParameterDeleteDelegate(self) + self.delegate.navFilter = ValueCellNavigationFilter(self) self.setItemDelegateForColumn(2, self.delegate) self.setAllDelegatesPersistent() @@ -769,6 +895,7 @@ def __init__(self, parent: Optional[QtCore.QObject] = None) -> None: super().__init__(parent=parent) self.methods: Dict[str, "MethodDisplay"] = {} + self.navFilter: Optional[ValueCellNavigationFilter] = None def createEditor( # type: ignore[override] self, @@ -777,6 +904,10 @@ def createEditor( # type: ignore[override] index: QtCore.QModelIndex, ) -> QtWidgets.QWidget: item = self.getItem(index) + + if not item.showDelegate: # type: ignore[attr-defined] + return None # type: ignore[return-value] + element = item.element # type: ignore[attr-defined] ret = MethodDisplay(element, item.name, parent=widget) # type: ignore[attr-defined] @@ -786,6 +917,12 @@ def createEditor( # type: ignore[override] parent.clearAlertsAction.triggered.connect(ret.alertLabel.clearAlert) # type: ignore[union-attr] self.methods[item.name] = ret # type: ignore[attr-defined] + ret.valueCommitted.connect(self.parent().setFocus) # type: ignore[union-attr] + + if self.navFilter is not None: + ret.anyInput.input.installEventFilter(self.navFilter) + self.navFilter.registerWidget(ret.anyInput.input, index) + return ret @@ -804,6 +941,7 @@ def __init__( self.contextMenu.addAction(self.clearAlertsAction) self.delegate = MethodsDelegate(self) + self.delegate.navFilter = ValueCellNavigationFilter(self) self.setItemDelegateForColumn(1, self.delegate) self.setAllDelegatesPersistent() @@ -834,10 +972,13 @@ def __init__(self, instrument: Any, **kwargs: Any) -> None: def connectSignals(self) -> None: super().connectSignals() + + self.view.editCurrentParameter.connect(self._focusToMethodValue) + self.view.clearCurrentParameter.connect(self._clearCurrentMethod) + self.shortcutManager.register( "toggle_python", self._togglePythonCurrentItem, self ) - self.shortcutManager.register("edit_value", self._focusToMethodValue, self) self.shortcutManager.register("run_method", self._runCurrentMethod, self) def _withCurrentMethod(self, callback: Callable[["MethodDisplay"], None]) -> None: @@ -855,6 +996,10 @@ def _togglePythonCurrentItem(self) -> None: def _focusToMethodValue(self) -> None: self._withCurrentMethod(lambda w: w.anyInput.input.setFocus()) + @QtCore.Slot() + def _clearCurrentMethod(self) -> None: + self._withCurrentMethod(lambda w: w.anyInput.input.clear()) + @QtCore.Slot() def _runCurrentMethod(self) -> None: self._withCurrentMethod(lambda w: w.runFun()) diff --git a/src/instrumentserver/gui/parameters.py b/src/instrumentserver/gui/parameters.py index c1d7f15..fe5f4e8 100644 --- a/src/instrumentserver/gui/parameters.py +++ b/src/instrumentserver/gui/parameters.py @@ -49,6 +49,10 @@ class ParameterWidget(QtWidgets.QWidget): #: Signal(Any) -- _valueFromWidget = QtCore.Signal(object) + #: Signal() -- + #: emitted when the user commits a value via Return/Enter + valueCommitted = QtCore.Signal() + def __init__( self, parameter: Parameter, @@ -181,7 +185,7 @@ def onReturnPressed(self) -> None: """Activates the setButton when the input is selected and enter is pressed.""" self.setButton.click() self.paramWidget.input.deselect() - self.setButton.setFocus() + self.valueCommitted.emit() def setParameter(self, value: Any) -> None: try: diff --git a/src/instrumentserver/gui/shortcuts.py b/src/instrumentserver/gui/shortcuts.py index c528a6a..77c8096 100644 --- a/src/instrumentserver/gui/shortcuts.py +++ b/src/instrumentserver/gui/shortcuts.py @@ -43,7 +43,6 @@ class KeyboardShortcutManager: "save_items": ("Ctrl+Shift+S", "Save parameters to JSON file"), "fit_column": ("Ctrl+Shift+D", "Fits column width"), "sort_column": ("Ctrl+D", "Toggle sorting of selected column"), - "edit_value": ("Right", "Jump cursor to value field for selected parameter"), } def __init__(self) -> None: diff --git a/src/instrumentserver/server/application.py b/src/instrumentserver/server/application.py index 608082b..1fb4358 100644 --- a/src/instrumentserver/server/application.py +++ b/src/instrumentserver/server/application.py @@ -632,8 +632,8 @@ def __init__( ) # A test client, just a simple helper object. - self.client = EmbeddedClient(raise_exceptions=False, timeout=5000) - self.client.recv_timeout_ms = 10_000 + # timeout is in seconds; it configures the socket's receive timeout when connecting. + self.client = EmbeddedClient(raise_exceptions=False, timeout=10) # Central widget is simply a tab container. self.tabs = DetachableTabWidget(self) diff --git a/test/pytest/test_gui_navigation.py b/test/pytest/test_gui_navigation.py new file mode 100644 index 0000000..e620882 --- /dev/null +++ b/test/pytest/test_gui_navigation.py @@ -0,0 +1,165 @@ +"""Keyboard navigation in the instrument parameter tree (PR #145).""" + +from instrumentserver import QtCore, QtWidgets +from instrumentserver.server.application import startServerGuiApplication + + +def _shutdown_server_window(qtbot, window): + """Trigger closeEvent and wait for the server thread to exit so the next + test can bind the same port again. + + The server's ``finished`` signal is delivered to ``QThread.quit`` through the + main-thread event loop, so we must keep processing events while waiting + instead of blocking in ``QThread.wait``. + """ + try: + window.close() + except Exception: + pass + thread = getattr(window, "stationServerThread", None) + if thread is not None: + qtbot.waitUntil(lambda: not thread.isRunning(), timeout=10000) + + +# Use a spare port so these tests never talk to a developer's live server on 5555. +TEST_PORT = 5599 + +TIMEOUT_INS = ( + "instrumentserver.testing.dummy_instruments.generic.DummyInstrumentTimeout" +) + + +def _start_window(qtbot): + """Create the server window and wait until its embedded client points at the + test server. + + The embedded client connects to the default port when it is constructed and + only re-targets the real server port once the event loop delivers the + server-started signal, so the first request must not be sent before then. + """ + window = startServerGuiApplication(port=TEST_PORT) + qtbot.addWidget(window) + qtbot.waitUntil(lambda: window.client.addr.endswith(f":{TEST_PORT}"), timeout=10000) + return window + + +def _open_instrument_tab(window, name, cls): + window.client.find_or_create_instrument(name, cls) + window.refreshStationAction.trigger() + item = window.stationList.findItems( + name, QtCore.Qt.MatchExactly | QtCore.Qt.MatchRecursive, 0 + ) + window.addInstrumentTab(item[0], 0) + return window.instrumentTabsOpen[name] + + +def _find_row(view, text): + model = view.model() + matches = model.match( + model.index(0, 0), QtCore.Qt.DisplayRole, text, 1, QtCore.Qt.MatchRecursive + ) + assert matches, f"row {text!r} not found" + return matches[0] + + +def test_backspace_does_not_blank_read_only_parameter(qtbot): + window = _start_window(qtbot) + try: + tab = _open_instrument_tab(window, "timeout", TIMEOUT_INS) + params = tab.parametersList + view = params.view + + widget = params.view.delegate.parameters["random_int"] + label = widget.paramWidget + assert isinstance(label, QtWidgets.QLabel) + before = label.text() + assert before != "" + + view.setCurrentIndex(_find_row(view, "random_int")) + # Drive the slot the Backspace shortcut is wired to. + view.clearCurrentParameter.emit() + + assert label.text() == before + finally: + _shutdown_server_window(qtbot, window) + + +def test_backspace_clears_editable_parameter(qtbot): + window = _start_window(qtbot) + try: + tab = _open_instrument_tab(window, "timeout", TIMEOUT_INS) + params = tab.parametersList + view = params.view + + widget = params.view.delegate.parameters["param1"] + # param1 has no validator, so it is shown in an AnyInput wrapping a QLineEdit + line_edit = widget.paramWidget.input + assert line_edit.text() != "" + + view.setCurrentIndex(_find_row(view, "param1")) + view.clearCurrentParameter.emit() + + assert line_edit.text() == "" + finally: + _shutdown_server_window(qtbot, window) + + +SUBMODULE_INS = ( + "instrumentserver.testing.dummy_instruments.generic.DummyInstrumentWithSubmodule" +) + + +def test_enter_toggles_node_with_children(qtbot): + window = _start_window(qtbot) + try: + tab = _open_instrument_tab(window, "dummy", SUBMODULE_INS) + view = tab.parametersList.view + + node = _find_row(view, "A") + assert view.model().hasChildren(node) + assert view.isExpanded(node) # trees start fully expanded + + view.setCurrentIndex(node) + edits = [] + view.editCurrentParameter.connect(lambda: edits.append(1)) + + view.onEditKeyPressed() + assert not view.isExpanded(node) + view.onEditKeyPressed() + assert view.isExpanded(node) + assert edits == [] # a node never asks to edit a value + finally: + _shutdown_server_window(qtbot, window) + + +def test_right_expands_node_then_moves_to_first_child(qtbot): + window = _start_window(qtbot) + try: + tab = _open_instrument_tab(window, "dummy", SUBMODULE_INS) + view = tab.parametersList.view + + node = _find_row(view, "A") + view.collapse(node) + view.setCurrentIndex(node) + + view.onRightKeyPressed() + assert view.isExpanded(node) + assert view.currentIndex() == node + + view.onRightKeyPressed() + assert view.currentIndex() == view.model().index(0, 0, node) + finally: + _shutdown_server_window(qtbot, window) + + +def test_enter_on_parameter_requests_edit(qtbot): + window = _start_window(qtbot) + try: + tab = _open_instrument_tab(window, "dummy", SUBMODULE_INS) + view = tab.parametersList.view + + view.setCurrentIndex(_find_row(view, "param1")) + with qtbot.waitSignal(view.editCurrentParameter, timeout=1000): + view.onEditKeyPressed() + finally: + _shutdown_server_window(qtbot, window)