From d0feb4fd44ef32a8e42c96f11eca955412a703c4 Mon Sep 17 00:00:00 2001 From: Piotr Rozyczko Date: Fri, 20 Mar 2026 11:50:43 +0100 Subject: [PATCH 1/2] undo_redo branch rebased to refactor_24 --- src/sas/qtgui/MainWindow/GuiManager.py | 88 +- src/sas/qtgui/MainWindow/UI/MainWindowUI.ui | 9 + .../Fitting/FittingPerspective.py | 6 + .../Perspectives/Fitting/FittingWidget.py | 357 +++++--- .../Perspectives/Fitting/MagnetismWidget.py | 17 + .../Perspectives/Fitting/OptionsWidget.py | 46 +- .../Fitting/PolydispersityWidget.py | 20 +- .../qtgui/Perspectives/Fitting/UndoRedo.py | 632 ++++++++++++++ .../UnitTesting/UndoRedoIntegrationTest.py | 733 +++++++++++++++++ .../Fitting/UnitTesting/UndoRedoTest.py | 775 ++++++++++++++++++ src/sas/qtgui/Perspectives/perspective.py | 8 + .../Preferences/GeneralPreferencesWidget.py | 33 + .../Utilities/Preferences/PreferencesPanel.py | 4 +- src/sas/system/config/config.py | 3 + 14 files changed, 2613 insertions(+), 118 deletions(-) create mode 100644 src/sas/qtgui/Perspectives/Fitting/UndoRedo.py create mode 100644 src/sas/qtgui/Perspectives/Fitting/UnitTesting/UndoRedoIntegrationTest.py create mode 100644 src/sas/qtgui/Perspectives/Fitting/UnitTesting/UndoRedoTest.py create mode 100644 src/sas/qtgui/Utilities/Preferences/GeneralPreferencesWidget.py diff --git a/src/sas/qtgui/MainWindow/GuiManager.py b/src/sas/qtgui/MainWindow/GuiManager.py index 5537daf9d7..f325cf7f60 100644 --- a/src/sas/qtgui/MainWindow/GuiManager.py +++ b/src/sas/qtgui/MainWindow/GuiManager.py @@ -107,6 +107,8 @@ def __init__(self, parent=None): # Currently displayed perspective self._current_perspective: Perspective | None = None self.loadedPerspectives: dict[str, Perspective] = {} + self._connected_undo_stack = None + self._connected_tabbed_perspective = None # Populate the main window with stuff self.addWidgets() @@ -403,6 +405,8 @@ def perspectiveChanged(self, new_perspective_name: str): Respond to change of the perspective signal """ + self._disconnect_undo_redo_hooks() + if new_perspective_name not in self.loadedPerspectives: keylist = ', '.join(self.loadedPerspectives.keys()) raise KeyError( @@ -493,6 +497,7 @@ def perspectiveChanged(self, new_perspective_name: str): # Set the current perspective to new one and show self._current_perspective = new_perspective self._current_perspective.show() + self._connect_undo_redo_hooks() def updatePerspective(self, data): """ @@ -690,8 +695,8 @@ def addTriggers(self): Trigger definitions for all menu/toolbar actions. """ # disable not yet fully implemented actions - self._workspace.actionUndo.setVisible(False) - self._workspace.actionRedo.setVisible(False) + self._workspace.actionUndo.setEnabled(False) + self._workspace.actionRedo.setEnabled(False) self._workspace.actionReset.setVisible(False) self._workspace.actionStartup_Settings.setVisible(False) #self._workspace.actionImage_Viewer.setVisible(False) @@ -889,12 +894,87 @@ def actionQuit(self): def actionUndo(self): """ """ - print("actionUndo TRIGGERED") + stack = self._active_undo_stack() + if stack is not None: + stack.undo() def actionRedo(self): """ """ - print("actionRedo TRIGGERED") + stack = self._active_undo_stack() + if stack is not None: + stack.redo() + + def _active_undo_stack(self): + """Return the undo stack for the active perspective, if available.""" + if self._current_perspective is None: + return None + return getattr(self._current_perspective, "undo_stack", None) + + def _disconnect_undo_redo_hooks(self): + """Disconnect temporary undo/redo signal hooks.""" + if self._connected_undo_stack is not None: + try: + self._connected_undo_stack.stackChanged.disconnect( + self._update_undo_redo_actions + ) + except (RuntimeError, TypeError): + pass + self._connected_undo_stack = None + + if self._connected_tabbed_perspective is not None: + try: + self._connected_tabbed_perspective.currentChanged.disconnect( + self._on_perspective_tab_changed + ) + except (RuntimeError, TypeError): + pass + self._connected_tabbed_perspective = None + + def _connect_undo_redo_hooks(self): + """Connect action refresh hooks for active perspective and stack.""" + perspective = self._current_perspective + if perspective is None: + self._update_undo_redo_actions() + return + + if hasattr(perspective, "currentChanged"): + try: + perspective.currentChanged.connect(self._on_perspective_tab_changed) + self._connected_tabbed_perspective = perspective + except (RuntimeError, TypeError): + self._connected_tabbed_perspective = None + + stack = self._active_undo_stack() + if stack is not None and hasattr(stack, "stackChanged"): + try: + stack.stackChanged.connect(self._update_undo_redo_actions) + self._connected_undo_stack = stack + except (RuntimeError, TypeError): + self._connected_undo_stack = None + + self._update_undo_redo_actions() + + def _on_perspective_tab_changed(self, *_): + """Rewire undo hooks when active tab changes (e.g., fitting tabs).""" + self._disconnect_undo_redo_hooks() + self._connect_undo_redo_hooks() + + def _update_undo_redo_actions(self): + """Refresh undo/redo enabled state and action tooltips.""" + stack = self._active_undo_stack() + + if stack is None: + self._workspace.actionUndo.setEnabled(False) + self._workspace.actionRedo.setEnabled(False) + self._workspace.actionUndo.setToolTip("Undo") + self._workspace.actionRedo.setToolTip("Redo") + return + + self._workspace.actionUndo.setEnabled(stack.can_undo()) + self._workspace.actionRedo.setEnabled(stack.can_redo()) + self._workspace.actionUndo.setToolTip(stack.undo_text()) + self._workspace.actionRedo.setToolTip(stack.redo_text()) def actionCopy(self): """ diff --git a/src/sas/qtgui/MainWindow/UI/MainWindowUI.ui b/src/sas/qtgui/MainWindow/UI/MainWindowUI.ui index 04c12aad30..1137303dbf 100755 --- a/src/sas/qtgui/MainWindow/UI/MainWindowUI.ui +++ b/src/sas/qtgui/MainWindow/UI/MainWindowUI.ui @@ -260,6 +260,12 @@ Undo + + Undo + + + Ctrl+Z + @@ -272,6 +278,9 @@ Redo + + Ctrl+Y + diff --git a/src/sas/qtgui/Perspectives/Fitting/FittingPerspective.py b/src/sas/qtgui/Perspectives/Fitting/FittingPerspective.py index f81ce04d96..9a216940cf 100644 --- a/src/sas/qtgui/Perspectives/Fitting/FittingPerspective.py +++ b/src/sas/qtgui/Perspectives/Fitting/FittingPerspective.py @@ -575,6 +575,12 @@ def currentFittingWidget(self) -> FittingWidget | None: else: return None + @property + def undo_stack(self): + """Return undo stack for the currently selected fitting tab.""" + fitting_widget = self.currentFittingWidget + return None if fitting_widget is None else fitting_widget.undo_stack + def getFitTabs(self): """ Returns the list of fitting tabs diff --git a/src/sas/qtgui/Perspectives/Fitting/FittingWidget.py b/src/sas/qtgui/Perspectives/Fitting/FittingWidget.py index 19624cb52a..1eec71304c 100644 --- a/src/sas/qtgui/Perspectives/Fitting/FittingWidget.py +++ b/src/sas/qtgui/Perspectives/Fitting/FittingWidget.py @@ -36,6 +36,16 @@ from sas.qtgui.Perspectives.Fitting.ReportPageLogic import ReportPageLogic from sas.qtgui.Perspectives.Fitting.SmearingWidget import SmearingWidget from sas.qtgui.Perspectives.Fitting.UI.FittingWidgetUI import Ui_FittingWidgetUI +from sas.qtgui.Perspectives.Fitting.UndoRedo import ( + CheckboxToggleCommand, + FitOptionsCommand, + FitResultCommand, + ModelSelectionCommand, + ParameterMinMaxCommand, + ParameterValueCommand, + SmearingOptionsCommand, + UndoStack, +) from sas.qtgui.Perspectives.Fitting.ViewDelegate import ModelViewDelegate from sas.qtgui.Plotting.Plotter import PlotterWidget from sas.qtgui.Plotting.PlotterData import Data1D, Data2D, DataRole @@ -296,10 +306,10 @@ def initializeGlobals(self) -> None: self.weighting = 0 self.chi2 = None - # Does the control support UNDO/REDO - # temporarily off - self.undo_supported = False - self.page_stack = [] + # Undo/redo stack (per-tab, incremental command pattern) + self.undo_stack = UndoStack(self) + self._last_smearing_state = None + self._last_model_triple = None # (category, model, structure) for undo capture self.all_data = [] # custom plugin models # {model.name:model} @@ -485,36 +495,40 @@ def setEnablementOnDataLoad(self) -> None: """ Enable/disable various UI elements based on data loaded """ - # Tag along functionality - self.label.setText("Data loaded from: ") - if self.logic.data.name: - self.lblFilename.setText(self.logic.data.name) - else: - self.lblFilename.setText(self.logic.data.filename) - self.updateQRange() - # Switch off Data2D control - self.chk2DView.setEnabled(False) - self.chk2DView.setVisible(False) - self.chkMagnetism.setEnabled(self.canHaveMagnetism()) - self.tabFitting.setTabEnabled(TAB_MAGNETISM, self.chkMagnetism.isChecked()) - # Combo box or label for file name" - if self.is_batch_fitting: - self.lblFilename.setVisible(False) - for dataitem in self.all_data: - name = GuiUtils.dataFromItem(dataitem).name - self.cbFileNames.addItem(name) - self.cbFileNames.setVisible(True) - self.chkChainFit.setEnabled(True) - self.chkChainFit.setVisible(True) - # This panel is not designed to view individual fits, so disable plotting - self.cmdPlot.setVisible(False) - # Similarly on other tabs - self.options_widget.setEnablementOnDataLoad() - self.onSelectModel() - # Smearing tab - self.smearing_widget.updateData(self.data) - # Check if a model was already loaded when data is sent to the tab - self.cmdFit.setEnabled(self.haveParamsToFit()) + # Suppress undo capture: data loading is a programmatic operation, + # not a user-initiated parameter change. Without this, updateQRange + # and onWeightingChoice push spurious FitOptionsCommand entries. + with self.undo_stack.suppressed(): + # Tag along functionality + self.label.setText("Data loaded from: ") + if self.logic.data.name: + self.lblFilename.setText(self.logic.data.name) + else: + self.lblFilename.setText(self.logic.data.filename) + self.updateQRange() + # Switch off Data2D control + self.chk2DView.setEnabled(False) + self.chk2DView.setVisible(False) + self.chkMagnetism.setEnabled(self.canHaveMagnetism()) + self.tabFitting.setTabEnabled(TAB_MAGNETISM, self.chkMagnetism.isChecked()) + # Combo box or label for file name" + if self.is_batch_fitting: + self.lblFilename.setVisible(False) + for dataitem in self.all_data: + name = GuiUtils.dataFromItem(dataitem).name + self.cbFileNames.addItem(name) + self.cbFileNames.setVisible(True) + self.chkChainFit.setEnabled(True) + self.chkChainFit.setVisible(True) + # This panel is not designed to view individual fits, so disable plotting + self.cmdPlot.setVisible(False) + # Similarly on other tabs + self.options_widget.setEnablementOnDataLoad() + self.onSelectModel() + # Smearing tab + self.smearing_widget.updateData(self.data) + # Check if a model was already loaded when data is sent to the tab + self.cmdFit.setEnabled(self.haveParamsToFit()) def acceptsData(self) -> bool: """ Tells the caller this widget can accept new dataset """ @@ -566,6 +580,9 @@ def togglePoly(self, isChecked: bool) -> None: # Check if any parameters are ready for fitting self.cmdFit.setEnabled(self.haveParamsToFit()) self.polydispersity_widget.togglePoly(isChecked) + self.undo_stack.push( + CheckboxToggleCommand('chkPolydispersity', not isChecked, isChecked) + ) def onPolyToggled(self, isChecked: bool) -> None: """ @@ -583,6 +600,9 @@ def toggleMagnetism(self, isChecked: bool) -> None: # Check if any parameters are ready for fitting self.cmdFit.setEnabled(self.haveParamsToFit()) self.magnetism_widget.isActive = isChecked + self.undo_stack.push( + CheckboxToggleCommand('chkMagnetism', not isChecked, isChecked) + ) def onMagnetismToggled(self, isChecked: bool) -> None: """ @@ -607,9 +627,15 @@ def toggle2D(self, isChecked: bool) -> None: """ Enable/disable the controls dependent on 1D/2D data instance """ self.chkMagnetism.setEnabled(isChecked) self.is2D = isChecked - # Reload the current model - if self.logic.kernel_module: - self.onSelectModel() + # Reload the current model — suppress so the inner onSelectModel + # doesn't push its own command; toggle2D is one atomic undo step. + with self.undo_stack.suppressed(): + if self.logic.kernel_module: + self.onSelectModel() + + self.undo_stack.push( + CheckboxToggleCommand('chk2DView', not isChecked, isChecked) + ) @classmethod def customModels(cls) -> dict[str, Any]: @@ -1163,6 +1189,11 @@ def onSelectModel(self) -> None: self.cbModel.setCurrentIndex(self._previous_model_index) self.cbModel.blockSignals(False) return + + # Capture old state for undo before anything changes + old_triple = self._last_model_triple + old_params = self._get_parameter_dict() + if self.model_data is not None: # Store any old parameters before switching to a new model self.page_parameters = self.getParameterDict() @@ -1174,30 +1205,46 @@ def onSelectModel(self) -> None: if not model: return - self.chkMagnetism.setEnabled(self.canHaveMagnetism()) - self.tabFitting.setTabEnabled(TAB_MAGNETISM, self.chkMagnetism.isChecked() and self.canHaveMagnetism()) - self._previous_model_index = self.cbModel.currentIndex() - - # Reset parameters to fit - self.resetParametersToFit() - self.has_error_column = False - self.polydispersity_widget.is2D = self.is2D - self.polydispersity_widget.has_poly_error_column = False - self.magnetism_widget.has_magnet_error_column = False + # Suppress undo capture during the model rebuild + with self.undo_stack.suppressed(): + self.chkMagnetism.setEnabled(self.canHaveMagnetism()) + self.tabFitting.setTabEnabled(TAB_MAGNETISM, self.chkMagnetism.isChecked() and self.canHaveMagnetism()) + self._previous_model_index = self.cbModel.currentIndex() - structure = None - if self.cbStructureFactor.isEnabled(): - structure = str(self.cbStructureFactor.currentText()) - self.respondToModelStructure(model=model, structure_factor=structure) + # Reset parameters to fit + self.resetParametersToFit() + self.has_error_column = False + self.polydispersity_widget.is2D = self.is2D + self.polydispersity_widget.has_poly_error_column = False + self.magnetism_widget.has_magnet_error_column = False - # paste parameters from previous state - if self.page_parameters: - self.updatePageWithParameters(self.page_parameters, warn_user=False) + structure = None + if self.cbStructureFactor.isEnabled(): + structure = str(self.cbStructureFactor.currentText()) + self.respondToModelStructure(model=model, structure_factor=structure) + + # paste parameters from previous state + if self.page_parameters: + self.updatePageWithParameters(self.page_parameters, warn_user=False) + + # disable polydispersity if the model does not support it + has_poly = self.polydispersity_widget.poly_model.rowCount() != 0 + self.chkPolydispersity.setEnabled(has_poly) + # self.tabFitting.setTabEnabled(TAB_POLY, has_poly) + + # Capture new state after model change + new_triple = ( + str(self.cbCategory.currentText()), + str(self.cbModel.currentText()), + str(self.cbStructureFactor.currentText()) if self.cbStructureFactor.isEnabled() else '', + ) + new_params = self._get_parameter_dict() + self._last_model_triple = new_triple - # disable polydispersity if the model does not support it - has_poly = self.polydispersity_widget.poly_model.rowCount() != 0 - self.chkPolydispersity.setEnabled(has_poly) - # self.tabFitting.setTabEnabled(TAB_POLY, has_poly) + if old_triple is not None and (old_triple != new_triple or old_params != new_params): + self.undo_stack.push( + ModelSelectionCommand(old_triple, new_triple, old_params, new_params) + ) # set focus so it doesn't move up self.cbModel.setFocus() @@ -1380,9 +1427,6 @@ def respondToModelStructure(self, model: str | None = None, structure_factor: st # Update plot self.updateData() - # Update state stack - self.updateUndo() - # Let others know self.newModelSignal.emit() @@ -1532,6 +1576,7 @@ def onFit(self) -> None: # Disable some elements self.disableInteractiveElements() + self.undo_stack.set_enabled(False) def stopFit(self) -> None: """ @@ -1542,6 +1587,7 @@ def stopFit(self) -> None: self.calc_fit.stop() #re-enable the Fit button self.enableInteractiveElements() + self.undo_stack.set_enabled(True) msg = "Fitting cancelled." self.communicate.statusBarUpdateSignal.emit(msg) @@ -1636,6 +1682,7 @@ def fitComplete(self, result: tuple) -> None: """ #re-enable the Fit button self.enableInteractiveElements() + self.undo_stack.set_enabled(True) if not result or not result[0] or not result[0][0]: msg = "Fitting failed." @@ -1644,6 +1691,9 @@ def fitComplete(self, result: tuple) -> None: self.kernel_module = copy.deepcopy(self.kernel_module_copy) return + # Capture parameter snapshot BEFORE fit results are applied + old_params = self._get_parameter_dict() + # Don't recalculate chi2 - it's in res.fitness already self.fitResults = True if result is None or len(result) == 0 or len(result[0]) == 0: @@ -1671,11 +1721,10 @@ def fitComplete(self, result: tuple) -> None: # Dictionary of fitted parameter: value, error # e.g. param_dic = {"sld":(1.703, 0.0034), "length":(33.455, -0.0983)} - self.fitting_controller.updateModelFromList(param_dict) - - self.polydispersity_widget.updatePolyModelFromList(param_dict) - - self.magnetism_widget.updateMagnetModelFromList(param_dict) + with self.undo_stack.suppressed(): + self.fitting_controller.updateModelFromList(param_dict) + self.polydispersity_widget.updatePolyModelFromList(param_dict) + self.magnetism_widget.updateMagnetModelFromList(param_dict) # update charts self.onPlot() @@ -1684,6 +1733,11 @@ def fitComplete(self, result: tuple) -> None: chi2_repr = GuiUtils.formatNumber(self.chi2, high=True) self.lblChi2Value.setText(chi2_repr) + # Push a single FitResultCommand for the entire fit + new_params = self._get_parameter_dict() + if old_params != new_params: + self.undo_stack.push(FitResultCommand(old_params, new_params)) + def prepareFitters(self, fitter: Fit | None = None, fit_id: int = 0, weight_increase: int = 1) -> tuple[list[Fit], int]: """ @@ -1752,11 +1806,18 @@ def onSmearingOptionsUpdate(self) -> None: """ React to changes in the smearing widget """ + old_state = self._last_smearing_state + # update display smearing, accuracy, smearing_min, smearing_max = self.smearing_widget.state() self.lblCurrentSmearing.setText(smearing) self.calculateQGridForModel() + new_state = self._get_smearing_state_dict() + if old_state is not None and old_state != new_state: + self.undo_stack.push(SmearingOptionsCommand(old_state, new_state)) + self._last_smearing_state = new_state + def onKey(self, event: QtGui.QKeyEvent) -> None: if event.key() in [QtCore.Qt.Key_Enter, QtCore.Qt.Key_Return] and self.cmdPlot.isEnabled(): self.onPlot() @@ -1813,6 +1874,8 @@ def onOptionsUpdate(self) -> None: """ Update local option values and replot """ + old_opts = self._get_fit_options_dict() + self.q_range_min, self.q_range_max, self.npts, self.log_points, self.weighting = \ self.options_widget.state() # set Q range labels on the main tab @@ -1820,6 +1883,10 @@ def onOptionsUpdate(self) -> None: self.lblMaxRangeDef.setText(GuiUtils.formatNumber(self.q_range_max, high=True)) self.recalculatePlotData() + new_opts = self._get_fit_options_dict() + if old_opts != new_opts: + self.undo_stack.push(FitOptionsCommand(old_opts, new_opts)) + def setDefaultStructureCombo(self) -> None: """ Fill in the structure factors combo box with defaults @@ -2061,11 +2128,9 @@ def fromModelToQModel(self, model_name: str) -> None: self.logic.kernel_module.name = self.modelName() # Explicitly add scale and background with default values - temp_undo_state = self.undo_supported - self.undo_supported = False - self.addScaleToModel(self._model_model) - self.addBackgroundToModel(self._model_model) - self.undo_supported = temp_undo_state + with self.undo_stack.suppressed(): + self.addScaleToModel(self._model_model) + self.addBackgroundToModel(self._model_model) self.logic.shell_names = self.shellNamesList() @@ -2195,8 +2260,7 @@ def onMainParamsChange(self, top: QtCore.QModelIndex, bottom: QtCore.QModelIndex if model_column == 0: self.checkboxSelected(item, model_key="standard") self.cmdFit.setEnabled(self.haveParamsToFit()) - # Update state stack - self.updateUndo() + # Fit-checkbox toggles intentionally excluded from undo stack return model_row = item.row() @@ -2221,16 +2285,18 @@ def onMainParamsChange(self, top: QtCore.QModelIndex, bottom: QtCore.QModelIndex min_column = self.lstParams.itemDelegate().param_min max_column = self.lstParams.itemDelegate().param_max if model_column == param_column: - # don't try to update multiplicity counters if they aren't there. - # Note that this will fail for proper bad update where the model - # doesn't contain multiplicity parameter + old_val = self.logic.kernel_module.getParam(parameter_name) self.logic.kernel_module.setParam(parameter_name, value) + self.undo_stack.push(ParameterValueCommand(parameter_name, old_val, value)) elif model_column == min_column: - # min/max to be changed in self.logic.kernel_module.details[parameter_name] = ['Ang', 0.0, inf] + old_val = self.logic.kernel_module.details[parameter_name][1] self.logic.kernel_module.details[parameter_name][1] = value + self.undo_stack.push(ParameterMinMaxCommand(parameter_name, "min", old_val, value)) elif model_column == max_column: + old_val = self.logic.kernel_module.details[parameter_name][2] self.logic.kernel_module.details[parameter_name][2] = value + self.undo_stack.push(ParameterMinMaxCommand(parameter_name, "max", old_val, value)) else: # don't update the chart return @@ -2239,8 +2305,8 @@ def onMainParamsChange(self, top: QtCore.QModelIndex, bottom: QtCore.QModelIndex # necessary self.processEffectiveRadius() - # Update state stack - self.updateUndo() + # Keep page_parameters up-to-date so parameter retention across + # model switches works even when no plot has been computed yet. self.page_parameters = self.getParameterDict() def processEffectiveRadius(self) -> None: @@ -3093,34 +3159,121 @@ def saveToFitPage(self, fp: FitPage) -> None: # TODO: add polydispersity and magnetism - def updateUndo(self) -> None: - """ - Create a new state page and add it to the stack - """ - if self.undo_supported: - self.pushFitPage(self.currentState()) + # ------------------------------------------------------------------ + # Undo/redo helper methods (called by UndoCommand subclasses) + # ------------------------------------------------------------------ - def currentState(self) -> FitPage: - """ - Return fit page with current state - """ - new_page = FitPage() - self.saveToFitPage(new_page) + def _update_model_param_value(self, param_name: str, value: float) -> None: + """Update the UI model item for a parameter value (called by undo/redo).""" + for row in range(self._model_model.rowCount()): + name_item = self._model_model.item(row, 0) + if name_item is None: + continue + row_name = str(name_item.data(QtCore.Qt.UserRole) or name_item.text()) + if row_name == param_name: + col = self.lstParams.itemDelegate().param_value + value_item = self._model_model.item(row, col) + if value_item: + value_item.setText(GuiUtils.formatNumber(value, high=True)) + return - return new_page + def _update_model_param_limit(self, param_name: str, bound: str, value: float) -> None: + """Update min or max bound in the UI model (called by undo/redo).""" + col = self.lstParams.itemDelegate().param_min if bound == "min" else self.lstParams.itemDelegate().param_max + for row in range(self._model_model.rowCount()): + name_item = self._model_model.item(row, 0) + if name_item is None: + continue + row_name = str(name_item.data(QtCore.Qt.UserRole) or name_item.text()) + if row_name == param_name: + item = self._model_model.item(row, col) + if item: + item.setText(GuiUtils.formatNumber(value, high=True)) + return - def pushFitPage(self, new_page: FitPage) -> None: - """ - Add a new fit page object with current state - """ - self.page_stack.append(new_page) + def _restore_model_selection(self, triple: tuple, params: dict) -> None: + """Restore model selection and parameter values (called by undo/redo). - def popFitPage(self) -> None: + ``triple`` is ``(category, model, structure_factor)``. """ - Remove top fit page from stack - """ - if self.page_stack: - self.page_stack.pop() + with self.undo_stack.suppressed(): + category, model, structure = triple + # Block cbModel signals while changing category to prevent + # onSelectModel firing with the wrong model during repopulation. + self.cbModel.blockSignals(True) + self.cbCategory.setCurrentIndex(self.cbCategory.findText(category)) + self.cbModel.blockSignals(False) + self.cbModel.setCurrentIndex(self.cbModel.findText(model)) + if structure: + self.cbStructureFactor.setCurrentIndex( + self.cbStructureFactor.findText(structure) + ) + else: + self.cbStructureFactor.setCurrentIndex(0) + # Apply saved parameter values + self._restore_parameter_values(params) + + def _apply_fit_options(self, options: dict) -> None: + """Apply fit options dict (called by undo/redo).""" + with self.undo_stack.suppressed(): + self.q_range_min = options.get('q_range_min', self.q_range_min) + self.q_range_max = options.get('q_range_max', self.q_range_max) + self.npts = options.get('npts', self.npts) + self.log_points = options.get('log_points', self.log_points) + self.weighting = options.get('weighting', self.weighting) + self.options_widget.setState( + self.q_range_min, self.q_range_max, + self.npts, self.log_points, self.weighting, + ) + self.lblMinRangeDef.setText(GuiUtils.formatNumber(self.q_range_min, high=True)) + self.lblMaxRangeDef.setText(GuiUtils.formatNumber(self.q_range_max, high=True)) + self.recalculatePlotData() + + def _apply_smearing_state(self, state: dict) -> None: + """Apply smearing options from a dict (called by undo/redo).""" + with self.undo_stack.suppressed(): + self.smearing_widget.setState( + state.get('smearing'), state.get('accuracy'), + state.get('d_down'), state.get('d_up'), + ) + self.calculateQGridForModel() + + def _restore_parameter_values(self, params: dict) -> None: + """Restore all kernel parameter values from a ``{name: value}`` dict.""" + with self.undo_stack.suppressed(): + for name, value in params.items(): + self.logic.kernel_module.setParam(name, value) + self._update_model_param_value(name, value) + self.calculateQGridForModel() + + def _get_parameter_dict(self) -> dict: + """Return ``{param_name: float_value}`` for all current kernel params.""" + if self.logic.kernel_module is None: + return {} + return { + p.name: self.logic.kernel_module.getParam(p.name) + for p in self.logic.kernel_module._model_info.parameters.kernel_parameters + } + + def _get_fit_options_dict(self) -> dict: + """Return current fit options as a dict (for undo command capture).""" + return { + 'q_range_min': self.q_range_min, + 'q_range_max': self.q_range_max, + 'npts': self.npts, + 'log_points': self.log_points, + 'weighting': self.weighting, + } + + def _get_smearing_state_dict(self) -> dict: + """Return current smearing state as a dict (for undo command capture).""" + smearing, accuracy, d_down, d_up = self.smearing_widget.state() + return { + 'smearing': smearing, + 'accuracy': accuracy, + 'd_down': d_down, + 'd_up': d_up, + } def getReport(self) -> list[str]: """ diff --git a/src/sas/qtgui/Perspectives/Fitting/MagnetismWidget.py b/src/sas/qtgui/Perspectives/Fitting/MagnetismWidget.py index 985726e2a0..fee4c9d242 100644 --- a/src/sas/qtgui/Perspectives/Fitting/MagnetismWidget.py +++ b/src/sas/qtgui/Perspectives/Fitting/MagnetismWidget.py @@ -12,6 +12,10 @@ # Local UI from sas.qtgui.Perspectives.Fitting.UI.MagnetismWidget import Ui_MagnetismWidgetUI +from sas.qtgui.Perspectives.Fitting.UndoRedo import ( + ParameterMinMaxCommand, + ParameterValueCommand, +) from sas.qtgui.Perspectives.Fitting.ViewDelegate import MagnetismViewDelegate logger = logging.getLogger(__name__) @@ -31,6 +35,7 @@ def __init__(self, parent: QtWidgets.QWidget | None = None, logic: Any | None = self._magnet_model = FittingUtilities.ToolTippedItemModel() self.is2D = False self.isActive = False + self._fitting_widget = parent self.logic = parent.logic self.magnet_params = {} self.has_magnet_error_column = False @@ -138,18 +143,30 @@ def onMagnetModelChange(self, top: QtCore.QModelIndex, bottom: QtCore.QModelInde if model_column > 1: if model_column == delegate.mag_min: pos = 1 + bound = "min" elif model_column == delegate.mag_max: pos = 2 + bound = "max" elif model_column == delegate.mag_unit: pos = 0 + bound = None else: # For all other values sent here (e.g. the error column, do nothing) return # min/max to be changed in self.logic.kernel_module.details[parameter_name] = ['Ang', 0.0, inf] + old_val = self.logic.kernel_module.details[parameter_name][pos] self.logic.kernel_module.details[parameter_name][pos] = value + if bound is not None: + self._fitting_widget.undo_stack.push( + ParameterMinMaxCommand(parameter_name, bound, old_val, value) + ) else: + old_val = self.logic.kernel_module.getParam(parameter_name) self.magnet_params[parameter_name] = value self.logic.kernel_module.setParam(parameter_name, value) + self._fitting_widget.undo_stack.push( + ParameterValueCommand(parameter_name, old_val, value) + ) # Update plot self.updateDataSignal.emit() diff --git a/src/sas/qtgui/Perspectives/Fitting/OptionsWidget.py b/src/sas/qtgui/Perspectives/Fitting/OptionsWidget.py index 03d5bb8647..6d3d74f2f9 100644 --- a/src/sas/qtgui/Perspectives/Fitting/OptionsWidget.py +++ b/src/sas/qtgui/Perspectives/Fitting/OptionsWidget.py @@ -239,14 +239,24 @@ def updateQRange(self, q_range_min, q_range_max, npts): """ Update the local model based on calculated values """ - qmax = str(q_range_max) - qmin = str(q_range_min) - self.model.item(self.MODEL.index('MIN_RANGE')).setText(qmin) - self.model.item(self.MODEL.index('MAX_RANGE')).setText(qmax) - self.model.item(self.MODEL.index('NPTS')).setText(str(npts)) - self.qmin, self.qmax, self.npts = q_range_min, q_range_max, npts - npts_fit = self.npts2fit(self.logic.data, self.qmin, self.qmax, self.npts) - self.model.item(self.MODEL.index('NPTS_FIT')).setText(str(npts_fit)) + # Block signals to prevent intermediate dataChanged→onModelChange→ + # plot_signal firing for each individual setText. Without this, + # onOptionsUpdate receives partially-updated state and can push + # multiple spurious FitOptionsCommand entries onto the undo stack. + self.model.blockSignals(True) + try: + qmax = str(q_range_max) + qmin = str(q_range_min) + self.model.item(self.MODEL.index('MIN_RANGE')).setText(qmin) + self.model.item(self.MODEL.index('MAX_RANGE')).setText(qmax) + self.model.item(self.MODEL.index('NPTS')).setText(str(npts)) + self.qmin, self.qmax, self.npts = q_range_min, q_range_max, npts + npts_fit = self.npts2fit(self.logic.data, self.qmin, self.qmax, self.npts) + self.model.item(self.MODEL.index('NPTS_FIT')).setText(str(npts_fit)) + finally: + self.model.blockSignals(False) + # Single signal after all values are consistent + self.plot_signal.emit() def state(self): """ @@ -259,6 +269,26 @@ def state(self): log_points = self.chkLogData.isChecked() return (q_range_min, q_range_max, npts, log_points, self.weighting) + def setState(self, q_range_min, q_range_max, npts, log_points, weighting): + """ + Set the state of controls from provided values. + Used by undo/redo to restore fit options. + """ + self.model.blockSignals(True) + self.updateQRange(q_range_min, q_range_max, npts) + self.chkLogData.setChecked(log_points) + self.weighting = weighting + # Update the weighting radio buttons to match + buttons = self.weightingGroup.buttons() + for btn in buttons: + btn_id = abs(self.weightingGroup.id(btn) + 2) + if btn_id == weighting: + btn.setChecked(True) + break + self.model.blockSignals(False) + # Refresh the QDataWidgetMapper so text fields reflect the model + self.mapper.toFirst() + def npts2fit(self, data=None, qmin=None, qmax=None, npts=None): """ return numbers of data points within qrange diff --git a/src/sas/qtgui/Perspectives/Fitting/PolydispersityWidget.py b/src/sas/qtgui/Perspectives/Fitting/PolydispersityWidget.py index d6cf00291d..d2f26e2037 100644 --- a/src/sas/qtgui/Perspectives/Fitting/PolydispersityWidget.py +++ b/src/sas/qtgui/Perspectives/Fitting/PolydispersityWidget.py @@ -15,6 +15,10 @@ # Local UI from sas.qtgui.Perspectives.Fitting.UI.PolydispersityWidget import Ui_PolydispersityWidgetUI +from sas.qtgui.Perspectives.Fitting.UndoRedo import ( + ParameterMinMaxCommand, + ParameterValueCommand, +) from sas.qtgui.Perspectives.Fitting.ViewDelegate import PolyViewDelegate DEFAULT_POLYDISP_FUNCTION = 'gaussian' @@ -34,6 +38,7 @@ def __init__(self, parent: QtWidgets.QWidget | None = None) -> None: self.poly_model = FittingUtilities.ToolTippedItemModel() self.is2D = False self.isActive = False + self._fitting_widget = parent self.logic = parent.logic self.poly_params = {} self.has_poly_error_column = False @@ -139,7 +144,6 @@ def onPolyModelChange(self, top: QtCore.QModelIndex) -> None: if parameter_name_w in self.poly_params_to_fit: self.poly_params_to_fit.remove(parameter_name_w) self.cmdFitSignal.emit() - # self.updateUndo() elif model_column in [delegate.poly_min, delegate.poly_max]: try: @@ -151,9 +155,15 @@ def onPolyModelChange(self, top: QtCore.QModelIndex) -> None: current_details = self.logic.kernel_module.details[parameter_name_w] if self.has_poly_error_column: # err column changes the indexing - current_details[model_column-2] = value + pos = model_column - 2 else: - current_details[model_column-1] = value + pos = model_column - 1 + old_val = current_details[pos] + current_details[pos] = value + bound = "min" if pos == 1 else "max" + self._fitting_widget.undo_stack.push( + ParameterMinMaxCommand(parameter_name_w, bound, old_val, value) + ) elif model_column == delegate.poly_function: # name of the function - just pass @@ -172,8 +182,12 @@ def onPolyModelChange(self, top: QtCore.QModelIndex) -> None: # Map the column to the poly param that was changed associations = {1: "width", delegate.poly_npts: "npts", delegate.poly_nsigs: "nsigmas"} p_name = f"{parameter_name}.{associations.get(model_column, 'width')}" + old_val = self.logic.kernel_module.getParam(p_name) self.poly_params[p_name] = value self.logic.kernel_module.setParam(p_name, value) + self._fitting_widget.undo_stack.push( + ParameterValueCommand(p_name, old_val, value) + ) # Update plot self.updateDataSignal.emit() diff --git a/src/sas/qtgui/Perspectives/Fitting/UndoRedo.py b/src/sas/qtgui/Perspectives/Fitting/UndoRedo.py new file mode 100644 index 0000000000..e039246ec9 --- /dev/null +++ b/src/sas/qtgui/Perspectives/Fitting/UndoRedo.py @@ -0,0 +1,632 @@ +"""Undo/Redo infrastructure for the SasView Fitting perspective. + +Provides UndoCommand (abstract base), concrete subclasses for each kind of +undoable action, and UndoStack which manages per-tab history. + +Phase 1 of UNDO_PLAN_CLAUDE.md — core infrastructure only. +Integration with FittingWidget is done in Phase 2. + +Design notes: +- Each command stores only old_value + new_value (delta, not snapshot). +- UndoStack is a QObject so it can emit stackChanged for UI wiring. +- Command capture is suppressed during programmatic updates via suppressed(). +- ParameterValueCommand supports coalescing: consecutive edits to the same + parameter are merged into one entry (Qt fires dataChanged on commit, not + per keystroke, providing the natural coalescing boundary). +- Parameter 'fit' checkbox toggles are intentionally NOT tracked + (see UNDO_PLAN_CLAUDE.md, Decisions). +""" +from __future__ import annotations + +import contextlib +import logging +import time +import traceback +from typing import Any + +from PySide6 import QtCore, QtWidgets + +logger = logging.getLogger(__name__) + + +# --------------------------------------------------------------------------- +# Base command +# --------------------------------------------------------------------------- + +class UndoCommand: + """Abstract base for all undoable actions. + + Subclasses must implement ``undo(widget)`` and ``redo(widget)``. + ``description`` is shown in UI tooltips and the failure dialog. + """ + + def __init__(self, description: str) -> None: + self.description: str = description + self.timestamp: float = time.monotonic() + + def undo(self, widget) -> None: + """Apply the reverse change to *widget*.""" + raise NotImplementedError(f"{type(self).__name__}.undo() not implemented") + + def redo(self, widget) -> None: + """Re-apply the forward change to *widget*.""" + raise NotImplementedError(f"{type(self).__name__}.redo() not implemented") + + def can_merge(self, other: UndoCommand) -> bool: + """Return True if *other* may be merged into this command. + + Merging collapses consecutive edits (e.g. two value changes to the + same parameter) into a single undo entry. Default: no merging. + """ + return False + + def merge(self, other: UndoCommand) -> UndoCommand: + """Return a single command combining *self* (earlier) with *other* (later). + + Only called when ``can_merge(other)`` returns True. + """ + raise NotImplementedError(f"{type(self).__name__}.merge() not implemented") + + def __repr__(self) -> str: + return f"<{type(self).__name__}: {self.description!r}>" + + +# --------------------------------------------------------------------------- +# Concrete commands +# --------------------------------------------------------------------------- + +class ParameterValueCommand(UndoCommand): + """Single parameter value change. + + Applies via ``widget.logic.kernel_module.setParam()`` and + ``widget._update_model_param_value()`` (added in Phase 2). + + Supports coalescing: two consecutive edits to the same parameter are + merged into one entry whose undo reverts all the way to the first + captured value. + """ + + def __init__(self, param_name: str, old_val: float, new_val: float) -> None: + super().__init__(f"Change {param_name}") + self._param_name = param_name + self._old_val = old_val + self._new_val = new_val + + @property + def param_name(self) -> str: + return self._param_name + + def undo(self, widget) -> None: + widget.logic.kernel_module.setParam(self._param_name, self._old_val) + widget._update_model_param_value(self._param_name, self._old_val) + + def redo(self, widget) -> None: + widget.logic.kernel_module.setParam(self._param_name, self._new_val) + widget._update_model_param_value(self._param_name, self._new_val) + + #: Maximum age difference (seconds) between two edits to the same parameter + #: that may still be coalesced into a single undo entry. Edits farther + #: apart than this are treated as independent actions. + _COALESCE_WINDOW_SECONDS: float = 5.0 + + def can_merge(self, other: UndoCommand) -> bool: + return ( + isinstance(other, ParameterValueCommand) + and other._param_name == self._param_name + and (other.timestamp - self.timestamp) <= self._COALESCE_WINDOW_SECONDS + ) + + def merge(self, other: ParameterValueCommand) -> ParameterValueCommand: + """Merge *self* (earlier) with *other* (later). + + The merged command undoes all the way to *self*'s old value and + redoes all the way to *other*'s new value. The *self* timestamp + (earlier edit) is preserved. + """ + merged = ParameterValueCommand(self._param_name, self._old_val, other._new_val) + merged.timestamp = self.timestamp + return merged + + +class ParameterMinMaxCommand(UndoCommand): + """Parameter min or max bound change. + + ``bound`` must be ``"min"`` or ``"max"``. + Writes directly to ``kernel_module.details[param_name][1 or 2]`` and + delegates UI item update to ``widget._update_model_param_limit()`` + (added in Phase 2). + """ + + _BOUND_INDEX: dict[str, int] = {"min": 1, "max": 2} + + def __init__( + self, param_name: str, bound: str, old_val: float, new_val: float + ) -> None: + assert bound in ("min", "max"), ( + f"bound must be 'min' or 'max', got {bound!r}" + ) + super().__init__(f"Change {param_name} {bound}") + self._param_name = param_name + self._bound = bound + self._old_val = old_val + self._new_val = new_val + + def _apply(self, widget, value: float) -> None: + idx = self._BOUND_INDEX[self._bound] + widget.logic.kernel_module.details[self._param_name][idx] = value + widget._update_model_param_limit(self._param_name, self._bound, value) + + def undo(self, widget) -> None: + self._apply(widget, self._old_val) + + def redo(self, widget) -> None: + self._apply(widget, self._new_val) + + +class ModelSelectionCommand(UndoCommand): + """Category / model / structure-factor triple change. + + ``old_triple`` / ``new_triple`` are ``(category, model, structure_factor)``. + ``old_params`` / ``new_params`` are ``{param_name: value}`` dicts. + + On undo the old model triple is re-selected (triggering a param table + rebuild) and old parameter *values* are re-applied. UI micro-state + (expanded rows, active editor) is NOT restored — values only. + + The entire replay must run inside ``undo_stack.suppressed()`` (handled + in Phase 2) to prevent the internal rebuild from creating spurious + stack entries. + + Delegates to ``widget._restore_model_selection(triple, params)`` + (added in Phase 2). + """ + + def __init__( + self, + old_triple: tuple[str, str, str], + new_triple: tuple[str, str, str], + old_params: dict[str, float], + new_params: dict[str, float], + ) -> None: + super().__init__(f"Select model {new_triple[1]!r}") + self._old_triple = old_triple + self._new_triple = new_triple + self._old_params = dict(old_params) + self._new_params = dict(new_params) + + def undo(self, widget) -> None: + widget._restore_model_selection(self._old_triple, self._old_params) + + def redo(self, widget) -> None: + widget._restore_model_selection(self._new_triple, self._new_params) + + +class FitOptionsCommand(UndoCommand): + """Q range, npts, log_points, weighting changes. + + Delegates to ``widget._apply_fit_options(options)`` (added in Phase 2). + """ + + def __init__( + self, old_options: dict[str, Any], new_options: dict[str, Any] + ) -> None: + super().__init__("Change fit options") + self._old_options = dict(old_options) + self._new_options = dict(new_options) + + def undo(self, widget) -> None: + widget._apply_fit_options(self._old_options) + + def redo(self, widget) -> None: + widget._apply_fit_options(self._new_options) + + +class SmearingOptionsCommand(UndoCommand): + """Smearing state change. + + Delegates to ``widget._apply_smearing_state(state)`` (added in Phase 2). + """ + + def __init__( + self, old_state: dict[str, Any], new_state: dict[str, Any] + ) -> None: + super().__init__("Change smearing options") + self._old_state = dict(old_state) + self._new_state = dict(new_state) + + def undo(self, widget) -> None: + widget._apply_smearing_state(self._old_state) + + def redo(self, widget) -> None: + widget._apply_smearing_state(self._new_state) + + +class CheckboxToggleCommand(UndoCommand): + """Polydispersity / magnetism / 2D-view toggle. + + ``checkbox_id`` is the attribute name of the QCheckBox on *widget* + (e.g. ``"chkPolydispersity"``). + + Note: parameter 'fit' checkbox toggles are intentionally NOT tracked. + See UNDO_PLAN_CLAUDE.md, Decisions for rationale. + """ + + def __init__(self, checkbox_id: str, old_bool: bool, new_bool: bool) -> None: + super().__init__(f"Toggle {checkbox_id}") + self._checkbox_id = checkbox_id + self._old_bool = old_bool + self._new_bool = new_bool + + def _apply(self, widget, value: bool) -> None: + getattr(widget, self._checkbox_id).setChecked(value) + + def undo(self, widget) -> None: + self._apply(widget, self._old_bool) + + def redo(self, widget) -> None: + self._apply(widget, self._new_bool) + + +class FitResultCommand(UndoCommand): + """Full parameter snapshot before and after a fit. + + ``old_params`` MUST be captured at the very start of ``fitComplete()``, + before ``updateModelFromList()`` is called (see UNDO_PLAN_CLAUDE.md, + Step 2.6 — Critical ordering). + + Delegates to ``widget._restore_parameter_values(params)`` + (added in Phase 2). + """ + + def __init__( + self, old_params: dict[str, float], new_params: dict[str, float] + ) -> None: + super().__init__("Fit result") + self._old_params = dict(old_params) + self._new_params = dict(new_params) + + def undo(self, widget) -> None: + widget._restore_parameter_values(self._old_params) + + def redo(self, widget) -> None: + widget._restore_parameter_values(self._new_params) + + +class CompoundCommand(UndoCommand): + """Groups multiple commands into a single atomic undo/redo entry. + + ``undo()`` executes sub-commands in reverse order. + ``redo()`` executes them in forward order. + + Used for model-selection changes, where the model switch and subsequent + parameter-value restores must be treated as one logical action. + """ + + def __init__( + self, commands: list[UndoCommand], description: str = "" + ) -> None: + desc = description or ( + commands[0].description if commands else "Compound action" + ) + super().__init__(desc) + self._commands: list[UndoCommand] = list(commands) + + @property + def commands(self) -> list[UndoCommand]: + """A copy of the contained command list.""" + return list(self._commands) + + def undo(self, widget) -> None: + for cmd in reversed(self._commands): + cmd.undo(widget) + + def redo(self, widget) -> None: + for cmd in self._commands: + cmd.redo(widget) + + +# --------------------------------------------------------------------------- +# UndoStack +# --------------------------------------------------------------------------- + +class UndoStack(QtCore.QObject): + """Per-tab undo/redo history for FittingWidget. + + Responsibilities: + - Maintain undo and redo stacks of UndoCommand objects. + - Coalesce consecutive commands when supported by the command type. + - Emit ``stackChanged`` whenever state changes so that actionUndo / + actionRedo enabled state and tooltip text can be refreshed. + - Suppress command capture during programmatic updates (readFitPage, + model rebuild, undo/redo replay) via the ``suppressed()`` context + manager or ``set_enabled(False)``. + - Handle command execution failures: log at WARNING, show a dialog, + and offer ``reset_to_last_good()`` when failures repeat. + + The stack depth defaults to ``config.UNDO_STACK_MAX_DEPTH`` (200). + + Usage (Phase 2 integration):: + + # In FittingWidget.__init__: + self.undo_stack = UndoStack(self) + + # Pushing a command: + self.undo_stack.push(ParameterValueCommand(name, old, new)) + + # Suppressing during programmatic updates: + with self.undo_stack.suppressed(): + self.readFitPage(...) + + # Saving recovery state after a successful undo/redo: + self.undo_stack.save_last_good_state(self.getParameterDict()) + """ + + stackChanged = QtCore.Signal() + + def __init__( + self, widget, parent: QtCore.QObject | None = None + ) -> None: + super().__init__(parent) + self._widget = widget + from sas import config as _sas_config + self._max_depth: int = getattr(_sas_config, "UNDO_STACK_MAX_DEPTH", 200) + self._undo_stack: list[UndoCommand] = [] + self._redo_stack: list[UndoCommand] = [] + self._enabled: bool = True + self._replaying: bool = False + self._last_good_state: dict[str, float] | None = None + self._consecutive_failures: int = 0 + + # ------------------------------------------------------------------ + # Public API + # ------------------------------------------------------------------ + + def push(self, cmd: UndoCommand) -> None: + """Push *cmd* onto the undo stack. + + - Dropped silently if the stack is disabled or a replay is active. + - Coalesced with the stack top when ``top.can_merge(cmd)`` is True. + - Clears the redo stack (new action invalidates forward history). + - Trims the oldest entry when depth exceeds ``_max_depth``. + """ + if not self._enabled or self._replaying: + return + + if self._undo_stack and self._undo_stack[-1].can_merge(cmd): + self._undo_stack[-1] = self._undo_stack[-1].merge(cmd) + logger.debug("UndoStack: merged into %r", self._undo_stack[-1]) + else: + self._undo_stack.append(cmd) + if len(self._undo_stack) > self._max_depth: + dropped = self._undo_stack.pop(0) + logger.debug( + "UndoStack: depth limit reached, dropped %r", dropped + ) + + self._redo_stack.clear() + logger.debug( + "UndoStack: pushed %r (depth=%d)", cmd, len(self._undo_stack) + ) + self.stackChanged.emit() + + def undo(self) -> None: + """Undo the most recent command.""" + if not self._enabled or not self._undo_stack: + return + cmd = self._undo_stack[-1] # peek — only removed from source on success + self._replaying = True + try: + cmd.undo(self._widget) + self._undo_stack.pop() + self._redo_stack.append(cmd) + self._consecutive_failures = 0 + self._auto_snapshot() + self._refresh_view() + logger.debug( + "UndoStack: undo %r (undo=%d, redo=%d)", + cmd, len(self._undo_stack), len(self._redo_stack), + ) + except Exception: + tb = traceback.format_exc() + logger.warning("UndoStack: undo failed for %r:\n%s", cmd, tb) + self._consecutive_failures += 1 + self._handle_failure(cmd, tb) + finally: + self._replaying = False + self.stackChanged.emit() + + def redo(self) -> None: + """Redo the most recently undone command.""" + if not self._enabled or not self._redo_stack: + return + cmd = self._redo_stack[-1] # peek — only removed from source on success + self._replaying = True + try: + cmd.redo(self._widget) + self._redo_stack.pop() + self._undo_stack.append(cmd) + self._consecutive_failures = 0 + self._auto_snapshot() + self._refresh_view() + logger.debug( + "UndoStack: redo %r (undo=%d, redo=%d)", + cmd, len(self._undo_stack), len(self._redo_stack), + ) + except Exception: + tb = traceback.format_exc() + logger.warning("UndoStack: redo failed for %r:\n%s", cmd, tb) + self._consecutive_failures += 1 + self._handle_failure(cmd, tb) + finally: + self._replaying = False + self.stackChanged.emit() + + def can_undo(self) -> bool: + """Return True if undo is possible (enabled and stack non-empty).""" + return self._enabled and bool(self._undo_stack) + + def can_redo(self) -> bool: + """Return True if redo is possible (enabled and stack non-empty).""" + return self._enabled and bool(self._redo_stack) + + def clear(self) -> None: + """Clear both stacks and reset failure state.""" + self._undo_stack.clear() + self._redo_stack.clear() + self._last_good_state = None + self._consecutive_failures = 0 + logger.debug("UndoStack: cleared") + self.stackChanged.emit() + + def set_enabled(self, enabled: bool) -> None: + """Enable or disable command capture and undo/redo execution. + + Emits ``stackChanged`` so that action enabled state is refreshed + immediately (e.g. buttons grey out when a fit starts). + """ + self._enabled = enabled + logger.debug("UndoStack: set_enabled=%s", enabled) + self.stackChanged.emit() + + @contextlib.contextmanager + def suppressed(self): + """Context manager: temporarily disable command capture. + + Use around programmatic model updates (readFitPage, model + initialization, undo/redo replay) to prevent spurious entries:: + + with self.undo_stack.suppressed(): + self.readFitPage(...) + """ + was_enabled = self._enabled + self._enabled = False + logger.debug("UndoStack: suppression entered") + try: + yield + finally: + self._enabled = was_enabled + logger.debug( + "UndoStack: suppression lifted (enabled=%s)", self._enabled + ) + + def undo_text(self) -> str: + """Human-readable label for Undo (suitable for tooltip).""" + if self._undo_stack: + return f"Undo {self._undo_stack[-1].description}" + return "Undo" + + def redo_text(self) -> str: + """Human-readable label for Redo (suitable for tooltip).""" + if self._redo_stack: + return f"Redo {self._redo_stack[-1].description}" + return "Redo" + + def save_last_good_state(self, state: dict[str, float]) -> None: + """Store *state* as the recovery snapshot. + + Call from FittingWidget after each successful undo/redo:: + + self.undo_stack.undo() + self.undo_stack.save_last_good_state(self.getParameterDict()) + """ + self._last_good_state = dict(state) + logger.debug( + "UndoStack: last_good_state saved (%d params)", + len(self._last_good_state), + ) + + def reset_to_last_good(self) -> None: + """Restore widget parameters from the most recent good snapshot. + + Invoked when the user clicks "Reset to Last Good State" in the + failure dialog. If no snapshot exists, logs a warning and returns. + """ + if self._last_good_state is None: + logger.warning( + "UndoStack: reset_to_last_good called but no snapshot available" + ) + return + try: + self._widget._restore_parameter_values(self._last_good_state) + logger.info( + "UndoStack: reset to last good state (%d params)", + len(self._last_good_state), + ) + except Exception: + logger.warning( + "UndoStack: reset_to_last_good failed:\n%s", + traceback.format_exc(), + ) + + # ------------------------------------------------------------------ + # Internal helpers + # ------------------------------------------------------------------ + + def _handle_failure(self, cmd: UndoCommand, tb: str) -> None: + """Show an error dialog; offer reset when failures repeat.""" + offer_reset = ( + self._consecutive_failures >= 2 + and self._last_good_state is not None + ) + parent = ( + self._widget + if isinstance(self._widget, QtWidgets.QWidget) + else None + ) + msg_box = QtWidgets.QMessageBox(parent) + msg_box.setIcon(QtWidgets.QMessageBox.Icon.Warning) + msg_box.setWindowTitle("Undo/Redo Error") + msg_box.setText( + f"An error occurred while replaying:\n\n" + f" {cmd.description}\n\n" + f"The command has not been removed from history. The widget\n" + f"state may be inconsistent — you may try again, or use\n" + f"'Reset to Last Good State' to recover a known-good state." + ) + msg_box.setDetailedText(tb) + if offer_reset: + reset_btn = msg_box.addButton( + "Reset to Last Good State", + QtWidgets.QMessageBox.ButtonRole.ResetRole, + ) + msg_box.addButton(QtWidgets.QMessageBox.StandardButton.Close) + msg_box.exec() + if msg_box.clickedButton() is reset_btn: + self.reset_to_last_good() + else: + msg_box.setStandardButtons( + QtWidgets.QMessageBox.StandardButton.Close + ) + msg_box.exec() + + def _auto_snapshot(self) -> None: + """Record widget state as the recovery snapshot after a successful replay. + + Calls ``widget._get_parameter_dict()`` if available; silently skips + when the method is absent (Phase 1 / mocks) or returns non-dict data. + Phase 2 adds ``_get_parameter_dict()`` to FittingWidget, at which + point every successful undo/redo automatically updates the snapshot + without any integration-side calls. + """ + try: + state = self._widget._get_parameter_dict() + except AttributeError: + return + if isinstance(state, dict): + self._last_good_state = dict(state) + logger.debug( + "UndoStack: auto-saved last_good_state (%d params)", + len(self._last_good_state), + ) + + def _refresh_view(self) -> None: + """Force the parameter table viewport to repaint after undo/redo. + + PySide6 QTreeView may defer repainting when model items are changed + programmatically (via QStandardItem.setText) rather than through + user interaction. This forces an immediate visual update. + """ + try: + self._widget.lstParams.viewport().update() + except AttributeError: + pass diff --git a/src/sas/qtgui/Perspectives/Fitting/UnitTesting/UndoRedoIntegrationTest.py b/src/sas/qtgui/Perspectives/Fitting/UnitTesting/UndoRedoIntegrationTest.py new file mode 100644 index 0000000000..e3dc10dc7e --- /dev/null +++ b/src/sas/qtgui/Perspectives/Fitting/UnitTesting/UndoRedoIntegrationTest.py @@ -0,0 +1,733 @@ +"""Integration tests for undo/redo wiring in FittingWidget (Phase 2). + +These tests verify that FittingWidget handlers push the correct undo +commands to the UndoStack in response to user-initiated state changes. + +Unlike UndoRedoTest.py (which tests Phase 1 command/stack internals in +isolation with a mocked widget), these tests spin up a real FittingWidget +and exercise its actual signal/slot plumbing. + +Test organisation: + TestUndoStackInitialization — stack exists, initial state correct + TestParameterValueUndo — main params, poly, magnetism + TestParameterMinMaxUndo — min/max edits via main model + TestModelSelectionUndo — switching models via cbModel + TestFitOptionsUndo — Q range / npts / weighting changes + TestSmearingOptionsUndo — smearing state changes + TestCheckboxToggleUndo — poly / magnetism / 2D toggles + TestFitResultUndo — undo after a fit completes + TestUndoStackDisabledDuringFit — stack disabled while fitting + TestOptionsWidgetSetState — OptionsWidget.setState() round-trip +""" +import glob +import os +from unittest.mock import MagicMock + +import pytest +from PySide6 import QtCore, QtWidgets + +from sasmodels.sasview_model import load_custom_model + +from sas.qtgui.Perspectives.Fitting import FittingUtilities, FittingWidget +from sas.qtgui.Perspectives.Fitting.UndoRedo import ( + CheckboxToggleCommand, + FitOptionsCommand, + FitResultCommand, + ModelSelectionCommand, + ParameterMinMaxCommand, + ParameterValueCommand, + SmearingOptionsCommand, + UndoStack, +) +from sas.qtgui.Utilities import GuiUtils +from sas.sascalc.fit.models import ModelManager, ModelManagerBase + +# --------------------------------------------------------------------------- +# Helpers (same pattern as FittingWidgetTest.py) +# --------------------------------------------------------------------------- + +class dummy_manager: + HELP_DIRECTORY_LOCATION = "html" + communicate = GuiUtils.Communicate() + + def __init__(self): + self._perspective = dummy_perspective() + + def perspective(self): + return self._perspective + + +class dummy_perspective: + + def __init__(self): + self.symbol_dict = {} + self.constraint_list = [] + self.constraint_tab = None + + def getActiveConstraintList(self): + return self.constraint_list + + def getSymbolDictForConstraints(self): + return self.symbol_dict + + def getConstraintTab(self): + return self.constraint_tab + + +def find_plugin_models_mod(): + plugins_dir = [ + os.path.abspath(path) for path in glob.glob("**/plugin_models", recursive=True) + if os.path.normpath("qtgui/Perspectives/Fitting/plugin_models") in os.path.abspath(path) + ][0] + plugins = {} + for filename in os.listdir(plugins_dir): + name, ext = os.path.splitext(filename) + if ext == '.py' and not name == '__init__': + path = os.path.abspath(os.path.join(plugins_dir, filename)) + model = load_custom_model(path) + plugins[model.name] = model + return plugins + + +class ModelManagerBaseMod(ModelManagerBase): + def _is_plugin_dir_changed(self): + return False + + def plugins_reset(self): + self.plugin_models = find_plugin_models_mod() + self.model_dictionary.clear() + self.model_dictionary.update(self.standard_models) + self.model_dictionary.update(self.plugin_models) + return self.get_model_list() + + +class ModelManagerMod(ModelManager): + base = None + + def __init__(self): + if ModelManagerMod.base is None: + ModelManagerMod.base = ModelManagerBaseMod() + + +class FittingWidgetMod(FittingWidget.FittingWidget): + def customModels(cls): + manager = ModelManagerMod() + manager.update() + return manager.base.plugin_models + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + +@pytest.fixture(autouse=True) +def _suppress_message_boxes(monkeypatch): + """Suppress QMessageBox dialogs globally.""" + monkeypatch.setattr( + "sas.qtgui.Perspectives.Fitting.UndoRedo.QtWidgets.QMessageBox", + MagicMock(), + ) + + +@pytest.fixture +def widget(qapp, monkeypatch): + """Create a real FittingWidget for integration testing.""" + w = FittingWidgetMod(dummy_manager()) + monkeypatch.setattr(FittingUtilities, 'checkConstraints', lambda *a, **kw: None) + yield w + w.close() + del w + + +@pytest.fixture +def widget_with_model(widget): + """FittingWidget with 'cylinder' model loaded and processEvents run.""" + category_index = widget.cbCategory.findText("Cylinder") + widget.cbCategory.setCurrentIndex(category_index) + model_index = widget.cbModel.findText("cylinder") + widget.cbModel.setCurrentIndex(model_index) + QtWidgets.QApplication.processEvents() + # Clear the undo stack so model-load commands don't interfere with tests + widget.undo_stack.clear() + return widget + + +# --------------------------------------------------------------------------- +# TestUndoStackInitialization +# --------------------------------------------------------------------------- + +class TestUndoStackInitialization: + + def test_widget_has_undo_stack(self, widget): + assert hasattr(widget, 'undo_stack') + assert isinstance(widget.undo_stack, UndoStack) + + def test_initial_stack_is_empty(self, widget): + assert not widget.undo_stack.can_undo() + assert not widget.undo_stack.can_redo() + + def test_stack_widget_reference(self, widget): + """The stack must reference the correct widget for command replay.""" + assert widget.undo_stack._widget is widget + + +# --------------------------------------------------------------------------- +# TestParameterValueUndo +# --------------------------------------------------------------------------- + +class TestParameterValueUndo: + + def test_main_param_change_pushes_value_command(self, widget_with_model): + """Editing a model parameter value should push a ParameterValueCommand.""" + w = widget_with_model + # Find the 'radius' row + param_col = w.lstParams.itemDelegate().param_value + for row in range(w._model_model.rowCount()): + name = w._model_model.item(row, 0).text() + if name == "radius": + item = w._model_model.item(row, param_col) + old_val = float(item.text()) + item.setText("99.0") + break + else: + pytest.fail("Could not find 'radius' parameter in the model") + + assert w.undo_stack.can_undo() + top_cmd = w.undo_stack._undo_stack[-1] + assert isinstance(top_cmd, ParameterValueCommand) + assert top_cmd.param_name == "radius" + assert top_cmd._new_val == 99.0 + assert top_cmd._old_val == old_val + + def test_main_param_undo_restores_value(self, widget_with_model): + """Undo should restore the original kernel parameter value.""" + w = widget_with_model + param_col = w.lstParams.itemDelegate().param_value + for row in range(w._model_model.rowCount()): + name = w._model_model.item(row, 0).text() + if name == "radius": + old_val = float(w._model_model.item(row, param_col).text()) + w._model_model.item(row, param_col).setText("99.0") + break + + assert w.logic.kernel_module.getParam("radius") == 99.0 + w.undo_stack.undo() + assert w.logic.kernel_module.getParam("radius") == old_val + + def test_checkbox_column_does_not_push_command(self, widget_with_model): + """Toggling the 'fit' checkbox (column 0) must NOT push an undo command.""" + w = widget_with_model + initial_count = len(w.undo_stack._undo_stack) + item = w._model_model.item(0, 0) + item.setCheckState( + QtCore.Qt.Unchecked if item.checkState() == QtCore.Qt.Checked + else QtCore.Qt.Checked + ) + assert len(w.undo_stack._undo_stack) == initial_count + + +# --------------------------------------------------------------------------- +# TestParameterMinMaxUndo +# --------------------------------------------------------------------------- + +class TestParameterMinMaxUndo: + + def test_min_change_pushes_minmax_command(self, widget_with_model): + """Editing a min bound should push a ParameterMinMaxCommand.""" + w = widget_with_model + min_col = w.lstParams.itemDelegate().param_min + for row in range(w._model_model.rowCount()): + name = w._model_model.item(row, 0).text() + if name == "radius": + w._model_model.item(row, min_col).setText("1.0") + break + + top_cmd = w.undo_stack._undo_stack[-1] + assert isinstance(top_cmd, ParameterMinMaxCommand) + assert top_cmd._param_name == "radius" + assert top_cmd._bound == "min" + assert top_cmd._new_val == 1.0 + + def test_max_change_pushes_minmax_command(self, widget_with_model): + """Editing a max bound should push a ParameterMinMaxCommand.""" + w = widget_with_model + max_col = w.lstParams.itemDelegate().param_max + for row in range(w._model_model.rowCount()): + name = w._model_model.item(row, 0).text() + if name == "radius": + w._model_model.item(row, max_col).setText("500.0") + break + + top_cmd = w.undo_stack._undo_stack[-1] + assert isinstance(top_cmd, ParameterMinMaxCommand) + assert top_cmd._bound == "max" + assert top_cmd._new_val == 500.0 + + +# --------------------------------------------------------------------------- +# TestModelSelectionUndo +# --------------------------------------------------------------------------- + +class TestModelSelectionUndo: + + def test_model_switch_pushes_model_selection_command(self, widget_with_model): + """Changing cbModel should push a ModelSelectionCommand.""" + w = widget_with_model + # Switch to a different model in the same category + model_index = w.cbModel.findText("barbell") + if model_index < 0: + pytest.skip("barbell model not available") + w.cbModel.setCurrentIndex(model_index) + QtWidgets.QApplication.processEvents() + + assert w.undo_stack.can_undo() + top_cmd = w.undo_stack._undo_stack[-1] + assert isinstance(top_cmd, ModelSelectionCommand) + assert "barbell" in top_cmd._new_triple[1] + + def test_model_selection_undo_restores_previous_model(self, widget_with_model): + """Undoing a model switch should restore the previous model type.""" + w = widget_with_model + original_model_type = type(w.logic.kernel_module) + + # Switch to a different model + model_index = w.cbModel.findText("barbell") + if model_index < 0: + pytest.skip("barbell model not available") + w.cbModel.setCurrentIndex(model_index) + QtWidgets.QApplication.processEvents() + + assert type(w.logic.kernel_module) is not original_model_type + w.undo_stack.undo() + QtWidgets.QApplication.processEvents() + # Verify the kernel module type was restored + assert type(w.logic.kernel_module) is original_model_type + + def test_select_default_model_no_command(self, widget_with_model): + """Selecting MODEL_DEFAULT should not push a command.""" + w = widget_with_model + initial_count = len(w.undo_stack._undo_stack) + + default_index = w.cbModel.findText(FittingWidget.MODEL_DEFAULT) + if default_index >= 0: + w.cbModel.setCurrentIndex(default_index) + # Should not push + assert len(w.undo_stack._undo_stack) == initial_count + + +# --------------------------------------------------------------------------- +# TestFitOptionsUndo +# --------------------------------------------------------------------------- + +class TestFitOptionsUndo: + + def test_options_update_pushes_fit_options_command(self, widget_with_model): + """Changing Q-range or npts should push a FitOptionsCommand.""" + w = widget_with_model + + # Modify the Q range via the options widget's model + from sas.qtgui.Perspectives.Fitting.OptionsWidget import OptionsWidget + w._get_fit_options_dict() + # Directly change the widget's model value to simulate user edit + w.options_widget.model.item( + OptionsWidget.MODEL.index('MIN_RANGE') + ).setText("0.01") + QtWidgets.QApplication.processEvents() + + if w.undo_stack.can_undo(): + top_cmd = w.undo_stack._undo_stack[-1] + assert isinstance(top_cmd, FitOptionsCommand) + + def test_options_widget_setState_round_trip(self, widget_with_model): + """OptionsWidget.setState must restore values consistently.""" + w = widget_with_model + ow = w.options_widget + + # Get current state + original = ow.state() + # Set to different values + ow.setState(0.001, 1.0, 200, True, 2) + new_state = ow.state() + assert new_state[0] == pytest.approx(0.001) + assert new_state[1] == pytest.approx(1.0) + assert new_state[2] == 200 + assert new_state[3] is True + assert new_state[4] == 2 + + # Restore original + ow.setState(*original) + restored = ow.state() + assert restored[0] == pytest.approx(original[0]) + assert restored[1] == pytest.approx(original[1]) + assert restored[2] == original[2] + + +# --------------------------------------------------------------------------- +# TestSmearingOptionsUndo +# --------------------------------------------------------------------------- + +class TestSmearingOptionsUndo: + + def test_initial_smearing_state_is_none(self, widget): + """_last_smearing_state should be None until first update.""" + assert widget._last_smearing_state is None + + def test_first_smearing_update_no_command(self, widget_with_model): + """First smearing update should NOT push a command (no prior state).""" + w = widget_with_model + w._last_smearing_state = None + initial_count = len(w.undo_stack._undo_stack) + w.onSmearingOptionsUpdate() + # No command pushed because old_state was None + assert len(w.undo_stack._undo_stack) == initial_count + # But _last_smearing_state should now be populated + assert w._last_smearing_state is not None + + def test_second_smearing_update_pushes_command(self, widget_with_model): + """A smearing change after the first should push SmearingOptionsCommand.""" + w = widget_with_model + # Prime the initial state + w.onSmearingOptionsUpdate() + w.undo_stack.clear() + + # Change smearing — simulate by altering the stored state + old_state = dict(w._last_smearing_state) + # Trigger another update (even if values are the same, verify logic) + w.onSmearingOptionsUpdate() + new_state = w._last_smearing_state + + if old_state != new_state: + assert w.undo_stack.can_undo() + top_cmd = w.undo_stack._undo_stack[-1] + assert isinstance(top_cmd, SmearingOptionsCommand) + + +# --------------------------------------------------------------------------- +# TestCheckboxToggleUndo +# --------------------------------------------------------------------------- + +class TestCheckboxToggleUndo: + + def test_toggle_poly_pushes_checkbox_command(self, widget_with_model): + """Toggling polydispersity should push a CheckboxToggleCommand.""" + w = widget_with_model + w.undo_stack.clear() + + w.chkPolydispersity.setEnabled(True) + w.chkPolydispersity.setChecked(True) + QtWidgets.QApplication.processEvents() + + assert w.undo_stack.can_undo() + top_cmd = w.undo_stack._undo_stack[-1] + assert isinstance(top_cmd, CheckboxToggleCommand) + assert top_cmd._checkbox_id == "chkPolydispersity" + assert top_cmd._new_bool is True + + def test_toggle_poly_undo_unchecks(self, widget_with_model): + """Undoing poly toggle should uncheck the checkbox.""" + w = widget_with_model + w.undo_stack.clear() + + w.chkPolydispersity.setEnabled(True) + w.chkPolydispersity.setChecked(True) + QtWidgets.QApplication.processEvents() + + w.undo_stack.undo() + assert not w.chkPolydispersity.isChecked() + + def test_toggle_magnetism_pushes_checkbox_command(self, widget_with_model): + """Toggling magnetism should push a CheckboxToggleCommand.""" + w = widget_with_model + w.undo_stack.clear() + + w.chkMagnetism.setEnabled(True) + w.chkMagnetism.setChecked(True) + QtWidgets.QApplication.processEvents() + + assert w.undo_stack.can_undo() + top_cmd = w.undo_stack._undo_stack[-1] + assert isinstance(top_cmd, CheckboxToggleCommand) + assert top_cmd._checkbox_id == "chkMagnetism" + + def test_toggle_2d_pushes_checkbox_command(self, widget_with_model): + """Toggling 2D view should push a CheckboxToggleCommand.""" + w = widget_with_model + w.undo_stack.clear() + + w.chk2DView.setEnabled(True) + w.chk2DView.setChecked(True) + QtWidgets.QApplication.processEvents() + + assert w.undo_stack.can_undo() + top_cmd = w.undo_stack._undo_stack[-1] + assert isinstance(top_cmd, CheckboxToggleCommand) + assert top_cmd._checkbox_id == "chk2DView" + + def test_toggle_2d_does_not_push_model_selection(self, widget_with_model): + """toggle2D calls onSelectModel internally — it must be suppressed.""" + w = widget_with_model + w.undo_stack.clear() + + w.chk2DView.setEnabled(True) + w.chk2DView.setChecked(True) + QtWidgets.QApplication.processEvents() + + # Should have exactly one command (CheckboxToggle), not two + # (i.e. the inner onSelectModel must not push its own ModelSelection) + checkbox_cmds = [ + c for c in w.undo_stack._undo_stack + if isinstance(c, CheckboxToggleCommand) + ] + model_cmds = [ + c for c in w.undo_stack._undo_stack + if isinstance(c, ModelSelectionCommand) + ] + assert len(checkbox_cmds) == 1 + assert len(model_cmds) == 0 + + +# --------------------------------------------------------------------------- +# TestFitResultUndo +# --------------------------------------------------------------------------- + +class TestFitResultUndo: + + def _make_fit_result(self, widget): + """Create a minimal fake fit result tuple.""" + res = MagicMock() + res.fitness = 1.5 + res.pvec = [1.0, 2.0, 3.0] + res.stderr = [0.1, 0.2, 0.3] + # Build param_dict that paramDictFromResults would return + param_names = [ + p.name for p in widget.logic.kernel_module._model_info.parameters.kernel_parameters + ] + res.pname = param_names[:len(res.pvec)] + return ([[res]], 0.5) + + def test_fit_complete_pushes_fit_result_command(self, widget_with_model, monkeypatch): + """fitComplete should push a FitResultCommand.""" + w = widget_with_model + w.undo_stack.clear() + + # Mock methods that would crash without real fit data + monkeypatch.setattr(w.polydispersity_widget, 'updatePolyModelFromList', lambda *a, **kw: None) + monkeypatch.setattr(w.magnetism_widget, 'updateMagnetModelFromList', lambda *a, **kw: None) + monkeypatch.setattr(w, 'onPlot', lambda *a, **kw: None) + + # Create a result that paramDictFromResults can handle + old_params = w._get_parameter_dict() + param_dict = {n: (v + 1.0, 0.01) for n, v in old_params.items()} + monkeypatch.setattr( + w.fitting_controller, 'paramDictFromResults', lambda *a, **kw: param_dict + ) + + # Make updateModelFromList actually modify the kernel so new_params != old_params + def fake_update(pd): + for name, (val, _err) in pd.items(): + w.logic.kernel_module.setParam(name, val) + + monkeypatch.setattr( + w.fitting_controller, 'updateModelFromList', fake_update + ) + + result = self._make_fit_result(w) + w.fitComplete(result) + + assert w.undo_stack.can_undo() + top_cmd = w.undo_stack._undo_stack[-1] + assert isinstance(top_cmd, FitResultCommand) + + def test_fit_complete_reenables_undo_stack(self, widget_with_model, monkeypatch): + """fitComplete must re-enable the undo stack (disabled during fit).""" + w = widget_with_model + + monkeypatch.setattr(w.fitting_controller, 'updateModelFromList', lambda *a, **kw: None) + monkeypatch.setattr(w.polydispersity_widget, 'updatePolyModelFromList', lambda *a, **kw: None) + monkeypatch.setattr(w.magnetism_widget, 'updateMagnetModelFromList', lambda *a, **kw: None) + monkeypatch.setattr(w, 'onPlot', lambda *a, **kw: None) + monkeypatch.setattr( + w.fitting_controller, 'paramDictFromResults', + lambda *a, **kw: {n: (v, 0.01) for n, v in w._get_parameter_dict().items()} + ) + + w.undo_stack.set_enabled(False) + result = self._make_fit_result(w) + w.fitComplete(result) + assert w.undo_stack._enabled + + def test_fit_complete_failed_no_command(self, widget_with_model, monkeypatch): + """A failed fit should not push an undo command.""" + w = widget_with_model + w.undo_stack.clear() + + monkeypatch.setattr(w, 'enableInteractiveElements', lambda *a, **kw: None) + w.kernel_module_copy = MagicMock() + + # Simulate failed fit + w.fitComplete(None) + assert not w.undo_stack.can_undo() + + +# --------------------------------------------------------------------------- +# TestUndoStackDisabledDuringFit +# --------------------------------------------------------------------------- + +class TestUndoStackDisabledDuringFit: + + def test_onfit_disables_undo_stack(self, widget_with_model, monkeypatch): + """onFit must disable the undo stack when fitting starts.""" + w = widget_with_model + + monkeypatch.setattr(w.fitting_controller, 'prepareFitters', lambda *a, **kw: ([MagicMock()], 0)) + monkeypatch.setattr(w, 'disableInteractiveElements', lambda *a, **kw: None) + monkeypatch.setattr('twisted.internet.threads.deferToThread', lambda *a, **kw: MagicMock()) + + w.onFit() + assert not w.undo_stack._enabled + + def test_stopfit_reenables_undo_stack(self, widget_with_model, monkeypatch): + """stopFit must re-enable the undo stack.""" + w = widget_with_model + w.calc_fit = MagicMock() + w.calc_fit.isrunning.return_value = True + w.undo_stack.set_enabled(False) + + monkeypatch.setattr(w, 'enableInteractiveElements', lambda *a, **kw: None) + w.stopFit() + assert w.undo_stack._enabled + + +# --------------------------------------------------------------------------- +# TestOptionsWidgetSetState +# --------------------------------------------------------------------------- + +class TestOptionsWidgetSetState: + + def test_setState_updates_q_range(self, widget_with_model): + """setState should update Q range text fields.""" + ow = widget_with_model.options_widget + ow.setState(0.002, 2.0, 300, False, 0) + q_min, q_max, npts, log_pts, weighting = ow.state() + assert q_min == pytest.approx(0.002) + assert q_max == pytest.approx(2.0) + assert npts == 300 + + def test_setState_updates_log_checkbox(self, widget_with_model): + """setState should toggle the log checkbox.""" + ow = widget_with_model.options_widget + ow.setState(0.001, 0.5, 150, True, 0) + assert ow.chkLogData.isChecked() + ow.setState(0.001, 0.5, 150, False, 0) + assert not ow.chkLogData.isChecked() + + def test_setState_updates_weighting(self, widget_with_model): + """setState should update the weighting radio buttons.""" + ow = widget_with_model.options_widget + for w_val in range(4): + ow.setState(0.001, 0.5, 150, False, w_val) + assert ow.weighting == w_val + + def test_setState_does_not_emit_signals(self, widget_with_model): + """setState blocks model signals to avoid feedback loops.""" + ow = widget_with_model.options_widget + signal_received = [] + ow.model.dataChanged.connect(lambda *a: signal_received.append(1)) + ow.setState(0.003, 3.0, 500, True, 1) + assert len(signal_received) == 0 + + +# --------------------------------------------------------------------------- +# TestUndoRedoRoundTrip +# --------------------------------------------------------------------------- + +class TestUndoRedoRoundTrip: + """End-to-end: make a change, undo it, redo it, verify state at each step.""" + + def test_param_value_round_trip(self, widget_with_model): + """Change radius → undo → redo should return to changed value.""" + w = widget_with_model + param_col = w.lstParams.itemDelegate().param_value + for row in range(w._model_model.rowCount()): + if w._model_model.item(row, 0).text() == "radius": + original = w.logic.kernel_module.getParam("radius") + w._model_model.item(row, param_col).setText("42.0") + break + + assert w.logic.kernel_module.getParam("radius") == 42.0 + w.undo_stack.undo() + assert w.logic.kernel_module.getParam("radius") == original + w.undo_stack.redo() + assert w.logic.kernel_module.getParam("radius") == 42.0 + + def test_multiple_undo_redo(self, widget_with_model): + """Multiple parameter edits: undo all, then redo all.""" + w = widget_with_model + param_col = w.lstParams.itemDelegate().param_value + + # Find radius and length rows + radius_row = length_row = None + for row in range(w._model_model.rowCount()): + name = w._model_model.item(row, 0).text() + if name == "radius": + radius_row = row + elif name == "length": + length_row = row + + if radius_row is None or length_row is None: + pytest.skip("Could not find both radius and length parameters") + + original_radius = w.logic.kernel_module.getParam("radius") + original_length = w.logic.kernel_module.getParam("length") + + # Edit radius then length + w._model_model.item(radius_row, param_col).setText("11.0") + w._model_model.item(length_row, param_col).setText("22.0") + + assert w.logic.kernel_module.getParam("radius") == 11.0 + assert w.logic.kernel_module.getParam("length") == 22.0 + + # Undo length + w.undo_stack.undo() + assert w.logic.kernel_module.getParam("length") == original_length + assert w.logic.kernel_module.getParam("radius") == 11.0 + + # Undo radius + w.undo_stack.undo() + assert w.logic.kernel_module.getParam("radius") == original_radius + + # Redo radius + w.undo_stack.redo() + assert w.logic.kernel_module.getParam("radius") == 11.0 + + # Redo length + w.undo_stack.redo() + assert w.logic.kernel_module.getParam("length") == 22.0 + + def test_suppressed_context_prevents_push(self, widget_with_model): + """Changes inside suppressed() must not appear on the stack.""" + w = widget_with_model + param_col = w.lstParams.itemDelegate().param_value + w.undo_stack.clear() + + with w.undo_stack.suppressed(): + for row in range(w._model_model.rowCount()): + if w._model_model.item(row, 0).text() == "radius": + w._model_model.item(row, param_col).setText("77.0") + break + + assert not w.undo_stack.can_undo() + + def test_stackChanged_signal_emitted_on_param_edit(self, widget_with_model): + """stackChanged should fire when a parameter edit pushes a command.""" + w = widget_with_model + received = [] + w.undo_stack.stackChanged.connect(lambda: received.append(1)) + + param_col = w.lstParams.itemDelegate().param_value + for row in range(w._model_model.rowCount()): + if w._model_model.item(row, 0).text() == "radius": + w._model_model.item(row, param_col).setText("55.0") + break + + assert len(received) >= 1 diff --git a/src/sas/qtgui/Perspectives/Fitting/UnitTesting/UndoRedoTest.py b/src/sas/qtgui/Perspectives/Fitting/UnitTesting/UndoRedoTest.py new file mode 100644 index 0000000000..532b6be083 --- /dev/null +++ b/src/sas/qtgui/Perspectives/Fitting/UnitTesting/UndoRedoTest.py @@ -0,0 +1,775 @@ +"""Unit tests for UndoRedo.py — UndoCommand subclasses and UndoStack. + +Tests focus on single-tab fitting scenarios. The FittingWidget dependency +is fully mocked; no real fitting window is opened. + +Test organisation: + TestUndoCommand — abstract base behaviour + TestParameterValueCommand — value change, coalescing + TestParameterMinMaxCommand — bound change + TestModelSelectionCommand — model triple + param restore + TestFitOptionsCommand — options dict round-trip + TestSmearingOptionsCommand — smearing state round-trip + TestCheckboxToggleCommand — checkbox flip + TestFitResultCommand — pre/post fit snapshot + TestCompoundCommand — atomic group, ordering + TestUndoStack — push / undo / redo / depth / suppression + TestUndoStackFailure — failure dialog, reset_to_last_good +""" +import logging +import time +from unittest.mock import MagicMock + +import pytest + +from sas.qtgui.Perspectives.Fitting.UndoRedo import ( + CheckboxToggleCommand, + CompoundCommand, + FitOptionsCommand, + FitResultCommand, + ModelSelectionCommand, + ParameterMinMaxCommand, + ParameterValueCommand, + SmearingOptionsCommand, + UndoCommand, + UndoStack, +) + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + +@pytest.fixture(autouse=True) +def mock_message_box(monkeypatch): + """Suppress all QMessageBox dialogs for the entire test module.""" + mock = MagicMock() + monkeypatch.setattr( + "sas.qtgui.Perspectives.Fitting.UndoRedo.QtWidgets.QMessageBox", mock + ) + return mock + + +@pytest.fixture +def widget(): + """Minimal mock that satisfies the widget protocol used by commands.""" + w = MagicMock() + # kernel_module.details must support real dict operations for MinMax tests + w.logic.kernel_module.details = {} + return w + + +@pytest.fixture +def stack(widget, qapp): + """An UndoStack wired to a mock widget.""" + return UndoStack(widget) + + +# --------------------------------------------------------------------------- +# UndoCommand — abstract base +# --------------------------------------------------------------------------- + +class TestUndoCommand: + + def test_undo_raises_not_implemented(self): + cmd = UndoCommand("test") + with pytest.raises(NotImplementedError): + cmd.undo(None) + + def test_redo_raises_not_implemented(self): + cmd = UndoCommand("test") + with pytest.raises(NotImplementedError): + cmd.redo(None) + + def test_can_merge_false_by_default(self): + cmd = UndoCommand("test") + assert cmd.can_merge(UndoCommand("other")) is False + + def test_merge_raises_not_implemented(self): + cmd = UndoCommand("test") + with pytest.raises(NotImplementedError): + cmd.merge(UndoCommand("other")) + + def test_description_stored(self): + cmd = UndoCommand("my action") + assert cmd.description == "my action" + + def test_timestamp_is_recent(self): + t_before = time.monotonic() + cmd = UndoCommand("t") + t_after = time.monotonic() + assert t_before <= cmd.timestamp <= t_after + + def test_repr_contains_class_and_description(self): + cmd = UndoCommand("hello") + assert "UndoCommand" in repr(cmd) + assert "hello" in repr(cmd) + + +# --------------------------------------------------------------------------- +# ParameterValueCommand +# --------------------------------------------------------------------------- + +class TestParameterValueCommand: + + def test_undo_sets_old_value(self, widget): + cmd = ParameterValueCommand("radius", 5.0, 10.0) + cmd.undo(widget) + widget.logic.kernel_module.setParam.assert_called_once_with("radius", 5.0) + widget._update_model_param_value.assert_called_once_with("radius", 5.0) + + def test_redo_sets_new_value(self, widget): + cmd = ParameterValueCommand("radius", 5.0, 10.0) + cmd.redo(widget) + widget.logic.kernel_module.setParam.assert_called_once_with("radius", 10.0) + widget._update_model_param_value.assert_called_once_with("radius", 10.0) + + def test_can_merge_same_param(self): + cmd1 = ParameterValueCommand("radius", 1.0, 2.0) + cmd2 = ParameterValueCommand("radius", 2.0, 3.0) + assert cmd1.can_merge(cmd2) is True + + def test_cannot_merge_different_param(self): + cmd1 = ParameterValueCommand("radius", 1.0, 2.0) + cmd2 = ParameterValueCommand("length", 1.0, 2.0) + assert cmd1.can_merge(cmd2) is False + + def test_cannot_merge_different_type(self): + cmd1 = ParameterValueCommand("radius", 1.0, 2.0) + assert cmd1.can_merge(UndoCommand("other")) is False + + def test_merge_spans_full_range(self): + cmd1 = ParameterValueCommand("radius", 1.0, 2.0) + cmd2 = ParameterValueCommand("radius", 2.0, 3.0) + merged = cmd1.merge(cmd2) + assert merged._old_val == 1.0 + assert merged._new_val == 3.0 + assert merged.param_name == "radius" + + def test_merge_preserves_earlier_timestamp(self): + cmd1 = ParameterValueCommand("r", 1.0, 2.0) + cmd2 = ParameterValueCommand("r", 2.0, 3.0) + merged = cmd1.merge(cmd2) + assert merged.timestamp == cmd1.timestamp + + def test_param_name_property(self): + cmd = ParameterValueCommand("scale", 1.0, 2.0) + assert cmd.param_name == "scale" + + def test_cannot_merge_stale_same_param_command(self): + """Edits to the same parameter outside the coalescing window must not merge.""" + cmd1 = ParameterValueCommand("radius", 1.0, 2.0) + cmd2 = ParameterValueCommand("radius", 2.0, 3.0) + # Backdate cmd1 so the gap exceeds the coalescing window + cmd1.timestamp = ( + cmd2.timestamp - ParameterValueCommand._COALESCE_WINDOW_SECONDS - 1.0 + ) + assert cmd1.can_merge(cmd2) is False + + +# --------------------------------------------------------------------------- +# ParameterMinMaxCommand +# --------------------------------------------------------------------------- + +class TestParameterMinMaxCommand: + + def test_undo_restores_min(self, widget): + widget.logic.kernel_module.details = {"radius": [None, 0.0, 100.0]} + cmd = ParameterMinMaxCommand("radius", "min", 0.0, 5.0) + cmd.undo(widget) + assert widget.logic.kernel_module.details["radius"][1] == 0.0 + widget._update_model_param_limit.assert_called_once_with( + "radius", "min", 0.0 + ) + + def test_redo_applies_new_min(self, widget): + widget.logic.kernel_module.details = {"radius": [None, 0.0, 100.0]} + cmd = ParameterMinMaxCommand("radius", "min", 0.0, 5.0) + cmd.redo(widget) + assert widget.logic.kernel_module.details["radius"][1] == 5.0 + widget._update_model_param_limit.assert_called_once_with( + "radius", "min", 5.0 + ) + + def test_undo_restores_max(self, widget): + widget.logic.kernel_module.details = {"length": [None, 0.0, 100.0]} + cmd = ParameterMinMaxCommand("length", "max", 100.0, 200.0) + cmd.undo(widget) + assert widget.logic.kernel_module.details["length"][2] == 100.0 + + def test_redo_applies_new_max(self, widget): + widget.logic.kernel_module.details = {"length": [None, 0.0, 100.0]} + cmd = ParameterMinMaxCommand("length", "max", 100.0, 200.0) + cmd.redo(widget) + assert widget.logic.kernel_module.details["length"][2] == 200.0 + + def test_invalid_bound_raises(self): + with pytest.raises(AssertionError): + ParameterMinMaxCommand("r", "middle", 1.0, 2.0) + + +# --------------------------------------------------------------------------- +# ModelSelectionCommand +# --------------------------------------------------------------------------- + +class TestModelSelectionCommand: + + def test_undo_restores_old_triple_and_params(self, widget): + old_triple = ("Shape", "sphere", "None") + new_triple = ("Shape", "cylinder", "None") + cmd = ModelSelectionCommand( + old_triple, new_triple, {"radius": 1.0}, {"length": 5.0} + ) + cmd.undo(widget) + widget._restore_model_selection.assert_called_once_with( + old_triple, {"radius": 1.0} + ) + + def test_redo_restores_new_triple_and_params(self, widget): + old_triple = ("Shape", "sphere", "None") + new_triple = ("Shape", "cylinder", "None") + cmd = ModelSelectionCommand( + old_triple, new_triple, {"radius": 1.0}, {"length": 5.0} + ) + cmd.redo(widget) + widget._restore_model_selection.assert_called_once_with( + new_triple, {"length": 5.0} + ) + + def test_params_are_deep_copied(self): + params = {"radius": 5.0} + cmd = ModelSelectionCommand( + ("A", "B", "C"), ("D", "E", "F"), params, {} + ) + params["radius"] = 999.0 # mutate original + assert cmd._old_params["radius"] == 5.0 # snapshot unchanged + + def test_description_includes_new_model_name(self): + cmd = ModelSelectionCommand( + ("A", "sphere", "C"), ("A", "cylinder", "C"), {}, {} + ) + assert "cylinder" in cmd.description + + +# --------------------------------------------------------------------------- +# FitOptionsCommand +# --------------------------------------------------------------------------- + +class TestFitOptionsCommand: + + def test_undo_applies_old_options(self, widget): + old = {"q_min": 0.01, "q_max": 0.5} + new = {"q_min": 0.05, "q_max": 1.0} + cmd = FitOptionsCommand(old, new) + cmd.undo(widget) + widget._apply_fit_options.assert_called_once_with(old) + + def test_redo_applies_new_options(self, widget): + old = {"q_min": 0.01, "q_max": 0.5} + new = {"q_min": 0.05, "q_max": 1.0} + cmd = FitOptionsCommand(old, new) + cmd.redo(widget) + widget._apply_fit_options.assert_called_once_with(new) + + def test_options_are_deep_copied(self): + opts = {"q_min": 0.01} + cmd = FitOptionsCommand(opts, {}) + opts["q_min"] = 999.0 + assert cmd._old_options["q_min"] == 0.01 + + +# --------------------------------------------------------------------------- +# SmearingOptionsCommand +# --------------------------------------------------------------------------- + +class TestSmearingOptionsCommand: + + def test_undo_applies_old_state(self, widget): + cmd = SmearingOptionsCommand({"type": "none"}, {"type": "pinhole"}) + cmd.undo(widget) + widget._apply_smearing_state.assert_called_once_with({"type": "none"}) + + def test_redo_applies_new_state(self, widget): + cmd = SmearingOptionsCommand({"type": "none"}, {"type": "pinhole"}) + cmd.redo(widget) + widget._apply_smearing_state.assert_called_once_with({"type": "pinhole"}) + + def test_state_is_deep_copied(self): + state = {"type": "none"} + cmd = SmearingOptionsCommand(state, {}) + state["type"] = "changed" + assert cmd._old_state["type"] == "none" + + +# --------------------------------------------------------------------------- +# CheckboxToggleCommand +# --------------------------------------------------------------------------- + +class TestCheckboxToggleCommand: + + def test_undo_sets_old_bool(self, widget): + widget.chkPolydispersity = MagicMock() + cmd = CheckboxToggleCommand("chkPolydispersity", False, True) + cmd.undo(widget) + widget.chkPolydispersity.setChecked.assert_called_once_with(False) + + def test_redo_sets_new_bool(self, widget): + widget.chkPolydispersity = MagicMock() + cmd = CheckboxToggleCommand("chkPolydispersity", False, True) + cmd.redo(widget) + widget.chkPolydispersity.setChecked.assert_called_once_with(True) + + def test_description_includes_checkbox_id(self): + cmd = CheckboxToggleCommand("chkMagnetism", False, True) + assert "chkMagnetism" in cmd.description + + +# --------------------------------------------------------------------------- +# FitResultCommand +# --------------------------------------------------------------------------- + +class TestFitResultCommand: + + def test_undo_restores_pre_fit_params(self, widget): + cmd = FitResultCommand({"radius": 1.0}, {"radius": 2.5}) + cmd.undo(widget) + widget._restore_parameter_values.assert_called_once_with({"radius": 1.0}) + + def test_redo_restores_post_fit_params(self, widget): + cmd = FitResultCommand({"radius": 1.0}, {"radius": 2.5}) + cmd.redo(widget) + widget._restore_parameter_values.assert_called_once_with({"radius": 2.5}) + + def test_params_are_deep_copied(self): + old = {"radius": 1.0} + cmd = FitResultCommand(old, {}) + old["radius"] = 999.0 + assert cmd._old_params["radius"] == 1.0 + + def test_description_is_fit_result(self): + cmd = FitResultCommand({}, {}) + assert cmd.description == "Fit result" + + +# --------------------------------------------------------------------------- +# CompoundCommand +# --------------------------------------------------------------------------- + +class TestCompoundCommand: + + def test_undo_executes_in_reverse_order(self, widget): + order = [] + cmd1 = MagicMock(spec=UndoCommand) + cmd1.undo.side_effect = lambda w: order.append("cmd1") + cmd2 = MagicMock(spec=UndoCommand) + cmd2.undo.side_effect = lambda w: order.append("cmd2") + CompoundCommand([cmd1, cmd2], "c").undo(widget) + assert order == ["cmd2", "cmd1"] + + def test_redo_executes_in_forward_order(self, widget): + order = [] + cmd1 = MagicMock(spec=UndoCommand) + cmd1.redo.side_effect = lambda w: order.append("cmd1") + cmd2 = MagicMock(spec=UndoCommand) + cmd2.redo.side_effect = lambda w: order.append("cmd2") + CompoundCommand([cmd1, cmd2], "c").redo(widget) + assert order == ["cmd1", "cmd2"] + + def test_commands_property_returns_copy(self): + cmd1 = MagicMock(spec=UndoCommand) + compound = CompoundCommand([cmd1], "c") + copy = compound.commands + copy.append(MagicMock()) + assert len(compound.commands) == 1 # original unaffected + + def test_description_falls_back_to_first_command(self): + cmd1 = MagicMock(spec=UndoCommand) + cmd1.description = "Select model" + compound = CompoundCommand([cmd1]) + assert compound.description == "Select model" + + def test_explicit_description_takes_precedence(self): + compound = CompoundCommand([], "Override") + assert compound.description == "Override" + + +# --------------------------------------------------------------------------- +# UndoStack — normal operation +# --------------------------------------------------------------------------- + +def _make_cmd(description: str = "cmd") -> MagicMock: + """Return a MagicMock UndoCommand that disallows coalescing.""" + cmd = MagicMock(spec=UndoCommand) + cmd.can_merge.return_value = False + cmd.description = description + return cmd + + +class TestUndoStack: + + def test_initial_state_empty(self, stack): + assert not stack.can_undo() + assert not stack.can_redo() + assert stack.undo_text() == "Undo" + assert stack.redo_text() == "Redo" + + def test_push_enables_undo(self, stack): + stack.push(_make_cmd()) + assert stack.can_undo() + assert not stack.can_redo() + + def test_undo_moves_to_redo(self, stack, widget): + stack.push(_make_cmd()) + stack.undo() + assert not stack.can_undo() + assert stack.can_redo() + + def test_redo_moves_back_to_undo(self, stack, widget): + stack.push(_make_cmd()) + stack.undo() + stack.redo() + assert stack.can_undo() + assert not stack.can_redo() + + def test_push_after_undo_clears_redo(self, stack, widget): + stack.push(_make_cmd("a")) + stack.undo() + assert stack.can_redo() + stack.push(_make_cmd("b")) + assert not stack.can_redo() + + def test_undo_calls_cmd_undo_with_widget(self, stack, widget): + cmd = _make_cmd() + stack.push(cmd) + stack.undo() + cmd.undo.assert_called_once_with(widget) + + def test_redo_calls_cmd_redo_with_widget(self, stack, widget): + cmd = _make_cmd() + stack.push(cmd) + stack.undo() + stack.redo() + cmd.redo.assert_called_once_with(widget) + + def test_stackChanged_emitted_on_push(self, stack): + received = [] + stack.stackChanged.connect(lambda: received.append(1)) + stack.push(_make_cmd()) + assert len(received) == 1 + + def test_stackChanged_emitted_on_undo(self, stack): + stack.push(_make_cmd()) + received = [] + stack.stackChanged.connect(lambda: received.append(1)) + stack.undo() + assert len(received) == 1 + + def test_stackChanged_emitted_on_redo(self, stack): + stack.push(_make_cmd()) + stack.undo() + received = [] + stack.stackChanged.connect(lambda: received.append(1)) + stack.redo() + assert len(received) == 1 + + def test_stackChanged_emitted_on_clear(self, stack): + stack.push(_make_cmd()) + received = [] + stack.stackChanged.connect(lambda: received.append(1)) + stack.clear() + assert len(received) == 1 + + def test_stackChanged_emitted_on_set_enabled(self, stack): + """set_enabled must emit stackChanged so UI actions refresh.""" + received = [] + stack.stackChanged.connect(lambda: received.append(1)) + stack.set_enabled(False) + assert len(received) == 1 + stack.set_enabled(True) + assert len(received) == 2 + + def test_can_undo_false_when_disabled(self, stack): + """can_undo must return False when the stack is disabled.""" + stack.push(_make_cmd()) + assert stack.can_undo() + stack.set_enabled(False) + assert not stack.can_undo() + # Re-enable: can_undo should return True again + stack.set_enabled(True) + assert stack.can_undo() + + def test_can_redo_false_when_disabled(self, stack, widget): + """can_redo must return False when the stack is disabled.""" + stack.push(_make_cmd()) + stack.undo() + assert stack.can_redo() + stack.set_enabled(False) + assert not stack.can_redo() + stack.set_enabled(True) + assert stack.can_redo() + + def test_max_depth_drops_oldest_entries(self, stack): + stack._max_depth = 3 + cmds = [_make_cmd(f"c{i}") for i in range(5)] + for cmd in cmds: + stack.push(cmd) + assert len(stack._undo_stack) == 3 + # Three newest survive + assert cmds[2] in stack._undo_stack + assert cmds[3] in stack._undo_stack + assert cmds[4] in stack._undo_stack + # Two oldest are gone + assert cmds[0] not in stack._undo_stack + assert cmds[1] not in stack._undo_stack + + def test_clear_empties_both_stacks(self, stack, widget): + stack.push(_make_cmd()) + stack.undo() + stack.clear() + assert not stack.can_undo() + assert not stack.can_redo() + + def test_suppressed_prevents_push(self, stack): + with stack.suppressed(): + stack.push(_make_cmd()) + assert not stack.can_undo() + + def test_suppressed_restores_previous_enabled_state(self, stack): + stack.set_enabled(True) + with stack.suppressed(): + assert not stack._enabled + assert stack._enabled + + def test_suppressed_restores_false_if_was_false(self, stack): + stack.set_enabled(False) + with stack.suppressed(): + pass + assert not stack._enabled # restored to False + + def test_set_enabled_false_prevents_push(self, stack): + stack.set_enabled(False) + stack.push(_make_cmd()) + assert not stack.can_undo() + + def test_disabled_prevents_undo(self, stack, widget): + """A disabled stack must not execute undo.""" + stack.push(_make_cmd()) + stack.set_enabled(False) + stack.undo() + # Command still on internal stack, but can_undo() reports False while disabled + assert len(stack._undo_stack) == 1 + assert not stack.can_undo() + assert not stack.can_redo() + + def test_disabled_prevents_redo(self, stack, widget): + """A disabled stack must not execute redo.""" + stack.push(_make_cmd()) + stack.undo() + stack.set_enabled(False) + stack.redo() + # Command still on internal redo stack, but can_redo() reports False while disabled + assert not stack.can_undo() + assert not stack.can_redo() + assert len(stack._redo_stack) == 1 + + def test_replaying_prevents_recursive_push(self, stack): + """Commands pushed during undo replay must be silently dropped.""" + inner = _make_cmd("inner") + + def undo_side_effect(w): + stack.push(inner) # simulate handler firing during replay + + outer = _make_cmd("outer") + outer.undo.side_effect = undo_side_effect + stack.push(outer) + stack.undo() + # outer moved to redo; inner was blocked → undo stack empty + assert not stack.can_undo() + inner.undo.assert_not_called() + + def test_coalescing_merges_same_param_commands(self, stack): + cmd1 = ParameterValueCommand("radius", 1.0, 2.0) + cmd2 = ParameterValueCommand("radius", 2.0, 3.0) + stack.push(cmd1) + stack.push(cmd2) + assert len(stack._undo_stack) == 1 + merged = stack._undo_stack[0] + assert merged._old_val == 1.0 + assert merged._new_val == 3.0 + + def test_no_coalescing_for_different_params(self, stack): + stack.push(ParameterValueCommand("radius", 1.0, 2.0)) + stack.push(ParameterValueCommand("length", 1.0, 2.0)) + assert len(stack._undo_stack) == 2 + + def test_undo_text_includes_description(self, stack): + stack.push(_make_cmd("Change radius")) + assert stack.undo_text() == "Undo Change radius" + + def test_redo_text_includes_description(self, stack, widget): + stack.push(_make_cmd("Change radius")) + stack.undo() + assert stack.redo_text() == "Redo Change radius" + + def test_undo_noop_when_empty(self, stack): + stack.undo() # must not raise + + def test_redo_noop_when_empty(self, stack): + stack.redo() # must not raise + + def test_max_depth_read_from_config(self, qapp): + """UndoStack reads UNDO_STACK_MAX_DEPTH from the SasView config.""" + from sas import config as sas_config + assert hasattr(sas_config, "UNDO_STACK_MAX_DEPTH") + widget = MagicMock() + s = UndoStack(widget) + assert s._max_depth == sas_config.UNDO_STACK_MAX_DEPTH + + def test_successful_undo_auto_snapshots_widget_state(self, stack, widget): + """A successful undo must save _last_good_state from the widget.""" + widget._get_parameter_dict.return_value = {"scale": 1.0} + stack.push(_make_cmd()) + assert stack._last_good_state is None + stack.undo() + assert stack._last_good_state == {"scale": 1.0} + + def test_successful_redo_auto_snapshots_widget_state(self, stack, widget): + """A successful redo must save _last_good_state from the widget.""" + stack.push(_make_cmd()) + stack.undo() + stack._last_good_state = None # reset so we can detect the auto-save + widget._get_parameter_dict.return_value = {"scale": 2.0} + stack.redo() + assert stack._last_good_state == {"scale": 2.0} + + def test_auto_snapshot_silent_if_no_get_parameter_dict(self, stack, widget): + """Missing _get_parameter_dict on the widget must not raise.""" + widget._get_parameter_dict.side_effect = AttributeError("no such method") + stack.push(_make_cmd()) + stack.undo() # must not raise + assert stack._last_good_state is None + + +# --------------------------------------------------------------------------- +# UndoStack — failure resilience +# --------------------------------------------------------------------------- + +class TestUndoStackFailure: + + @pytest.fixture + def failing_undo_cmd(self): + cmd = _make_cmd("failing command") + cmd.undo.side_effect = RuntimeError("simulated undo failure") + return cmd + + @pytest.fixture + def failing_redo_cmd(self): + cmd = _make_cmd("failing command") + cmd.redo.side_effect = RuntimeError("simulated redo failure") + return cmd + + def test_undo_failure_logs_warning( + self, stack, failing_undo_cmd, caplog + ): + stack.push(failing_undo_cmd) + with caplog.at_level( + logging.WARNING, + logger="sas.qtgui.Perspectives.Fitting.UndoRedo", + ): + stack.undo() + assert any("undo failed" in m.lower() for m in caplog.messages) + + def test_redo_failure_logs_warning( + self, stack, widget, failing_redo_cmd, caplog + ): + good = _make_cmd("good") + stack.push(good) + stack.undo() + # Replace redo stack entry with a failing one + stack._redo_stack[-1] = failing_redo_cmd + with caplog.at_level( + logging.WARNING, + logger="sas.qtgui.Perspectives.Fitting.UndoRedo", + ): + stack.redo() + assert any("redo failed" in m.lower() for m in caplog.messages) + + def test_failure_increments_consecutive_counter( + self, stack, failing_undo_cmd + ): + stack.push(failing_undo_cmd) + stack.undo() + assert stack._consecutive_failures == 1 + + def test_undo_failure_preserves_undo_stack(self, stack, failing_undo_cmd): + """A failing undo must leave the command on the undo stack.""" + stack.push(failing_undo_cmd) + stack.undo() + assert stack.can_undo() # command still in undo history + assert not stack.can_redo() # nothing moved to redo + + def test_redo_failure_preserves_redo_stack(self, stack, widget, failing_redo_cmd): + """A failing redo must leave the command on the redo stack.""" + stack.push(_make_cmd("good")) + stack.undo() + stack._redo_stack[-1] = failing_redo_cmd + stack.redo() + assert not stack.can_undo() # nothing moved to undo + assert stack.can_redo() # command still in redo history + + def test_success_resets_consecutive_counter(self, stack, widget): + stack._consecutive_failures = 5 + stack.push(_make_cmd()) + stack.undo() + assert stack._consecutive_failures == 0 + + def test_failure_emits_stackChanged(self, stack, failing_undo_cmd): + stack.push(failing_undo_cmd) + received = [] + stack.stackChanged.connect(lambda: received.append(1)) + stack.undo() + assert len(received) == 1 # stackChanged fired even on failure + + def test_save_and_reset_to_last_good_state(self, stack, widget): + stack.save_last_good_state({"radius": 5.0}) + stack.reset_to_last_good() + widget._restore_parameter_values.assert_called_once_with({"radius": 5.0}) + + def test_reset_without_snapshot_logs_warning(self, stack, caplog): + with caplog.at_level( + logging.WARNING, + logger="sas.qtgui.Perspectives.Fitting.UndoRedo", + ): + stack.reset_to_last_good() + assert any("no snapshot" in m.lower() for m in caplog.messages) + + def test_save_last_good_state_copies_dict(self, stack): + state = {"radius": 1.0} + stack.save_last_good_state(state) + state["radius"] = 999.0 + assert stack._last_good_state["radius"] == 1.0 + + def test_failure_dialog_shown(self, stack, failing_undo_cmd, mock_message_box): + stack.push(failing_undo_cmd) + stack.undo() + mock_message_box.assert_called_once() + + def test_repeated_failures_offer_reset_button( + self, stack, widget, mock_message_box + ): + """After 2 consecutive failures with a snapshot, reset button appears.""" + stack.save_last_good_state({"radius": 1.0}) + stack._consecutive_failures = 1 # pre-seed counter + + failing = _make_cmd("fail again") + failing.undo.side_effect = RuntimeError("boom") + stack.push(failing) + stack.undo() + + # addButton called with "Reset to Last Good State" + instance = mock_message_box.return_value + button_labels = [ + call.args[0] + for call in instance.addButton.call_args_list + if call.args + ] + assert any("Reset" in label for label in button_labels) diff --git a/src/sas/qtgui/Perspectives/perspective.py b/src/sas/qtgui/Perspectives/perspective.py index a3d0800a67..977807af57 100644 --- a/src/sas/qtgui/Perspectives/perspective.py +++ b/src/sas/qtgui/Perspectives/perspective.py @@ -196,5 +196,13 @@ def save_parameters(self): """ Save parameters to a file""" pass + @property + def undo_stack(self): + """Optional undo stack for this perspective. + + Perspectives without undo support should leave this as ``None``. + """ + return None + diff --git a/src/sas/qtgui/Utilities/Preferences/GeneralPreferencesWidget.py b/src/sas/qtgui/Utilities/Preferences/GeneralPreferencesWidget.py new file mode 100644 index 0000000000..35067f9028 --- /dev/null +++ b/src/sas/qtgui/Utilities/Preferences/GeneralPreferencesWidget.py @@ -0,0 +1,33 @@ +from PySide6.QtWidgets import QHBoxLayout, QLabel, QSpinBox + +from sas.system import config + +from .PreferencesWidget import PreferencesWidget + + +class GeneralPreferencesWidget(PreferencesWidget): + def __init__(self): + super().__init__("General Settings") + self.config_params = ['UNDO_STACK_MAX_DEPTH'] + + def _addAllWidgets(self): + layout = QHBoxLayout() + label = QLabel("Undo History Depth: ", self) + layout.addWidget(label) + + self.undoDepthSpinner = QSpinBox(self) + self.undoDepthSpinner.setMinimum(10) + self.undoDepthSpinner.setMaximum(1000) + self.undoDepthSpinner.setValue(config.UNDO_STACK_MAX_DEPTH) + layout.addWidget(self.undoDepthSpinner) + + self.verticalLayout.addLayout(layout) + + self.undoDepthSpinner.valueChanged.connect( + lambda val: self._stageChange('UNDO_STACK_MAX_DEPTH', val)) + + def _toggleBlockAllSignaling(self, toggle): + self.undoDepthSpinner.blockSignals(toggle) + + def _restoreFromConfig(self): + self.undoDepthSpinner.setValue(config.UNDO_STACK_MAX_DEPTH) diff --git a/src/sas/qtgui/Utilities/Preferences/PreferencesPanel.py b/src/sas/qtgui/Utilities/Preferences/PreferencesPanel.py index 29b5445163..b1bf29faca 100644 --- a/src/sas/qtgui/Utilities/Preferences/PreferencesPanel.py +++ b/src/sas/qtgui/Utilities/Preferences/PreferencesPanel.py @@ -18,11 +18,13 @@ # `BASE_PANELS = {"Bar Widget Options": BarWidget}` # PreferenceWidget Imports go here and then are added to the BASE_PANELS, but not instantiated. from .DisplayPreferencesWidget import DisplayPreferencesWidget +from .GeneralPreferencesWidget import GeneralPreferencesWidget from .PlottingPreferencesWidget import PlottingPreferencesWidget # Pre-made option widgets -BASE_PANELS = {"Plotting Settings": PlottingPreferencesWidget, +BASE_PANELS = {"General Settings": GeneralPreferencesWidget, + "Plotting Settings": PlottingPreferencesWidget, "Display Settings": DisplayPreferencesWidget, } # Type: Dict[str, Union[Type[PreferencesWidget], Callable[[],QWidget]] ConfigType = str | bool | float | int | list[str | float | int] diff --git a/src/sas/system/config/config.py b/src/sas/system/config/config.py index 57c8d33144..912ae11e37 100644 --- a/src/sas/system/config/config.py +++ b/src/sas/system/config/config.py @@ -205,6 +205,9 @@ def __init__(self): # Default fitting optimizer self.FITTING_DEFAULT_OPTIMIZER = 'lm' + # Undo/Redo stack depth per fitting tab + self.UNDO_STACK_MAX_DEPTH = 200 + # What's New variables self.LAST_WHATS_NEW_HIDDEN_VERSION = "6.0.1" From 5438d3aaa82dfba837194bfce7aef4998371e6c4 Mon Sep 17 00:00:00 2001 From: Piotr Rozyczko Date: Fri, 20 Mar 2026 17:20:21 +0100 Subject: [PATCH 2/2] fixed fitting undo. Fixed unit test --- src/sas/qtgui/MainWindow/GuiManager.py | 1 + .../Perspectives/Fitting/FittingWidget.py | 32 ++- .../qtgui/Perspectives/Fitting/UndoRedo.py | 7 +- .../Fitting/UnitTesting/FittingWidgetTest.py | 3 +- .../UnitTesting/UndoRedoIntegrationTest.py | 193 +++++++++++++++++- src/sas/qtgui/Utilities/GuiUtils.py | 3 + 6 files changed, 211 insertions(+), 28 deletions(-) diff --git a/src/sas/qtgui/MainWindow/GuiManager.py b/src/sas/qtgui/MainWindow/GuiManager.py index f325cf7f60..e1f98c8f58 100644 --- a/src/sas/qtgui/MainWindow/GuiManager.py +++ b/src/sas/qtgui/MainWindow/GuiManager.py @@ -688,6 +688,7 @@ def addCallbacks(self): self.communicate.plotFromNameSignal.connect(self.showPlotFromName) self.communicate.updateModelFromDataOperationPanelSignal.connect(self.updateModelFromDataOperationPanel) self.communicate.activeGraphsSignal.connect(self.updatePlotItems) + self.communicate.undoRedoUpdateSignal.connect(self._update_undo_redo_actions) def addTriggers(self): diff --git a/src/sas/qtgui/Perspectives/Fitting/FittingWidget.py b/src/sas/qtgui/Perspectives/Fitting/FittingWidget.py index 1eec71304c..720c346de2 100644 --- a/src/sas/qtgui/Perspectives/Fitting/FittingWidget.py +++ b/src/sas/qtgui/Perspectives/Fitting/FittingWidget.py @@ -1576,7 +1576,7 @@ def onFit(self) -> None: # Disable some elements self.disableInteractiveElements() - self.undo_stack.set_enabled(False) + # self.undo_stack.set_enabled(False) def stopFit(self) -> None: """ @@ -1587,10 +1587,11 @@ def stopFit(self) -> None: self.calc_fit.stop() #re-enable the Fit button self.enableInteractiveElements() - self.undo_stack.set_enabled(True) + # self.undo_stack.set_enabled(True) msg = "Fitting cancelled." self.communicate.statusBarUpdateSignal.emit(msg) + self.communicate.undoRedoUpdateSignal.emit() def updateFit(self) -> None: """ @@ -1682,7 +1683,7 @@ def fitComplete(self, result: tuple) -> None: """ #re-enable the Fit button self.enableInteractiveElements() - self.undo_stack.set_enabled(True) + # self.undo_stack.set_enabled(True) if not result or not result[0] or not result[0][0]: msg = "Fitting failed." @@ -1691,8 +1692,8 @@ def fitComplete(self, result: tuple) -> None: self.kernel_module = copy.deepcopy(self.kernel_module_copy) return - # Capture parameter snapshot BEFORE fit results are applied - old_params = self._get_parameter_dict() + # # Capture parameter snapshot BEFORE fit results are applied + self.old_params = self._get_parameter_dict(self.kernel_module_copy) # Don't recalculate chi2 - it's in res.fitness already self.fitResults = True @@ -1735,8 +1736,14 @@ def fitComplete(self, result: tuple) -> None: # Push a single FitResultCommand for the entire fit new_params = self._get_parameter_dict() - if old_params != new_params: - self.undo_stack.push(FitResultCommand(old_params, new_params)) + #if old_params != new_params: + self.undo_stack.push(FitResultCommand(self.old_params, new_params)) + + # Ensure undo/redo action state is refreshed in the GUI manager. + # The push above emits stackChanged, but if the per-stack signal + # connection was disrupted (e.g. tab change during processEvents + # in onPlot), this communicator signal provides a reliable fallback. + self.communicate.undoRedoUpdateSignal.emit() def prepareFitters(self, fitter: Fit | None = None, fit_id: int = 0, weight_increase: int = 1) -> tuple[list[Fit], int]: @@ -3175,6 +3182,7 @@ def _update_model_param_value(self, param_name: str, value: float) -> None: value_item = self._model_model.item(row, col) if value_item: value_item.setText(GuiUtils.formatNumber(value, high=True)) + # print(f"Updated {param_name} to {value} in model model.") return def _update_model_param_limit(self, param_name: str, bound: str, value: float) -> None: @@ -3246,13 +3254,15 @@ def _restore_parameter_values(self, params: dict) -> None: self._update_model_param_value(name, value) self.calculateQGridForModel() - def _get_parameter_dict(self) -> dict: + def _get_parameter_dict(self, kernel_module=None) -> dict: """Return ``{param_name: float_value}`` for all current kernel params.""" - if self.logic.kernel_module is None: + if kernel_module is None: + kernel_module = self.logic.kernel_module + if kernel_module is None: return {} return { - p.name: self.logic.kernel_module.getParam(p.name) - for p in self.logic.kernel_module._model_info.parameters.kernel_parameters + p.name: kernel_module.getParam(p.name) + for p in kernel_module._model_info.parameters.kernel_parameters } def _get_fit_options_dict(self) -> dict: diff --git a/src/sas/qtgui/Perspectives/Fitting/UndoRedo.py b/src/sas/qtgui/Perspectives/Fitting/UndoRedo.py index e039246ec9..9df21bfb3f 100644 --- a/src/sas/qtgui/Perspectives/Fitting/UndoRedo.py +++ b/src/sas/qtgui/Perspectives/Fitting/UndoRedo.py @@ -3,8 +3,6 @@ Provides UndoCommand (abstract base), concrete subclasses for each kind of undoable action, and UndoStack which manages per-tab history. -Phase 1 of UNDO_PLAN_CLAUDE.md — core infrastructure only. -Integration with FittingWidget is done in Phase 2. Design notes: - Each command stores only old_value + new_value (delta, not snapshot). @@ -14,7 +12,6 @@ parameter are merged into one entry (Qt fires dataChanged on commit, not per keystroke, providing the natural coalescing boundary). - Parameter 'fit' checkbox toggles are intentionally NOT tracked - (see UNDO_PLAN_CLAUDE.md, Decisions). """ from __future__ import annotations @@ -248,7 +245,6 @@ class CheckboxToggleCommand(UndoCommand): (e.g. ``"chkPolydispersity"``). Note: parameter 'fit' checkbox toggles are intentionally NOT tracked. - See UNDO_PLAN_CLAUDE.md, Decisions for rationale. """ def __init__(self, checkbox_id: str, old_bool: bool, new_bool: bool) -> None: @@ -271,8 +267,7 @@ class FitResultCommand(UndoCommand): """Full parameter snapshot before and after a fit. ``old_params`` MUST be captured at the very start of ``fitComplete()``, - before ``updateModelFromList()`` is called (see UNDO_PLAN_CLAUDE.md, - Step 2.6 — Critical ordering). + before ``updateModelFromList()`` is called. Delegates to ``widget._restore_parameter_values(params)`` (added in Phase 2). diff --git a/src/sas/qtgui/Perspectives/Fitting/UnitTesting/FittingWidgetTest.py b/src/sas/qtgui/Perspectives/Fitting/UnitTesting/FittingWidgetTest.py index bbac42fc53..aa4faecea2 100644 --- a/src/sas/qtgui/Perspectives/Fitting/UnitTesting/FittingWidgetTest.py +++ b/src/sas/qtgui/Perspectives/Fitting/UnitTesting/FittingWidgetTest.py @@ -328,10 +328,11 @@ def testSelectModel(self, widget, mocker): # Observe calculateQGridForModel called assert widget.calculateQGridForModel.called - def testSelectFactor(self, widget): + def testSelectFactor(self, widget, mocker): """ Assure proper behaviour on changing structure factor """ + mocker.patch.object(widget, 'calculateQGridForModel') widget.show() # Change the category index so we have some models category_index = widget.cbCategory.findText("Cylinder") diff --git a/src/sas/qtgui/Perspectives/Fitting/UnitTesting/UndoRedoIntegrationTest.py b/src/sas/qtgui/Perspectives/Fitting/UnitTesting/UndoRedoIntegrationTest.py index e3dc10dc7e..89ccb305aa 100644 --- a/src/sas/qtgui/Perspectives/Fitting/UnitTesting/UndoRedoIntegrationTest.py +++ b/src/sas/qtgui/Perspectives/Fitting/UnitTesting/UndoRedoIntegrationTest.py @@ -507,8 +507,10 @@ def _make_fit_result(self, widget): def test_fit_complete_pushes_fit_result_command(self, widget_with_model, monkeypatch): """fitComplete should push a FitResultCommand.""" + import copy w = widget_with_model w.undo_stack.clear() + w.kernel_module_copy = copy.deepcopy(w.logic.kernel_module) # Mock methods that would crash without real fit data monkeypatch.setattr(w.polydispersity_widget, 'updatePolyModelFromList', lambda *a, **kw: None) @@ -538,9 +540,11 @@ def fake_update(pd): top_cmd = w.undo_stack._undo_stack[-1] assert isinstance(top_cmd, FitResultCommand) - def test_fit_complete_reenables_undo_stack(self, widget_with_model, monkeypatch): - """fitComplete must re-enable the undo stack (disabled during fit).""" + def test_fit_complete_keeps_undo_stack_enabled(self, widget_with_model, monkeypatch): + """fitComplete should leave the undo stack enabled.""" w = widget_with_model + import copy + w.kernel_module_copy = copy.deepcopy(w.logic.kernel_module) monkeypatch.setattr(w.fitting_controller, 'updateModelFromList', lambda *a, **kw: None) monkeypatch.setattr(w.polydispersity_widget, 'updatePolyModelFromList', lambda *a, **kw: None) @@ -551,7 +555,6 @@ def test_fit_complete_reenables_undo_stack(self, widget_with_model, monkeypatch) lambda *a, **kw: {n: (v, 0.01) for n, v in w._get_parameter_dict().items()} ) - w.undo_stack.set_enabled(False) result = self._make_fit_result(w) w.fitComplete(result) assert w.undo_stack._enabled @@ -573,10 +576,10 @@ def test_fit_complete_failed_no_command(self, widget_with_model, monkeypatch): # TestUndoStackDisabledDuringFit # --------------------------------------------------------------------------- -class TestUndoStackDisabledDuringFit: +class TestUndoStackDuringFit: - def test_onfit_disables_undo_stack(self, widget_with_model, monkeypatch): - """onFit must disable the undo stack when fitting starts.""" + def test_onfit_leaves_undo_stack_enabled(self, widget_with_model, monkeypatch): + """onFit should leave the undo stack enabled (suppressed() handles blocking).""" w = widget_with_model monkeypatch.setattr(w.fitting_controller, 'prepareFitters', lambda *a, **kw: ([MagicMock()], 0)) @@ -584,14 +587,13 @@ def test_onfit_disables_undo_stack(self, widget_with_model, monkeypatch): monkeypatch.setattr('twisted.internet.threads.deferToThread', lambda *a, **kw: MagicMock()) w.onFit() - assert not w.undo_stack._enabled + assert w.undo_stack._enabled - def test_stopfit_reenables_undo_stack(self, widget_with_model, monkeypatch): - """stopFit must re-enable the undo stack.""" + def test_stopfit_keeps_undo_stack_enabled(self, widget_with_model, monkeypatch): + """stopFit should leave the undo stack enabled.""" w = widget_with_model w.calc_fit = MagicMock() w.calc_fit.isrunning.return_value = True - w.undo_stack.set_enabled(False) monkeypatch.setattr(w, 'enableInteractiveElements', lambda *a, **kw: None) w.stopFit() @@ -637,6 +639,177 @@ def test_setState_does_not_emit_signals(self, widget_with_model): assert len(signal_received) == 0 +# --------------------------------------------------------------------------- +# TestGuiManagerUndoHookSignalChain +# --------------------------------------------------------------------------- + +class TestGuiManagerUndoHookSignalChain: + """Verify that the signal chain from UndoStack.stackChanged through + to the GuiManager-style handler actually enables the undo action, + reproducing the real wiring done in GuiManager._connect_undo_redo_hooks.""" + + def _make_fit_result(self, widget): + """Create a minimal fake fit result tuple.""" + res = MagicMock() + res.fitness = 1.5 + res.pvec = [1.0, 2.0, 3.0] + res.stderr = [0.1, 0.2, 0.3] + param_names = [ + p.name for p in widget.logic.kernel_module._model_info.parameters.kernel_parameters + ] + res.pname = param_names[:len(res.pvec)] + return ([[res]], 0.5) + + def test_stack_changed_calls_handler_on_push(self, widget_with_model): + """stackChanged should fire when push() is called.""" + w = widget_with_model + handler_calls = [] + w.undo_stack.stackChanged.connect(lambda: handler_calls.append(True)) + from sas.qtgui.Perspectives.Fitting.UndoRedo import ParameterValueCommand + w.undo_stack.push(ParameterValueCommand("scale", 1.0, 2.0)) + assert len(handler_calls) >= 1 + + def test_fit_complete_triggers_action_enable_via_signal_chain( + self, widget_with_model, monkeypatch + ): + """Simulate what GuiManager does: connect stackChanged to a handler + that reads _active_undo_stack() and calls actionUndo.setEnabled. + Then run fitComplete and verify the action ends up enabled.""" + import copy + w = widget_with_model + w.undo_stack.clear() + w.kernel_module_copy = copy.deepcopy(w.logic.kernel_module) + + # --- simulate GuiManager's mock workspace action --- + action_undo = MagicMock() + action_redo = MagicMock() + + # GuiManager's _active_undo_stack() equivalent: just return w.undo_stack + def active_stack(): + return w.undo_stack + + def update_undo_redo_actions(): + stack = active_stack() + if stack is None: + action_undo.setEnabled(False) + action_redo.setEnabled(False) + return + action_undo.setEnabled(stack.can_undo()) + action_redo.setEnabled(stack.can_redo()) + + # wire up exactly as GuiManager._connect_undo_redo_hooks + w.undo_stack.stackChanged.connect(update_undo_redo_actions) + + # --- prepare fit result (same as TestFitResultUndo) --- + monkeypatch.setattr( + w.polydispersity_widget, 'updatePolyModelFromList', lambda *a, **kw: None + ) + monkeypatch.setattr( + w.magnetism_widget, 'updateMagnetModelFromList', lambda *a, **kw: None + ) + monkeypatch.setattr(w, 'onPlot', lambda *a, **kw: None) + + old_params = w._get_parameter_dict() + param_dict = {n: (v + 1.0, 0.01) for n, v in old_params.items()} + monkeypatch.setattr( + w.fitting_controller, 'paramDictFromResults', lambda *a, **kw: param_dict + ) + + def fake_update(pd): + for name, (val, _err) in pd.items(): + w.logic.kernel_module.setParam(name, val) + + monkeypatch.setattr(w.fitting_controller, 'updateModelFromList', fake_update) + + # reset mock so we can check only calls after fitComplete + action_undo.reset_mock() + action_redo.reset_mock() + + # --- run fitComplete --- + result = self._make_fit_result(w) + w.fitComplete(result) + + # The final call to setEnabled should be True + assert action_undo.setEnabled.call_args_list[-1] == ((True,),) + assert w.undo_stack.can_undo() + + def test_fitting_window_property_indirection(self, widget_with_model, monkeypatch): + """Reproduce how GuiManager connects: get the stack via the + FittingWindow.undo_stack property (which delegates to + currentWidget().undo_stack), connect stackChanged, then simulate + fitComplete on the widget. The property-returned stack should be + the exact same object as the widget's undo_stack so the emit is + received.""" + from sas.qtgui.Perspectives.Fitting.FittingPerspective import FittingWindow + + class _DummyMgr: + def communicator(self): + return GuiUtils.Communicate() + communicate = GuiUtils.Communicate() + + fw = FittingWindow(_DummyMgr()) + # FittingWindow.__init__ already called addFit(None), so there is one tab. + # Grab the widget from the first (and only) tab. + w = fw.currentWidget() + assert w is not None + + # Load a model so the widget has a kernel_module + category_index = w.cbCategory.findText("Cylinder") + w.cbCategory.setCurrentIndex(category_index) + model_index = w.cbModel.findText("cylinder") + w.cbModel.setCurrentIndex(model_index) + QtWidgets.QApplication.processEvents() + w.undo_stack.clear() + + # ---- mimic GuiManager._connect_undo_redo_hooks ---- + # _active_undo_stack() goes through the FittingWindow property + stack_via_property = fw.undo_stack + assert stack_via_property is w.undo_stack, ( + "Property indirection must return the same stack object" + ) + + action_undo = MagicMock() + + def _update(): + s = fw.undo_stack # called each time, just like GuiManager + if s is None: + action_undo.setEnabled(False) + return + action_undo.setEnabled(s.can_undo()) + + stack_via_property.stackChanged.connect(_update) + + # ---- prepare and run fitComplete (same boilerplate) ---- + import copy + w.kernel_module_copy = copy.deepcopy(w.logic.kernel_module) + monkeypatch.setattr(w.polydispersity_widget, 'updatePolyModelFromList', lambda *a, **kw: None) + monkeypatch.setattr(w.magnetism_widget, 'updateMagnetModelFromList', lambda *a, **kw: None) + monkeypatch.setattr(w, 'onPlot', lambda *a, **kw: None) + + old_params = w._get_parameter_dict() + param_dict = {n: (v + 1.0, 0.01) for n, v in old_params.items()} + monkeypatch.setattr(w.fitting_controller, 'paramDictFromResults', lambda *a, **kw: param_dict) + + def fake_update(pd): + for name, (val, _err) in pd.items(): + w.logic.kernel_module.setParam(name, val) + + monkeypatch.setattr(w.fitting_controller, 'updateModelFromList', fake_update) + + action_undo.reset_mock() + + result = self._make_fit_result(w) + w.fitComplete(result) + + assert action_undo.setEnabled.call_args_list[-1] == ((True,),), ( + f"Expected final setEnabled(True), got calls: {action_undo.setEnabled.call_args_list}" + ) + + # Cleanup + fw.close() + del fw + + # --------------------------------------------------------------------------- # TestUndoRedoRoundTrip # --------------------------------------------------------------------------- diff --git a/src/sas/qtgui/Utilities/GuiUtils.py b/src/sas/qtgui/Utilities/GuiUtils.py index c7bebbbceb..26aa3bf092 100644 --- a/src/sas/qtgui/Utilities/GuiUtils.py +++ b/src/sas/qtgui/Utilities/GuiUtils.py @@ -183,6 +183,9 @@ class Communicate(QtCore.QObject): # Notify about a data name to be frozen and send to fitting perspective freezeDataNameSignal = QtCore.Signal(str) + # Request refresh of undo/redo action enabled state + undoRedoUpdateSignal = QtCore.Signal() + communicate = Communicate()