Refs #8279 - Fix crash and undesired behaviour.

Ultimately, the problems were caused as a result of code I wrote nearly
two years ago.  It was some of the first C++ and Mantid code I ever did,
and it really does show.  Overengineered in places and underengineered in
others, redundant code, useless abstractions, and poor use of common
idioms.  Just awful.

I'd love a chance to go back and redo it, and probably make it a third of
the size in the process.

In the meantime, I've made the following changes:

* Correctly filtered the workspaces that have been selected by the user to
exclude non-MatrixWorkspaces.  This means including the children of
WorkspaceGroups and excluding WorkspaceGroups themselves.

* Correctly find the common spectra IDs between selected workspaces.

* Only offer the user the chance to select spectra IDs to plot if there are
indeed common IDs between all selected workspaces.  Otherwise just allow
selection of common workspace indices.

* If workspaces only share a single common workspace index (and don't
share any common spectra IDs), or if all workspaces only have a single
spectrum, then just go ahead and plot them.  No need to ask the user at
this point.

Code/Mantid/MantidPlot/src/Mantid/MantidDock.cpp

@@ -9,13 +9,18 @@
 
 #include <MantidAPI/AlgorithmFactory.h>
 #include <MantidAPI/FileProperty.h>
+#include <MantidAPI/WorkspaceGroup.h>
 #include <MantidGeometry/MDGeometry/IMDDimension.h>
 #include <MantidGeometry/Crystal/OrientedLattice.h>
 #include <MantidQtAPI/InterfaceManager.h>
 #include <MantidQtAPI/Message.h>
 
+#include <boost/assign/list_of.hpp>
+
 #include <Poco/Path.h>
 
+#include <algorithm>
+
 using namespace Mantid::API;
 using namespace Mantid::Kernel;
 using namespace Mantid::Geometry;
@@ -1221,55 +1226,83 @@ QStringList MantidTreeWidget::getSelectedWorkspaceNames() const
   return names;
 }
 
-/** Allows the user to select a spectrum from the selected workspaces.
-*  Automatically chooses spectrum 0 if all are single-spectrum workspaces.
-*  @return A map of workspace name - spectrum index pairs
-*/
+/**
+ * Allows users to choose spectra from the selected workspaces by presenting them
+ * with a dialog box.  Skips showing the dialog box and automatically chooses
+ * workspace index 0 for all selected workspaces if one or more of the them are
+ * single-spectrum workspaces.
+ *
+ * We also must filter the list of selected workspace names to account for any
+ * non-MatrixWorkspaces that may have been selected.  In particular WorkspaceGroups
+ * (the children of which are to be included if they are MatrixWorkspaces) and
+ * TableWorkspaces (which are implicitly excluded).  We only want workspaces we
+ * can actually plot!
+ *
+ * @return :: A map of workspace name to spectrum numbers to plot.
+ */
 QMultiMap<QString,std::set<int> > MantidTreeWidget::chooseSpectrumFromSelected() const
 {
-  // Get hold of the names of all the selected workspaces
-  QList<QString> allWsNames = this->getSelectedWorkspaceNames();
-  QList<QString> wsNames;
-
-  for (int i=0; i<allWsNames.size(); i++)
+  // Check for any selected WorkspaceGroup names and replace with the names of
+  // their children.
+  QSet<QString> selectedWsNames;
+  foreach( const QString wsName, this->getSelectedWorkspaceNames() )
   {
-    if (m_ads.retrieve(allWsNames[i].toStdString())->id() != "TableWorkspace")
-      wsNames.append(allWsNames[i]);
+    const auto groupWs = boost::dynamic_pointer_cast<const WorkspaceGroup>(m_ads.retrieve(wsName.toStdString()));
+    if( groupWs )
+    {
+      const auto childWsNames = groupWs->getNames();
+      for( auto childWsName = childWsNames.begin(); childWsName != childWsNames.end(); ++childWsName )
+      {
+        selectedWsNames.insert(QString::fromStdString(*childWsName));
+      }
+    }
+    else
+    {
+      selectedWsNames.insert(wsName);
+    }
   }
 
-  // cppcheck-suppress redundantAssignment
-  QList<QString>::const_iterator it = wsNames.constBegin();
-
-  // Check to see if all workspaces have a *single* histogram ...
-  QList<size_t> wsSizes;
-  it = wsNames.constBegin();
-  size_t maxHists = 0;
-  for ( ; it != wsNames.constEnd(); ++it )
+  // Get the names of, and pointers to, the MatrixWorkspaces only.
+  QList<MatrixWorkspace_const_sptr> selectedMatrixWsList;
+  QList<QString> selectedMatrixWsNameList;
+  foreach( const auto selectedWsName, selectedWsNames )
   {
-    MatrixWorkspace_const_sptr ws = boost::dynamic_pointer_cast<const MatrixWorkspace>(m_ads.retrieve((*it).toStdString()));
-    if ( !ws ) continue;
-    const size_t currentHists = ws->getNumberHistograms();
-    wsSizes.append(currentHists);
-    if ( currentHists > maxHists ) maxHists = currentHists;
+    const auto matrixWs = boost::dynamic_pointer_cast<const MatrixWorkspace>(m_ads.retrieve(selectedWsName.toStdString()));
+    if( matrixWs )
+    {
+      selectedMatrixWsList.append(matrixWs);
+      selectedMatrixWsNameList.append(QString::fromStdString(matrixWs->name()));
+    }
   }
 
-  QMultiMap<QString,std::set<int> > toPlot;
-
-  // ... if so, no need to ask user which one to plot - just go!
-  if(maxHists == 1)
+  // Check to see if all workspaces have only a single spectrum ...
+  bool allSingleWorkspaces = true;
+  foreach( const auto selectedMatrixWs, selectedMatrixWsList )
   {
-    it = wsNames.constBegin();
-    for ( ; it != wsNames.constEnd(); ++it )
+    if( selectedMatrixWs->getNumberHistograms() != 1 )
     {
-      std::set<int> zero;
-      zero.insert(0);
-      toPlot.insert((*it),zero);
+      allSingleWorkspaces = false;
+      break;
     }
+  }
 
-    return toPlot;
+  // ... and if so, just return all workspace names mapped to workspace index 0;
+  if( allSingleWorkspaces )
+  {
+    const std::set<int> SINGLE_SPECTRUM = boost::assign::list_of<int>(0);
+    QMultiMap<QString,std::set<int>> spectrumToPlot;
+    foreach( const auto selectedMatrixWs, selectedMatrixWsList )
+    {
+      spectrumToPlot.insert(
+        QString::fromStdString(selectedMatrixWs->name()),
+        SINGLE_SPECTRUM
+      );
+    }
+    return spectrumToPlot;
   }
 
-  MantidWSIndexDialog *dio = new MantidWSIndexDialog(m_mantidUI, 0, wsNames);
+  // Else, one or more workspaces 
+  MantidWSIndexDialog *dio = new MantidWSIndexDialog(m_mantidUI, 0, selectedMatrixWsNameList);
   dio->exec();
   return dio->getPlots();
 }

Code/Mantid/MantidPlot/src/Mantid/MantidWSIndexDialog.cpp

@@ -127,7 +127,7 @@ void MantidWSIndexDialog::plotAll()
 
 void MantidWSIndexDialog::editedWsField()
 {
-  if(m_spectra) {
+  if(usingSpectraIDs()) {
     m_spectraField->lineEdit()->clear();
     m_spectraField->setError(""); 
   }
@@ -178,7 +178,9 @@ void MantidWSIndexDialog::initSpectraBox()
   m_spectraBox->add(m_spectraMessage);
   m_spectraBox->add(m_spectraField);
   m_spectraBox->add(m_orMessage);
-  if(m_spectra) m_outer->addItem(m_spectraBox);
+
+  if( usingSpectraIDs() )
+    m_outer->addItem(m_spectraBox);
 
   connect(m_spectraField->lineEdit(), SIGNAL(textEdited(const QString &)), this, SLOT(editedSpectraField()));
 }
@@ -187,7 +189,7 @@ void MantidWSIndexDialog::initButtons()
 {
   m_buttonBox = new QHBoxLayout;
 
-  m_okButton = new QPushButton("Ok");
+  m_okButton = new QPushButton("OK");
   m_cancelButton = new QPushButton("Cancel");
   m_plotAllButton = new QPushButton("Plot All");
 
@@ -256,55 +258,37 @@ void MantidWSIndexDialog::generateWsIndexIntervals()
 
 void MantidWSIndexDialog::generateSpectraIdIntervals()
 {
-  // Get the list of available intervals for each of the workspaces, and then
-  // present the user with intervals which are the INTERSECTION of each of
-  // those lists of intervals.
-  QList<QString>::const_iterator it = m_wsNames.constBegin();
-
-  // Cycle through the workspaces ...
-  for ( ; it != m_wsNames.constEnd(); ++it )
+  bool firstWs = true;
+  foreach( const QString wsName, m_wsNames )
   {
-    Mantid::API::MatrixWorkspace_const_sptr ws = boost::dynamic_pointer_cast<const Mantid::API::MatrixWorkspace>(Mantid::API::AnalysisDataService::Instance().retrieve((*it).toStdString()));
-    if ( NULL == ws ) continue;
+    Mantid::API::MatrixWorkspace_const_sptr ws = boost::dynamic_pointer_cast<const Mantid::API::MatrixWorkspace>(Mantid::API::AnalysisDataService::Instance().retrieve(wsName.toStdString()));
+    if( !ws ) continue; // Belt and braces.
+
     const Mantid::spec2index_map spec2index = ws->getSpectrumToWorkspaceIndexMap();
-
-    Mantid::spec2index_map::const_iterator last = spec2index.end();
-    --last;
-    Mantid::spec2index_map::const_iterator first = spec2index.begin();
-
-    const int startSpectrum = static_cast<int> (first->first);
-    const int endSpectrum = static_cast<int> (last->first);
-    const int size = static_cast<int> (spec2index.size());
-    if(size == (1 + endSpectrum - startSpectrum))
+
+    IntervalList spectraIntervalList;
+    for( auto pair = spec2index.begin(); pair != spec2index.end(); ++pair )
+    {
+      spectraIntervalList.addInterval(static_cast<int>(pair->first));
+    }
+
+    if( firstWs )
     {
-      // Here we make the assumption (?) that the spectra IDs are sorted, and so
-      // are a list of ints from startSpectrum to endSpectrum without any gaps.
-      Interval interval(startSpectrum, endSpectrum);
-      if(it == m_wsNames.constBegin())
-        m_spectraIdIntervals.addInterval(interval);
-      else
-        m_spectraIdIntervals.setIntervalList(IntervalList::intersect(m_spectraIdIntervals,interval));
+      m_spectraIdIntervals = spectraIntervalList;
+      firstWs = false;
     }
     else
     {
-      // The spectra IDs do not appear to be an uninterrupted list of numbers,
-      // and so we must go through each one and construct the intervals that way.
-      // TODO - is this at all feasible for large workspaces, and/or many workspaces?
-      ++last;
-      for ( ; first != last; ++first) 
-      {
-        const int spectraId = static_cast<int> (first->first);
-
-        Interval interval(spectraId);
-        if(it == m_wsNames.constBegin())
-          m_spectraIdIntervals.addInterval(interval);
-        else
-          m_spectraIdIntervals.setIntervalList(IntervalList::intersect(m_spectraIdIntervals,interval));
-      }
+      m_spectraIdIntervals.setIntervalList(IntervalList::intersect(m_spectraIdIntervals, spectraIntervalList));
     }
   }
 }
 
+bool MantidWSIndexDialog::usingSpectraIDs() const
+{
+  return m_spectra && m_spectraIdIntervals.getList().size() > 0;
+}
+
 //----------------------------------
 // Interval public methods
 //----------------------------------
@@ -732,122 +716,14 @@ IntervalList IntervalList::intersect(const IntervalList& a, const IntervalList&
 {
   IntervalList output;
 
-  const QList<Interval> aList = a.getList();
-  const QList<Interval> bList = b.getList();
-
-  QList<Interval>::const_iterator aIt = aList.constBegin();
-  QList<Interval>::const_iterator bIt = bList.constBegin();
+  const std::set<int> aInts = a.getIntSet();
+  const std::set<int> bInts = b.getIntSet();
 
-  QList<Interval>::const_iterator aItEnd = aList.constEnd();
-  QList<Interval>::const_iterator bItEnd = bList.constEnd();
-
-  bool a_open = false;
-  bool b_open = false;
-
-  int start = 0;
-  int end = 0;
-
-  // This looks atrocious, but is probably one of the quickest available methods
-  // to find the intersections of two lists of Intervals.  An easier (but much 
-  // less effecient way) would be to define Interval::intersect(Interval a, Interval b)
-  // for the Interval class, and then use that method to intersect two lists.  
-
-  // Plus, this is probably overkill anyway: who is going to spend time typing 
-  // in an interval list big enough to slow a computer down?
-  while(aIt != aItEnd && bIt != bItEnd)
+  for( auto aInt = aInts.begin(); aInt != aInts.end(); ++aInt )
   {
-    if(!a_open && !b_open)
-    {
-      if((*aIt).start() < (*bIt).start())
-      {
-        a_open = true;
-      } 
-      else if((*aIt).start() > (*bIt).start())
-      {
-        b_open = true;
-      }
-      else
-      {
-        a_open = true;
-        b_open = true;
-        start = (*aIt).start();
-      }
-    }
-    else if(a_open && !b_open)
-    {
-      if((*aIt).end() < (*bIt).start())
-      {
-        a_open = false;
-        ++aIt;
-      } 
-      else if((*aIt).end() > (*bIt).start())
-      {
-        b_open = true;
-        start = (*bIt).start();
-      }
-      else
-      {
-        a_open = false;
-        b_open = true;
-        start = (*aIt).end();
-        end = (*bIt).start();
-        Interval interval(start, end);
-        output.addInterval(interval);
-        ++aIt;
-      }
-    }
-    else if(!a_open && b_open)
-    {
-      if((*bIt).end() < (*aIt).start())
-      {
-        b_open = false;
-        ++bIt;
-      } 
-      else if((*aIt).end() > (*bIt).start())
-      {
-        a_open = true;
-        start = (*aIt).start();
-      }
-      else
-      {
-        b_open = false;
-        a_open = true;
-        start = (*bIt).end();
-        end = (*aIt).start();
-        Interval interval(start, end);
-        output.addInterval(interval);
-        ++bIt;
-      }
-    }
-    else
-    {
-      if((*aIt).end() < (*bIt).end())
-      {
-        a_open = false;
-        end = (*aIt).end();
-        Interval interval(start, end);
-        output.addInterval(interval);
-        ++aIt;
-      } 
-      else if((*aIt).end() > (*bIt).end())
-      {
-        b_open = false;
-        end = (*bIt).end();
-        Interval interval(start, end);
-        output.addInterval(interval);
-        ++bIt;
-      }
-      else
-      {
-        a_open = false;
-        b_open = false;
-        end = (*aIt).end();
-        Interval interval(start, end);
-        output.addInterval(interval);
-        ++aIt;
-        ++bIt;
-      }
-    }
+    const bool inIntervalListB = bInts.find(*aInt) != bInts.end();
+    if( inIntervalListB )
+      output.addInterval(*aInt);
   }
 
   return output;

Code/Mantid/MantidPlot/src/Mantid/MantidWSIndexDialog.h

@@ -244,6 +244,9 @@ private slots:
   /// Generates an IntervalList which defines which spectra IDs the user can ask to plot.
   void generateSpectraIdIntervals();
 
+  /// Whether or not there are any common spectra IDs between workspaces.
+  bool usingSpectraIDs() const;
+
   /// A pointer to the parent MantidUI object
   MantidUI* m_mantidUI;