Merge pull request #28444 from gemmaguest/28429_fix_incomplete_load_on_refl_gui

Fix bugs with restoring defaults on the reflectometry GUI

docs/source/release/v5.1.0/reflectometry.rst

@@ -9,4 +9,16 @@ Reflectometry Changes
     putting new features at the top of the section, followed by
     improvements, followed by bug fixes.
 
-:ref:`Release 5.1.0 <v5.1.0>`
+ISIS Reflectometry Interface
\ No newline at end of file
+############################
+
+Bug fixes
+---------
+
+- Save/Load settings: A bug has been fixed where Experiment/Instrument settings were not being restored if the instrument changes on load.
+- 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>`

qt/scientific_interfaces/ISISReflectometry/GUI/Common/Decoder.cpp

@@ -77,14 +77,18 @@ void Decoder::decodeBatch(const IMainWindowView *mwv, int batchIndex,
   auto runsTablePresenter =
       dynamic_cast<RunsTablePresenter *>(runsPresenter->m_tablePresenter.get());
   auto reductionJobs = &runsTablePresenter->m_model.m_reductionJobs;
+  // We must do the Runs tab first because this sets the instrument, which
+  // other settings may need to be correct. There is also a notification to set
+  // defaults for this instrument so we need to do that before other settings
+  // or it will override them.
+  decodeRuns(gui->m_runs.get(), reductionJobs, runsTablePresenter,
+             map[QString("runsView")].toMap());
   decodeEvent(gui->m_eventHandling.get(), map[QString("eventView")].toMap());
   decodeExperiment(gui->m_experiment.get(),
                    map[QString("experimentView")].toMap());
   decodeInstrument(gui->m_instrument.get(),
                    map[QString("instrumentView")].toMap());
   decodeSave(gui->m_save.get(), map[QString("saveView")].toMap());
-  decodeRuns(gui->m_runs.get(), reductionJobs, runsTablePresenter,
-             map[QString("runsView")].toMap());
 }
 
 void Decoder::decodeExperiment(const QtExperimentView *gui,

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() {

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; }

qt/scientific_interfaces/ISISReflectometry/GUI/MainWindow/MainWindowPresenter.cpp

@@ -125,9 +125,16 @@ void MainWindowPresenter::notifyAnyBatchReductionPaused() {
 }
 
 void MainWindowPresenter::notifyChangeInstrumentRequested(
-    std::string const &instrumentName) {
-  // Re-load instrument with the new name
-  updateInstrument(instrumentName);
+    std::string const &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() {
@@ -245,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(
@@ -272,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

qt/scientific_interfaces/ISISReflectometry/GUI/MainWindow/MainWindowPresenter.h

@@ -88,9 +88,9 @@ private:
   void addNewBatch(IBatchView *batchView);
   void initNewBatch(IBatchPresenter *batchPresenter,
                     std::string const &instrument);
-  void changeInstrument(std::string const &instrumentName);
   void updateInstrument(const std::string &instrumentName);
   void setDefaultInstrument(const std::string &newInstrument);
+  void onInstrumentChanged();
 
   void disableSaveAndLoadBatch();
   void enableSaveAndLoadBatch();
@@ -101,4 +101,4 @@ private:
 };
 } // namespace ISISReflectometry
 } // namespace CustomInterfaces
-} // namespace MantidQt
+} // namespace MantidQt
\ No newline at end of file

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();
   }
@@ -316,32 +328,16 @@ public:
 
   void testSaveBatch() {
     auto presenter = makePresenter();
-    auto const filename = std::string("test.json");
-    auto const map = QMap<QString, QVariant>();
     auto const batchIndex = 1;
-    EXPECT_CALL(m_messageHandler, askUserForSaveFileName("JSON (*.json)"))
-        .Times(1)
-        .WillOnce(Return(filename));
-    EXPECT_CALL(*m_encoder, encodeBatch(&m_view, batchIndex, false))
-        .Times(1)
-        .WillOnce(Return(map));
-    EXPECT_CALL(m_fileHandler, saveJSONToFile(filename, map)).Times(1);
+    expectBatchIsSavedToFile(batchIndex);
     presenter.notifySaveBatchRequested(batchIndex);
     verifyAndClear();
   }
 
   void testLoadBatch() {
     auto presenter = makePresenter();
-    auto const filename = std::string("test.json");
-    auto const map = QMap<QString, QVariant>();
     auto const batchIndex = 1;
-    EXPECT_CALL(m_messageHandler, askUserForLoadFileName("JSON (*.json)"))
-        .Times(1)
-        .WillOnce(Return(filename));
-    EXPECT_CALL(m_fileHandler, loadJSONFromFile(filename))
-        .Times(1)
-        .WillOnce(Return(map));
-    EXPECT_CALL(*m_decoder, decodeBatch(&m_view, batchIndex, map)).Times(1);
+    expectBatchIsLoadedFromFile(batchIndex);
     presenter.notifyLoadBatchRequested(batchIndex);
     verifyAndClear();
   }
@@ -489,6 +485,30 @@ private:
     EXPECT_CALL(*m_slitCalculator, processInstrumentHasBeenChanged()).Times(1);
   }
 
+  void expectBatchIsSavedToFile(int batchIndex) {
+    auto const filename = std::string("test.json");
+    auto const map = QMap<QString, QVariant>();
+    EXPECT_CALL(m_messageHandler, askUserForSaveFileName("JSON (*.json)"))
+        .Times(1)
+        .WillOnce(Return(filename));
+    EXPECT_CALL(*m_encoder, encodeBatch(&m_view, batchIndex, false))
+        .Times(1)
+        .WillOnce(Return(map));
+    EXPECT_CALL(m_fileHandler, saveJSONToFile(filename, map)).Times(1);
+  }
+
+  void expectBatchIsLoadedFromFile(int batchIndex) {
+    auto const filename = std::string("test.json");
+    auto const map = QMap<QString, QVariant>();
+    EXPECT_CALL(m_messageHandler, askUserForLoadFileName("JSON (*.json)"))
+        .Times(1)
+        .WillOnce(Return(filename));
+    EXPECT_CALL(m_fileHandler, loadJSONFromFile(filename))
+        .Times(1)
+        .WillOnce(Return(map));
+    EXPECT_CALL(*m_decoder, decodeBatch(&m_view, batchIndex, map)).Times(1);
+  }
+
   void assertFirstBatchWasRemovedFromModel(
       MainWindowPresenterFriend const &presenter) {
     TS_ASSERT_EQUALS(presenter.m_batchPresenters.size(), 1);