Changes to removing methods of WorkspaceGroup. Re #6199

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

@@ -116,6 +116,8 @@ class DLLExport AnalysisDataServiceImpl : public Kernel::DataService<API::Worksp
    virtual boost::shared_ptr<API::Workspace> retrieve( const std::string& name) const;
    /// Check to see if a data object exists in the store
    virtual bool doesExist(const std::string& name) const;
+   /// Overridden to take workspace groups into account
+   virtual void remove( const std::string& name);
 
    /** Retrieve a workspace and cast it to the given WSTYPE
     *
@@ -133,7 +135,7 @@ class DLLExport AnalysisDataServiceImpl : public Kernel::DataService<API::Worksp
    }
 
    /// Remove a workspace if it is on the top level of the ADS, ie not a part of a workspace group.
-   void removeFromTopLevel(const std::string &name);
+   bool removeFromTopLevel(const std::string &name);
    /// Count instances of a workspace in the ADS
    size_t count(Workspace_const_sptr workspace) const;
    /// Find a stored workspace

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

@@ -92,6 +92,8 @@ class MANTID_API_DLL WorkspaceGroup : public Workspace
   void print() const;
   /// Remove a name from the group
   void remove(const std::string& name);
+  /// Remove a workspace by name.
+  bool deepRemove(const std::string& name, bool convertToUpperCase = true, size_t nesting = 0);
   /// Remove all names from the group but do not touch the ADS
   void removeAll();
   /// Remove all names from the group and also from the ADS
@@ -111,10 +113,10 @@ class MANTID_API_DLL WorkspaceGroup : public Workspace
   WorkspaceGroup(const WorkspaceGroup& ref);
   /// Private, unimplemented copy assignment operator
   const WorkspaceGroup& operator=(const WorkspaceGroup&);
-  /// Callback when a delete notification is received
-  void workspaceDeleteHandle(Mantid::API::WorkspacePostDeleteNotification_ptr notice);
-  /// Observer for workspace delete notfications
-  Poco::NObserver<WorkspaceGroup, Mantid::API::WorkspacePostDeleteNotification> m_deleteObserver;
+//  /// Callback when a delete notification is received
+//  void workspaceDeleteHandle(Mantid::API::WorkspacePostDeleteNotification_ptr notice);
+//  /// Observer for workspace delete notfications
+//  Poco::NObserver<WorkspaceGroup, Mantid::API::WorkspacePostDeleteNotification> m_deleteObserver;
   /// Callback when a before-replace notification is received
   void workspaceReplaceHandle(Mantid::API::WorkspaceBeforeReplaceNotification_ptr notice);
   /// Observer for workspace before-replace notfications

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

@@ -121,11 +121,30 @@ namespace Mantid
         return false;
     }
 
+    /**
+     * Remove a workspace
+     * @param name :: Name of a workspace to remove.
+     */
+    void AnalysisDataServiceImpl::remove(const std::string &name)
+    {
+        if ( removeFromTopLevel(name) ) return;
+
+        std::vector<Workspace_sptr> workspaces = getObjects();
+        for(auto it = workspaces.begin(); it != workspaces.end(); ++it)
+        {
+            WorkspaceGroup* wsg = dynamic_cast<WorkspaceGroup*>( it->get() );
+            if ( wsg && wsg->deepRemove(name) )
+            {
+                return;
+            }
+        }
+    }
+
     /**
      * A method to help with workspace group management.
      * @param name :: Name of a workspace to emove.
      */
-    void AnalysisDataServiceImpl::removeFromTopLevel(const std::string &name)
+    bool AnalysisDataServiceImpl::removeFromTopLevel(const std::string &name)
     {
         std::string foundName = name;
         std::transform(foundName.begin(), foundName.end(), foundName.begin(),toupper);
@@ -135,9 +154,10 @@ namespace Mantid
             if ( (**it).getUpperCaseName() == foundName )
             {
                 Kernel::DataService<Workspace>::remove( name );
-                return;
+                return true;
             }
         }
+        return false;
     }
 
     /**

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

@@ -21,7 +21,6 @@ size_t WorkspaceGroup::g_maxNestingLevel = 100;
 
 WorkspaceGroup::WorkspaceGroup(const bool observeADS) :
   Workspace(), 
-  m_deleteObserver(*this, &WorkspaceGroup::workspaceDeleteHandle),
   m_replaceObserver(*this, &WorkspaceGroup::workspaceReplaceHandle),
   m_workspaces(), m_observingADS(false), m_nameCounter(0)
 {
@@ -76,7 +75,6 @@ void WorkspaceGroup::observeADSNotifications(const bool observeADS)
   {
     if(!m_observingADS)
     {
-      AnalysisDataService::Instance().notificationCenter.addObserver(m_deleteObserver);
       AnalysisDataService::Instance().notificationCenter.addObserver(m_replaceObserver);
       m_observingADS = true;
     }
@@ -85,7 +83,6 @@ void WorkspaceGroup::observeADSNotifications(const bool observeADS)
   {
     if(m_observingADS)
     {
-      AnalysisDataService::Instance().notificationCenter.removeObserver(m_deleteObserver);
       AnalysisDataService::Instance().notificationCenter.removeObserver(m_replaceObserver);
       m_observingADS = false;
     }
@@ -142,6 +139,9 @@ void WorkspaceGroup::addWorkspace(Workspace_sptr workspace)
 
 /**
  * Remove a workspace if it is in the group. Doesn't look into nested groups.
+ * Removes the workspace from ADS (if it was there). If ADS has multiple copies
+ * of the workspace only one of them is removed.
+ *
  * @param workspace :: A workspace to remove.
  */
 void WorkspaceGroup::removeWorkspace(Workspace_sptr workspace)
@@ -288,7 +288,16 @@ size_t WorkspaceGroup::count(Workspace_const_sptr workspace, size_t nesting) con
 /// Empty all the entries out of the workspace group. Does not remove the workspaces from the ADS.
 void WorkspaceGroup::removeAll()
 {
-  m_workspaces.clear();
+    for(auto it = m_workspaces.begin(); it != m_workspaces.end(); ++it)
+    {
+        auto ws = *it;
+        if ( ! ws->name().empty() )
+        {
+            // leave removed workspace in the ADS
+            AnalysisDataService::Instance().add( ws->name(), ws );
+        }
+    }
+    m_workspaces.clear();
 }
 
 /** Remove the named workspace from the group. Does not delete the workspace from the AnalysisDataService.
@@ -302,24 +311,81 @@ void WorkspaceGroup::remove(const std::string& wsName)
   {
     if ( (**it).name() == wsName )
     {
+      // leave removed workspace in the ADS
+      auto ws = *it;
+      AnalysisDataService::Instance().add( ws->name(), ws );
       m_workspaces.erase(it);
       break;
     }
   }
   updated();
 }
 
+/**
+ *  Search in nested groups for a name and remove the workspace if found.
+ * @param name :: Name of a workspace to remove.
+ * @param convertToUpperCase  :: Set true if wsName needs to be converted to upper case before search.
+ * @param nesting :: Current nesting level. To detect cycles.
+ * @return :: True in success.
+ */
+bool WorkspaceGroup::deepRemove(const std::string &name, bool convertToUpperCase, size_t nesting)
+{
+    if ( nesting >= g_maxNestingLevel )
+    {
+        // check for cycles
+        throw std::runtime_error("Workspace group nesting is too deep. Could be a cycle.");
+    }
+
+    std::string upperName = name;
+    if ( convertToUpperCase )
+    {
+        std::transform(upperName.begin(), upperName.end(), upperName.begin(),toupper);
+    }
+
+    Poco::Mutex::ScopedLock _lock(m_mutex);
+
+    // try direct members first
+    size_t oldSize = m_workspaces.size();
+    for(auto it = m_workspaces.begin(); it != m_workspaces.end(); ++it)
+    {
+      if ( (**it).getUpperCaseName() == upperName )
+      {
+        m_workspaces.erase(it);
+        break;
+      }
+    }
+
+    // empty top-level groups must be removed from the ADS
+    if ( nesting == 0 && m_workspaces.empty() && ! getName().empty() )
+    {
+        AnalysisDataService::Instance().remove( getName() );
+        return true;
+    }
+
+    if ( oldSize != m_workspaces.size() ) return true;
+
+    // if not found look recursively in child groups
+    for(auto it = m_workspaces.begin(); it != m_workspaces.end(); ++it)
+    {
+        WorkspaceGroup* wsg = dynamic_cast<WorkspaceGroup*>( it->get() );
+        if ( wsg && wsg->deepRemove( upperName, false, nesting + 1) )
+        {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 /// Removes all members of the group from the group AND from the AnalysisDataService
 void WorkspaceGroup::deepRemoveAll()
 {
   Poco::Mutex::ScopedLock _lock(m_mutex);
-  if( m_observingADS ) AnalysisDataService::Instance().notificationCenter.removeObserver(m_deleteObserver);
   while (!m_workspaces.empty())
   {
     AnalysisDataService::Instance().remove(m_workspaces.back()->name());
-	  m_workspaces.pop_back();
+    m_workspaces.pop_back();
   }
-  if( m_observingADS ) AnalysisDataService::Instance().notificationCenter.addObserver(m_deleteObserver);
 }
 
 /// Print the names of all the workspaces in this group to the logger (at debug level)
@@ -332,31 +398,6 @@ void WorkspaceGroup::print() const
   }
 }
 
-/** Callback for a workspace delete notification
- *
- * Removes any deleted entries from the group.
- * This also deletes the workspace group when the last member of it gets deteleted.
- *
- * @param notice :: A pointer to a workspace delete notificiation object
- */
-void WorkspaceGroup::workspaceDeleteHandle(Mantid::API::WorkspacePostDeleteNotification_ptr notice)
-{
-  Poco::Mutex::ScopedLock _lock(m_mutex);
-  const std::string deletedName = notice->object_name();
-  if( !this->contains(deletedName)) return;
-
-  if( deletedName != this->getName() )
-  {
-    this->remove(deletedName);
-    if(isEmpty())
-    {
-      //We are about to get deleted so we don't want to recieve any notifications
-      observeADSNotifications(false);
-      AnalysisDataService::Instance().remove(this->getName());
-    }
-  }
-}
-
 /**
  * Callback when a after-replace notification is received
  * Replaces a member if it was replaced in the ADS.

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

@@ -296,7 +296,7 @@ class WorkspaceGroupTest : public CxxTest::TestSuite
     TS_ASSERT_EQUALS( boost::dynamic_pointer_cast<Workspace>(group->getItem(0)), ws );
     TS_ASSERT_EQUALS( AnalysisDataService::Instance().size(), 1);
     // add same workspace again -
-    group->add("ws");
+    //group->add("ws");
     TS_ASSERT_EQUALS( group->size(), 1 );
     TS_ASSERT_EQUALS( boost::dynamic_pointer_cast<Workspace>(group->getItem(0)), ws );
     TS_ASSERT_EQUALS( AnalysisDataService::Instance().size(), 1);
@@ -373,45 +373,74 @@ class WorkspaceGroupTest : public CxxTest::TestSuite
     AnalysisDataService::Instance().clear();
   }
 
-  void xtest_remove()
+  void test_removeWorkspace()
+  {
+      WorkspaceGroup_sptr group(new WorkspaceGroup());
+      WorkspaceGroup_sptr child_group(new WorkspaceGroup());
+      Workspace_sptr ws(new WorkspaceTester());
+      child_group->addWorkspace( ws );
+
+      group->addWorkspace( child_group );
+      group->addWorkspace( ws );
+
+      // count decreases by 1
+      TS_ASSERT_EQUALS( group->count( ws ), 2 );
+      group->removeWorkspace( ws );
+      TS_ASSERT_EQUALS( group->count( ws ), 1 );
+
+      TS_ASSERT_THROWS_NOTHING( group->removeWorkspace( makeWorkspace() ) );
+
+      // removes workspace from ADS as well
+      AnalysisDataService::Instance().add( "group", group );
+      TS_ASSERT( AnalysisDataService::Instance().doesExist("group_1") );
+      group->removeWorkspace( group->getItem(0) );
+      TS_ASSERT( ! AnalysisDataService::Instance().doesExist("group_1") );
+      AnalysisDataService::Instance().clear();
+  }
+
+  void test_remove()
   {
     WorkspaceGroup_sptr group = makeGroup();
-    group->remove("ws0");
-    TSM_ASSERT( "remove() takes out from group", !group->contains("ws0") );
-    TSM_ASSERT( "remove() does not take out of ADS ", AnalysisDataService::Instance().doesExist("ws0") );
+    AnalysisDataService::Instance().add( "group", group );
+    TS_ASSERT( AnalysisDataService::Instance().doesExist("group_1") );
+    group->remove("group_1");
+    TSM_ASSERT( "remove() takes out from group", !group->contains("group_1") );
+    TSM_ASSERT( "remove() does not take out of ADS ", AnalysisDataService::Instance().doesExist("group_1") );
     AnalysisDataService::Instance().clear();
 }
 
-  void xtest_removeAll()
+  void test_removeAll()
   {
     WorkspaceGroup_sptr group = makeGroup();
+    AnalysisDataService::Instance().add( "group", group );
     group->removeAll();
     TS_ASSERT_EQUALS( group->size(), 0);
-    TSM_ASSERT( "removeAll() does not take out of ADS ", AnalysisDataService::Instance().doesExist("ws0") );
+    TSM_ASSERT( "removeAll() does not take out of ADS ", AnalysisDataService::Instance().doesExist("group_1") );
     AnalysisDataService::Instance().clear();
   }
 
-  /*
-  void xtest_deleting_workspaces()
+  void test_deleting_workspaces()
   {
     WorkspaceGroup_sptr group = makeGroup();
+    AnalysisDataService::Instance().add( "group", group );
     TS_ASSERT( AnalysisDataService::Instance().doesExist("group") );
 
     // When you delete a workspace it gets removed from the group
-    AnalysisDataService::Instance().remove("ws0");
+    AnalysisDataService::Instance().remove("group_1");
     TS_ASSERT( AnalysisDataService::Instance().doesExist("group") );
-    TS_ASSERT( !group->contains("ws0") );
+    TS_ASSERT( !group->contains("group_1") );
 
-    AnalysisDataService::Instance().remove("ws1");
+    AnalysisDataService::Instance().remove("group_2");
     TS_ASSERT( AnalysisDataService::Instance().doesExist("group") );
-    TS_ASSERT( !group->contains("ws1") );
+    TS_ASSERT( !group->contains("group_2") );
 
     // When you remove the last one, the group deletes itself
-    AnalysisDataService::Instance().remove("ws2");
+    AnalysisDataService::Instance().remove("group_3");
     TS_ASSERT( !AnalysisDataService::Instance().doesExist("group") );
     AnalysisDataService::Instance().clear();
   }
 
+  /*
   void xtest_areNamesSimilar()
   {
     WorkspaceGroup_sptr group(new WorkspaceGroup());

Code/Mantid/Framework/Kernel/inc/MantidKernel/DataService.h

@@ -265,7 +265,7 @@ class DLLExport DataService
   //--------------------------------------------------------------------------
   /** Remove an object from the service.
    * @param name :: name of the object */
-  void remove( const std::string& name)
+  virtual void remove( const std::string& name)
   {
     // Make DataService access thread-safe
     m_mutex.lock();