Refactor adding new batches in ISIS Reflectometry GUI (#38758)

* Refactor adding new batches

* Fix Cpp Check error

buildconfig/CMake/CppCheck_Suppressions.txt.in

@@ -766,7 +766,6 @@ missingOverride:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/G
 missingOverride:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/Preview/PreviewModel.h:31
 missingOverride:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/Runs/IRunsView.h:66
 missingOverride:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/Runs/QtCatalogSearcher.h:32
-constVariableReference:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/Runs/RunsPresenter.cpp:545
 missingOverride:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/RunsTable/IRunsTableView.h:31
 constVariablePointer:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/RunsTable/RunsTablePresenter.cpp:510
 constVariableReference:${CMAKE_SOURCE_DIR}/qt/scientific_interfaces/ISISReflectometry/GUI/RunsTable/RunsTablePresenter.cpp:749

qt/scientific_interfaces/ISISReflectometry/GUI/Batch/BatchPresenter.cpp

@@ -72,8 +72,8 @@ BatchPresenter::BatchPresenter(
  */
 void BatchPresenter::acceptMainPresenter(IMainWindowPresenter *mainPresenter) { m_mainPresenter = mainPresenter; }
 
-void BatchPresenter::initInstrumentList(const std::string &selectedInstrument) {
-  m_runsPresenter->initInstrumentList(selectedInstrument);
+std::string BatchPresenter::initInstrumentList(const std::string &selectedInstrument) {
+  return m_runsPresenter->initInstrumentList(selectedInstrument);
 }
 
 bool BatchPresenter::requestClose() const { return true; }

qt/scientific_interfaces/ISISReflectometry/GUI/Batch/BatchPresenter.h

@@ -59,7 +59,7 @@ class MANTIDQT_ISISREFLECTOMETRY_DLL BatchPresenter : public IBatchPresenter,
 
   // IBatchPresenter overrides
   void acceptMainPresenter(IMainWindowPresenter *mainPresenter) override;
-  void initInstrumentList(const std::string &selectedInstrument = "") override;
+  std::string initInstrumentList(const std::string &selectedInstrument = "") override;
   void notifyPauseReductionRequested() override;
   void notifyResumeReductionRequested() override;
   void notifyResumeAutoreductionRequested() override;

qt/scientific_interfaces/ISISReflectometry/GUI/Batch/IBatchPresenter.h

@@ -32,7 +32,7 @@ class IBatchPresenter {
   virtual ~IBatchPresenter() = default;
 
   virtual void acceptMainPresenter(IMainWindowPresenter *mainPresenter) = 0;
-  virtual void initInstrumentList(const std::string &selectedInstrument = "") = 0;
+  virtual std::string initInstrumentList(const std::string &selectedInstrument = "") = 0;
 
   virtual void notifyPauseReductionRequested() = 0;
   virtual void notifyResumeReductionRequested() = 0;

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

@@ -120,16 +120,7 @@ void MainWindowPresenter::notifyAnyBatchReductionPaused() {
 // Top level function to handle when user has requested to change the
 // instrument
 void MainWindowPresenter::notifyChangeInstrumentRequested(std::string const &newInstrumentName) {
-  // Cache changed state before calling updateInstrument
-  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();
+  processInstrumentSelection(newInstrumentName);
 }
 
 void MainWindowPresenter::notifyCloseEvent() {
@@ -259,8 +250,8 @@ void MainWindowPresenter::optionsChanged() const {
 }
 
 void MainWindowPresenter::addNewBatch(IBatchView *batchView) {
-  // Remember the instrument name so we can re-set it (it will otherwise
-  // get overridden by the instrument list default in the new batch)
+  // Remember the instrument name, if we have one, so that we can re-set it
+  // (it will otherwise get overridden by the instrument list default in the new batch).
   auto const instrument = instrumentName();
   m_batchPresenters.emplace_back(m_batchPresenterFactory->make(batchView));
   m_batchPresenters.back()->acceptMainPresenter(this);
@@ -269,9 +260,10 @@ void MainWindowPresenter::addNewBatch(IBatchView *batchView) {
 
 void MainWindowPresenter::initNewBatch(IBatchPresenter *batchPresenter, std::string const &instrument,
                                        std::optional<int> precision) {
+  // For the very first batch, the selected instrument will be the default from the instrument list
+  auto const selectedInstrument = batchPresenter->initInstrumentList(instrument);
+  processInstrumentSelection(selectedInstrument, batchPresenter);
 
-  batchPresenter->initInstrumentList(instrument);
-  batchPresenter->notifyInstrumentChanged(instrument);
   if (precision.has_value())
     batchPresenter->notifySetRoundPrecision(precision.value());
 
@@ -383,4 +375,23 @@ void MainWindowPresenter::onInstrumentChanged() {
   m_slitCalculator->setCurrentInstrumentName(instrumentName());
   m_slitCalculator->processInstrumentHasBeenChanged();
 }
+
+void MainWindowPresenter::processInstrumentSelection(std::string const &selectedInstrument,
+                                                     IBatchPresenter *batchPresenter) {
+  // Cache changed state before calling updateInstrument
+  auto const hasChanged = (selectedInstrument != 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(selectedInstrument);
+
+  if (hasChanged) {
+    // Perform updates for all batches. Only do this if the instrument has changed to avoid
+    // overriding any user-specified settings
+    onInstrumentChanged();
+  } else if (batchPresenter) {
+    // Perform updates on the specified batch
+    batchPresenter->notifyInstrumentChanged(selectedInstrument);
+  }
+}
 } // namespace MantidQt::CustomInterfaces::ISISReflectometry

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

@@ -112,6 +112,7 @@ class MANTIDQT_ISISREFLECTOMETRY_DLL MainWindowPresenter : public MainWindowSubs
   void updateInstrument(const std::string &instrumentName);
   void setDefaultInstrument(const std::string &newInstrument);
   void onInstrumentChanged();
+  void processInstrumentSelection(std::string const &selectedInstrument, IBatchPresenter *batchPresenter = nullptr);
 
   void disableSaveAndLoadBatch();
   void enableSaveAndLoadBatch();

qt/scientific_interfaces/ISISReflectometry/GUI/Runs/IRunsPresenter.h

@@ -24,7 +24,7 @@ class IRunsPresenter {
 public:
   virtual ~IRunsPresenter() = default;
   virtual void acceptMainPresenter(IBatchPresenter *mainPresenter) = 0;
-  virtual void initInstrumentList(const std::string &selectedInstrument = "") = 0;
+  virtual std::string initInstrumentList(const std::string &selectedInstrument = "") = 0;
   virtual RunsTable const &runsTable() const = 0;
   virtual RunsTable &mutableRunsTable() = 0;
 

qt/scientific_interfaces/ISISReflectometry/GUI/Runs/QtRunsView.cpp

@@ -13,6 +13,7 @@
 #include "MantidQtWidgets/Common/QtAlgorithmRunner.h"
 #include <QMenu>
 #include <QMessageBox>
+#include <QSignalBlocker>
 
 namespace MantidQt::CustomInterfaces::ISISReflectometry {
 using namespace Mantid::API;
@@ -168,14 +169,12 @@ void QtRunsView::setInstrumentList(const std::vector<std::string> &instruments,
   // We block signals while populating the list and setting the selected instrument because adding the first item
   // will trigger a currentIndexChanged signal. This causes existing batch settings to be overwritten when we're
   // initialising a new batch for an instrument that isn't the first in the list.
-  m_ui.comboSearchInstrument->blockSignals(true);
+  const QSignalBlocker blocker(m_ui.comboSearchInstrument);
+
   m_ui.comboSearchInstrument->clear();
   for (auto &&instrument : instruments)
     m_ui.comboSearchInstrument->addItem(QString::fromStdString(instrument));
   setSearchInstrument(selectedInstrument);
-  m_ui.comboSearchInstrument->blockSignals(false);
-  // Manually emit the signal once we've finished setting up the dropdown.
-  emit m_ui.comboSearchInstrument->currentIndexChanged(m_ui.comboSearchInstrument->currentIndex());
 }
 
 /**

qt/scientific_interfaces/ISISReflectometry/GUI/Runs/RunsPresenter.cpp

@@ -72,8 +72,13 @@ RunsPresenter::~RunsPresenter() {
  */
 void RunsPresenter::acceptMainPresenter(IBatchPresenter *mainPresenter) { m_mainPresenter = mainPresenter; }
 
-void RunsPresenter::initInstrumentList(const std::string &selectedInstrument) {
+/** Initialise the list of available instruments.
+ * @param selectedInstrument : Optional name of an instrument to try and select from the list.
+ * @returns The name of the instrument that is selected.
+ */
+std::string RunsPresenter::initInstrumentList(const std::string &selectedInstrument) {
   m_view->setInstrumentList(m_instruments, selectedInstrument);
+  return m_view->getSearchInstrument();
 }
 
 RunsTable const &RunsPresenter::runsTable() const { return tablePresenter()->runsTable(); }
@@ -542,7 +547,7 @@ IAlgorithm_sptr RunsPresenter::setupLiveDataMonitorAlgorithm() {
   auto errorMap = alg->validateInputs();
   if (!errorMap.empty()) {
     std::string errorString;
-    for (auto &kvp : errorMap)
+    for (auto const &kvp : errorMap)
       errorString.append(kvp.first + ":" + kvp.second);
     handleError(errorString);
     return nullptr;

qt/scientific_interfaces/ISISReflectometry/GUI/Runs/RunsPresenter.h

@@ -67,7 +67,7 @@ class MANTIDQT_ISISREFLECTOMETRY_DLL RunsPresenter : public IRunsPresenter,
 
   // IRunsPresenter overrides
   void acceptMainPresenter(IBatchPresenter *mainPresenter) override;
-  void initInstrumentList(const std::string &selectedInstrument = "") override;
+  std::string initInstrumentList(const std::string &selectedInstrument = "") override;
   RunsTable const &runsTable() const override;
   RunsTable &mutableRunsTable() override;
   bool isProcessing() const override;

qt/scientific_interfaces/ISISReflectometry/test/Batch/BatchPresenterTest.h

@@ -64,8 +64,9 @@ class BatchPresenterTest : public CxxTest::TestSuite {
 
   void testInitInstrumentListUpdatesRunsPresenter() {
     auto presenter = makePresenter(makeModel());
-    EXPECT_CALL(*m_runsPresenter, initInstrumentList(_)).Times(1);
-    presenter->initInstrumentList();
+    std::string const selectedInstrument = "INTER";
+    EXPECT_CALL(*m_runsPresenter, initInstrumentList(selectedInstrument)).Times(1).WillOnce(Return(selectedInstrument));
+    TS_ASSERT_EQUALS(presenter->initInstrumentList(selectedInstrument), selectedInstrument);
   }
 
   void testMainPresenterUpdatedWhenChangeInstrumentRequested() {