WorkspaceGroup removes by name via the ADS. Re #7253

Code/Mantid/Framework/API/inc/MantidAPI/AnalysisDataService.h

@@ -132,6 +132,7 @@ class DLLExport AnalysisDataServiceImpl : public Kernel::DataService<API::Worksp
    //@{
 
    void deepRemoveGroup(const std::string& name);
+   void removeFromGroup(const std::string& groupName, const std::string& wsName);
 
    //@}
 

Code/Mantid/Framework/API/inc/MantidAPI/WorkspaceGroup.h

@@ -66,11 +66,7 @@ class MANTID_API_DLL WorkspaceGroup : public Workspace
   /// Turn ADS observations on/off
   void observeADSNotifications(const bool observeADS);
   /// Adds a workspace to the group.
-  void add(const std::string& wsName);
-  /// Adds a workspace to the group.
   void addWorkspace(Workspace_sptr workspace);
-  /// Does a workspace exist within the group
-  bool contains(const std::string & wsName) const;
   /// Returns the names of workspaces that make up this group. Note that this returns a copy as the internal vector can mutate while the vector is being iterated over.
   std::vector<std::string> getNames() const;
   /// Return the number of entries within the group
@@ -83,8 +79,8 @@ class MANTID_API_DLL WorkspaceGroup : public Workspace
   Workspace_sptr getItem(const std::string wsName) const;
   /// Prints the group to the screen using the logger at debug
   void print() const;
-  /// Remove a name from the group
-  void remove(const std::string& name);
+  /// Remove a workspace from the group
+  void removeItem(const size_t index);
   /// Remove all names from the group but do not touch the ADS
   void removeAll();
   /// This method returns true if the group is empty (no member workspace)
@@ -94,12 +90,26 @@ class MANTID_API_DLL WorkspaceGroup : public Workspace
   void updated() const;
   /// Inidicates that the workspace group can be treated as multiperiod.
   bool isMultiperiod() const;
+
+  /// @name Wrapped ADS calls
+  //@{
+
+  /// Adds a workspace to the group.
+  void add(const std::string& wsName);
+  /// Remove a name from the group
+  void remove(const std::string& name) { AnalysisDataService::Instance().removeFromGroup( this->name(), name); }
+  /// Does a workspace exist within the group
+  bool contains(const std::string & wsName) const;
+
+  //@}
 
 private:
   /// Private, unimplemented copy constructor
   WorkspaceGroup(const WorkspaceGroup& ref);
   /// Private, unimplemented copy assignment operator
   const WorkspaceGroup& operator=(const WorkspaceGroup&);
+  /// Remove a name from the group
+  void removeByADS(const std::string& name);
   /// Callback when a delete notification is received
   void workspaceDeleteHandle(Mantid::API::WorkspacePostDeleteNotification_ptr notice);
   /// Observer for workspace delete notfications
@@ -116,6 +126,8 @@ class MANTID_API_DLL WorkspaceGroup : public Workspace
   mutable Poco::Mutex m_mutex;
   /// Static reference to the logger
   static Kernel::Logger& g_log;
+
+  friend class AnalysisDataServiceImpl;
 };
 
 /// Shared pointer to a workspace group class

Code/Mantid/Framework/API/src/AnalysisDataService.cpp

@@ -172,6 +172,26 @@ namespace Mantid
         remove( name );
     }
 
+    /**
+     * Remove a workspace from a group but not from the ADS.
+     *
+     * @param groupName :: Name of a workspace group.
+     * @param wsName :: Name of a workspace to remove.
+     */
+    void AnalysisDataServiceImpl::removeFromGroup(const std::string &groupName, const std::string &wsName)
+    {
+        WorkspaceGroup_sptr group = retrieveWS<WorkspaceGroup>( groupName );
+        if ( !group )
+        {
+            throw std::runtime_error("Workspace " + groupName + " is not a workspace group.");
+        }
+        if ( !group->contains(wsName) )
+        {
+            throw std::runtime_error("WorkspaceGroup " + groupName + " does not containt workspace " + wsName);
+        }
+        group->removeByADS( wsName );
+    }
+
     //-------------------------------------------------------------------------
     // Private methods
     //-------------------------------------------------------------------------

Code/Mantid/Framework/API/src/WorkspaceGroup.cpp

@@ -158,7 +158,7 @@ void WorkspaceGroup::removeAll()
 /** Remove the named workspace from the group. Does not delete the workspace from the AnalysisDataService.
  *  @param wsName :: The name of the workspace to be removed from the group.
  */
-void WorkspaceGroup::remove(const std::string& wsName)
+void WorkspaceGroup::removeByADS(const std::string& wsName)
 {
   Poco::Mutex::ScopedLock _lock(m_mutex);
   auto it = m_workspaces.begin();
@@ -170,7 +170,6 @@ void WorkspaceGroup::remove(const std::string& wsName)
       break;
     }
   }
-  updated();
 }
 
 /// Print the names of all the workspaces in this group to the logger (at debug level)
@@ -183,6 +182,29 @@ void WorkspaceGroup::print() const
   }
 }
 
+/**
+ * Remove a workspace pointed to by an index. The workspace remains in the ADS if it was there
+ *
+ * @param index :: Index of a workspace to delete.
+ */
+void WorkspaceGroup::removeItem(const size_t index)
+{
+    Poco::Mutex::ScopedLock _lock(m_mutex);
+    // do not allow this way of removing for groups in the ADS
+    if ( ! name().empty() )
+    {
+        throw std::runtime_error("AnalysisDataService must be used to remove a workspace from group.");
+    }
+    if( index >= this->size() )
+    {
+      std::ostringstream os;
+      os << "WorkspaceGroup - index out of range. Requested=" << index << ", current size=" << this->size();
+      throw std::out_of_range(os.str());
+    }
+    auto it = m_workspaces.begin() + index;
+    m_workspaces.erase( it );
+}
+
 /** Callback for a workspace delete notification
  *
  * Removes any deleted entries from the group.
@@ -198,7 +220,7 @@ void WorkspaceGroup::workspaceDeleteHandle(Mantid::API::WorkspacePostDeleteNotif
 
   if( deletedName != this->getName() )
   {
-    this->remove(deletedName);
+    this->removeByADS(deletedName);
     if(isEmpty())
     {
       //We are about to get deleted so we don't want to recieve any notifications

Code/Mantid/Framework/API/test/AnalysisDataServiceTest.h

@@ -352,6 +352,33 @@ class AnalysisDataServiceTest : public CxxTest::TestSuite
       ads.clear();
   }
 
+  void test_removeFromGroup()
+  {
+      auto group = addGroupToADS("group");
+      TS_ASSERT_EQUALS( ads.size(), 3);
+      TS_ASSERT_EQUALS( group->size(), 2);
+      ads.removeFromGroup("group","group_2");
+      TS_ASSERT_EQUALS( ads.size(), 3);
+      TS_ASSERT_EQUALS( group->size(), 1);
+
+      TS_ASSERT_THROWS( ads.removeFromGroup("group","noworkspace"), std::runtime_error );
+      TS_ASSERT_THROWS( ads.removeFromGroup("nogroup","noworkspace"), std::runtime_error );
+      TS_ASSERT_THROWS( ads.removeFromGroup("nogroup","group_1"), std::runtime_error );
+      ads.clear();
+  }
+
+  void test_removeFromGroup_group()
+  {
+      auto group = addGroupWithGroupToADS("group");
+      TS_ASSERT_EQUALS( ads.size(), 5);
+      TS_ASSERT_EQUALS( group->size(), 2);
+      // remove group from group
+      ads.removeFromGroup("group","group_2");
+      TS_ASSERT_EQUALS( ads.size(), 5);
+      TS_ASSERT_EQUALS( group->size(), 1);
+      ads.clear();
+  }
+
 private:
 
   /// If replace=true then usea addOrReplace
@@ -395,7 +422,7 @@ class AnalysisDataServiceTest : public CxxTest::TestSuite
   }
 
   /// Add a group with 2 simple workspaces to the ADS
-  Workspace_sptr addGroupToADS(const std::string & name)
+  WorkspaceGroup_sptr addGroupToADS(const std::string & name)
   {
       WorkspaceGroup_sptr group( new WorkspaceGroup );
       group->addWorkspace( MockWorkspace_sptr(new MockWorkspace) );
@@ -405,7 +432,7 @@ class AnalysisDataServiceTest : public CxxTest::TestSuite
   }
 
   /// Add a group with 1 simple workspace and 1 group with 2 simple ws to the ADS
-  Workspace_sptr addGroupWithGroupToADS(const std::string & name)
+  WorkspaceGroup_sptr addGroupWithGroupToADS(const std::string & name)
   {
       WorkspaceGroup_sptr group( new WorkspaceGroup );
       group->addWorkspace( MockWorkspace_sptr(new MockWorkspace) );

Code/Mantid/Framework/API/test/WorkspaceGroupTest.h

@@ -161,6 +161,25 @@ class WorkspaceGroupTest : public CxxTest::TestSuite
     AnalysisDataService::Instance().clear();
 }
 
+  void test_removeItem()
+  {
+    WorkspaceGroup_sptr group1 = makeGroup();
+    TS_ASSERT_THROWS( group1->removeItem(1), std::runtime_error );
+
+    WorkspaceGroup_sptr group(new WorkspaceGroup());
+    Workspace_sptr ws1(new WorkspaceTester());
+    group->addWorkspace( ws1 );
+    Workspace_sptr ws2(new WorkspaceTester());
+    group->addWorkspace( ws2 );
+
+    TS_ASSERT_EQUALS( group->size(), 2 );
+    TS_ASSERT_THROWS_NOTHING( group->removeItem(1) );
+    TS_ASSERT_EQUALS( group->size(), 1 );
+    TS_ASSERT_EQUALS( group->getItem(0), ws1 );
+
+    AnalysisDataService::Instance().clear();
+}
+
   void test_removeAll()
   {
     WorkspaceGroup_sptr group = makeGroup();
@@ -225,45 +244,45 @@ class WorkspaceGroupTest : public CxxTest::TestSuite
     AnalysisDataService::Instance().clear();
   }
 
-  void testUpdated()
-  {
-    WorkspaceGroupTest_WorkspaceGroupObserver observer;
-    WorkspaceGroup_sptr group(new WorkspaceGroup());
-    AnalysisDataService::Instance().add( "group", group );
-    TS_ASSERT( !observer.received );
+//  void testUpdated()
+//  {
+//    WorkspaceGroupTest_WorkspaceGroupObserver observer;
+//    WorkspaceGroup_sptr group(new WorkspaceGroup());
+//    AnalysisDataService::Instance().add( "group", group );
+//    TS_ASSERT( !observer.received );
 
-    boost::shared_ptr<WorkspaceTester> ws(new WorkspaceTester());
-    ws->initialize(2,3,4);
-    AnalysisDataService::Instance().addOrReplace("name_0", ws);
+//    boost::shared_ptr<WorkspaceTester> ws(new WorkspaceTester());
+//    ws->initialize(2,3,4);
+//    AnalysisDataService::Instance().addOrReplace("name_0", ws);
 
-    ws.reset(new WorkspaceTester());
-    ws->initialize(2,3,4);
-    AnalysisDataService::Instance().addOrReplace("name_12", ws);
+//    ws.reset(new WorkspaceTester());
+//    ws->initialize(2,3,4);
+//    AnalysisDataService::Instance().addOrReplace("name_12", ws);
 
-    group->add( "name_0" );
-    TS_ASSERT( observer.received );
-    observer.received = false;
+//    group->add( "name_0" );
+//    TS_ASSERT( observer.received );
+//    observer.received = false;
 
-    group->observeADSNotifications( false );
+//    group->observeADSNotifications( false );
 
-    group->add( "name_12" );
-    TS_ASSERT( !observer.received );
-    observer.received = false;
+//    group->add( "name_12" );
+//    TS_ASSERT( !observer.received );
+//    observer.received = false;
 
-    group->observeADSNotifications( true );
+//    group->observeADSNotifications( true );
 
-    group->remove( "name_12" );
-    TS_ASSERT( observer.received );
-    observer.received = false;
+//    group->remove( "name_12" );
+//    TS_ASSERT( observer.received );
+//    observer.received = false;
 
-    group->observeADSNotifications( false );
+//    group->observeADSNotifications( false );
 
-    group->remove( "name_0" );
-    TS_ASSERT( !observer.received );
-    observer.received = false;
+//    group->remove( "name_0" );
+//    TS_ASSERT( !observer.received );
+//    observer.received = false;
 
-    AnalysisDataService::Instance().clear();
-  }
+//    AnalysisDataService::Instance().clear();
+//  }
 
   void test_not_multiperiod_with_less_than_one_element()
   {