From 3c888dd82275862fbcfe0934012eccf600075d5d Mon Sep 17 00:00:00 2001
From: Gemma Guest <gemma.guest@stfc.ac.uk>
Date: Fri, 27 Mar 2020 11:54:25 +0000
Subject: [PATCH] Fix a bug where restoring defaults affects all batches
Clicking restore-defaults on an Experiment or Instrument tab in a batch should only affect that tab and batch. This commit fixes a bug where a notification was sent to all batches, and every Experiment and Instrument tab was being reset. The fix is to separate updating the instrument (which is required to ensure we have up-to-date settings loaded from the instrument) from the subsequent notification which updates all batches. Instead, the particular Experiment/Instrument tab where the request was made restores its own defaults itself.
Re #28429
---
docs/source/release/v5.1.0/reflectometry.rst | 5 +++-
.../GUI/Experiment/ExperimentPresenter.cpp | 2 +-
.../GUI/Instrument/InstrumentPresenter.cpp | 2 +-
.../GUI/MainWindow/MainWindowPresenter.cpp | 30 ++++++++++++-------
.../GUI/MainWindow/MainWindowPresenter.h | 3 +-
.../MainWindow/MainWindowPresenterTest.h | 18 +++++++++--
6 files changed, 42 insertions(+), 18 deletions(-)
diff --git a/docs/source/release/v5.1.0/reflectometry.rst b/docs/source/release/v5.1.0/reflectometry.rst
index a71d0666d39..0be9fb8adc5 100644
--- a/docs/source/release/v5.1.0/reflectometry.rst
+++ b/docs/source/release/v5.1.0/reflectometry.rst
@@ -16,6 +16,9 @@ Bug fixes
---------
- Save/Load settings: A bug has been fixed where Experiment/Instrument settings were not being restored if the instrument changes on load.
-- New Batch loses settings: A bug has been fixed where creating a new Batch would result in the Experiment/Instrument settings of all batches being reset to their defaults.
+- Lost settings on New Batch and Restore Defaults:
+
+ - A bug has been fixed where creating a new Batch would result in the Experiment/Instrument settings of all batches being reset to their defaults.
+ - A bug has been fixed where clicking Restore Defaults on an Experiment/Instrument tab would cause all Experiment and Instrument tabs in every batch to be reset to defaults. Now, only the tab where you click Restore Defaults is changed.
:ref:`Release 5.1.0 <v5.1.0>`
diff --git a/qt/scientific_interfaces/ISISReflectometry/GUI/Experiment/ExperimentPresenter.cpp b/qt/scientific_interfaces/ISISReflectometry/GUI/Experiment/ExperimentPresenter.cpp
index da2ad7b330a..61e6016cc89 100644
--- a/qt/scientific_interfaces/ISISReflectometry/GUI/Experiment/ExperimentPresenter.cpp
+++ b/qt/scientific_interfaces/ISISReflectometry/GUI/Experiment/ExperimentPresenter.cpp
@@ -42,8 +42,8 @@ void ExperimentPresenter::notifySettingsChanged() {
void ExperimentPresenter::notifyRestoreDefaultsRequested() {
// Trigger a reload of the instrument to get up-to-date settings.
- // After the instrument is updated, the defaults will be restored.
m_mainPresenter->notifyUpdateInstrumentRequested();
+ restoreDefaults();
}
void ExperimentPresenter::notifySummationTypeChanged() {
diff --git a/qt/scientific_interfaces/ISISReflectometry/GUI/Instrument/InstrumentPresenter.cpp b/qt/scientific_interfaces/ISISReflectometry/GUI/Instrument/InstrumentPresenter.cpp
index c886b80071b..0efe747cc9d 100644
--- a/qt/scientific_interfaces/ISISReflectometry/GUI/Instrument/InstrumentPresenter.cpp
+++ b/qt/scientific_interfaces/ISISReflectometry/GUI/Instrument/InstrumentPresenter.cpp
@@ -45,8 +45,8 @@ void InstrumentPresenter::notifySettingsChanged() {
void InstrumentPresenter::notifyRestoreDefaultsRequested() {
// Trigger a reload of the instrument to get up-to-date settings.
- // After the instrument is updated, the defaults will be restored.
m_mainPresenter->notifyUpdateInstrumentRequested();
+ restoreDefaults();
}
Instrument const &InstrumentPresenter::instrument() const { return m_model; }
diff --git a/qt/scientific_interfaces/ISISReflectometry/GUI/MainWindow/MainWindowPresenter.cpp b/qt/scientific_interfaces/ISISReflectometry/GUI/MainWindow/MainWindowPresenter.cpp
index da2c9aff0f6..c6fe9441d61 100644
--- a/qt/scientific_interfaces/ISISReflectometry/GUI/MainWindow/MainWindowPresenter.cpp
+++ b/qt/scientific_interfaces/ISISReflectometry/GUI/MainWindow/MainWindowPresenter.cpp
@@ -126,9 +126,15 @@ void MainWindowPresenter::notifyAnyBatchReductionPaused() {
void MainWindowPresenter::notifyChangeInstrumentRequested(
std::string const &newInstrumentName) {
- // Re-load instrument with the new name, if it has changed
- if (newInstrumentName != instrumentName())
- updateInstrument(newInstrumentName);
+ auto const hasChanged = (newInstrumentName != instrumentName());
+ // Re-load instrument regardless of whether it has changed, e.g. if we are
+ // creating a new batch the instrument may not have changed but we still want
+ // the most up to date settings
+ updateInstrument(newInstrumentName);
+ // However, only perform updates if the instrument has changed, otherwise we
+ // may trigger overriding of user-specified settings
+ if (hasChanged)
+ onInstrumentChanged();
}
void MainWindowPresenter::notifyUpdateInstrumentRequested() {
@@ -246,14 +252,6 @@ void MainWindowPresenter::updateInstrument(const std::string &instrumentName) {
loadAlg->execute();
MatrixWorkspace_sptr instWorkspace = loadAlg->getProperty("OutputWorkspace");
m_instrument = instWorkspace->getInstrument();
-
- // Notify child presenters
- for (auto &batchPresenter : m_batchPresenters)
- batchPresenter->notifyInstrumentChanged(instrumentName);
-
- // Notify the slit calculator
- m_slitCalculator->setCurrentInstrumentName(instrumentName);
- m_slitCalculator->processInstrumentHasBeenChanged();
}
void MainWindowPresenter::setDefaultInstrument(
@@ -273,6 +271,16 @@ void MainWindowPresenter::setDefaultInstrument(
g_log.notice() << "Instrument changed to " << requiredInstrument;
}
}
+
+void MainWindowPresenter::onInstrumentChanged() {
+ // Notify child presenters
+ for (auto &batchPresenter : m_batchPresenters)
+ batchPresenter->notifyInstrumentChanged(instrumentName());
+
+ // Notify the slit calculator
+ m_slitCalculator->setCurrentInstrumentName(instrumentName());
+ m_slitCalculator->processInstrumentHasBeenChanged();
+}
} // namespace ISISReflectometry
} // namespace CustomInterfaces
} // namespace MantidQt
diff --git a/qt/scientific_interfaces/ISISReflectometry/GUI/MainWindow/MainWindowPresenter.h b/qt/scientific_interfaces/ISISReflectometry/GUI/MainWindow/MainWindowPresenter.h
index 7e276e8adeb..b07f6ee488a 100644
--- a/qt/scientific_interfaces/ISISReflectometry/GUI/MainWindow/MainWindowPresenter.h
+++ b/qt/scientific_interfaces/ISISReflectometry/GUI/MainWindow/MainWindowPresenter.h
@@ -90,6 +90,7 @@ private:
std::string const &instrument);
void updateInstrument(const std::string &instrumentName);
void setDefaultInstrument(const std::string &newInstrument);
+ void onInstrumentChanged();
void disableSaveAndLoadBatch();
void enableSaveAndLoadBatch();
@@ -100,4 +101,4 @@ private:
};
} // namespace ISISReflectometry
} // namespace CustomInterfaces
-} // namespace MantidQt
\ No newline at end of file
+} // namespace MantidQt
diff --git a/qt/scientific_interfaces/test/ISISReflectometry/MainWindow/MainWindowPresenterTest.h b/qt/scientific_interfaces/test/ISISReflectometry/MainWindow/MainWindowPresenterTest.h
index f2c2b6aba0b..464a01a69d2 100644
--- a/qt/scientific_interfaces/test/ISISReflectometry/MainWindow/MainWindowPresenterTest.h
+++ b/qt/scientific_interfaces/test/ISISReflectometry/MainWindow/MainWindowPresenterTest.h
@@ -252,6 +252,7 @@ public:
void testChangeInstrumentRequestedUpdatesInstrumentInChildPresenters() {
auto presenter = makePresenter();
+ setupInstrument(presenter, "INTER");
auto const instrument = std::string("POLREF");
EXPECT_CALL(*m_batchPresenters[0], notifyInstrumentChanged(instrument))
.Times(1);
@@ -261,13 +262,24 @@ public:
verifyAndClear();
}
- void testUpdateInstrumentUpdatesInstrumentInChildPresenters() {
+ void testChangeInstrumentRequestedDoesNotUpdateInstrumentIfNotChanged() {
auto presenter = makePresenter();
auto const instrument = setupInstrument(presenter, "POLREF");
EXPECT_CALL(*m_batchPresenters[0], notifyInstrumentChanged(instrument))
- .Times(1);
+ .Times(0);
EXPECT_CALL(*m_batchPresenters[1], notifyInstrumentChanged(instrument))
- .Times(1);
+ .Times(0);
+ presenter.notifyChangeInstrumentRequested(instrument);
+ verifyAndClear();
+ }
+
+ void testUpdateInstrumentDoesNotUpdateInstrumentInChildPresenters() {
+ auto presenter = makePresenter();
+ auto const instrument = setupInstrument(presenter, "POLREF");
+ EXPECT_CALL(*m_batchPresenters[0], notifyInstrumentChanged(instrument))
+ .Times(0);
+ EXPECT_CALL(*m_batchPresenters[1], notifyInstrumentChanged(instrument))
+ .Times(0);
presenter.notifyUpdateInstrumentRequested();
verifyAndClear();
}
--
GitLab