Fixed issue with not removing duplicate workspace outside group.

Also added rename notifications to data service.
Unit tests for workspace group also updated.

Refs #7807

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

@@ -280,42 +280,40 @@ void WorkspaceGroup::workspaceDeleteHandle(Mantid::API::WorkspacePostDeleteNotif
 }
 
 /**
- * Callback when a after-replace notification is received
+ * Callback when a before-replace notification is received
  * Replaces a member if it was replaced in the ADS.
- * @param notice :: A pointer to a workspace after-replace notificiation object
+ * @param notice :: A pointer to a workspace before-replace notificiation object
  */
 void WorkspaceGroup::workspaceReplaceHandle(Mantid::API::WorkspaceBeforeReplaceNotification_ptr notice)
 {
   Poco::Mutex::ScopedLock _lock(m_mutex);
   std::string newName = notice->object_name();
-  if(this->contains(newName))
-  {
-    bool isObserving = m_observingADS;
-    if ( isObserving )
-      observeADSNotifications( false );
 
-    //remove any workspace that already exists with the name
-    if(this->name() != newName)
-    {
+  bool isObserving = m_observingADS;
+  if ( isObserving )
+    observeADSNotifications( false );
 
-      //remove workspace from group
-      std::vector<Workspace_sptr>::iterator iter = this->m_workspaces.begin();
-      for(; iter != m_workspaces.end(); ++iter)
+  const std::string replacedName = notice->object_name();
+  bool found(false);
+  for(auto citr=m_workspaces.begin(); citr!=m_workspaces.end(); ++citr)
+  {
+    if ( (**citr).name() == replacedName )
+    {
+      if(!found)
       {
-        if((**iter).name() == newName)
-        {
-          m_workspaces.erase(iter);
-          break;
-        }
+        *citr = notice->new_object();
+        found = true;
+      }
+      else
+      {
+        m_workspaces.erase(citr);
+        break;
       }
-
-      //replace the object in the group
-      this->addWorkspace(notice->new_object());
     }
-
-    if ( isObserving )
-      observeADSNotifications( true );
   }
+
+  if ( isObserving )
+    observeADSNotifications( true );
 }
 
 /**

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

@@ -430,6 +430,32 @@ class AnalysisDataServiceTest : public CxxTest::TestSuite
 
   }
 
+  void test_Rename_Group_Child_To_Another_Child()
+  {
+    auto group = addGroupToADS("group");
+    TS_ASSERT_EQUALS( ads.size(), 3);
+
+    ads.rename("group_1", "group_2");
+
+    TS_ASSERT_EQUALS( ads.size(), 2);
+    TS_ASSERT( group->contains("group_2") );
+    TS_ASSERT( !group->contains("group_1") );
+  }
+
+  void test_Rename_Group_To_Another_Group()
+  {
+    auto group1 = addGroupToADS("group1");
+    auto group2 = addGroupToADS("group2");
+
+    ads.rename("group2", "group1");
+
+    TS_ASSERT_EQUALS( ads.size(), 5);
+    TS_ASSERT( group1->contains("group1_1") );
+    TS_ASSERT( !ads.doesExist("group2") );
+    TS_ASSERT( ads.doesExist("group2_1") );
+    TS_ASSERT( ads.doesExist("group2_2") );
+  }
+
 private:
 
   /// If replace=true then usea addOrReplace

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

@@ -278,7 +278,7 @@ class DLLExport DataService
   {
     checkForEmptyName(newName);
 
-    auto item = this->retrieve(oldName);
+    // Make DataService access thread-safe
     m_mutex.lock();
 
     std::string foundName;
@@ -290,14 +290,35 @@ class DLLExport DataService
       return;
     }
 
-    // delete the object with the old name silently
+    // delete the object with the old name
     auto object = it->second;
-    datamap.erase(it);
-
+    m_mutex.lock();
+    datamap.erase( it );
     m_mutex.unlock();
 
-    this->addOrReplace(newName,item);
+    // if there is another object which has newName delete it
+    // this is actually a replace operation, so we send out there notifications as well.
+    notificationCenter.postNotification(new BeforeReplaceNotification(newName, it->second, object));
+    it = datamap.find( newName );
+    if ( it != datamap.end() )
+    {
+      m_mutex.lock();
+      datamap.erase( it );
+      m_mutex.unlock();
+    }
+
+    // insert the old object with the new name
+    if ( ! datamap.insert(typename svcmap::value_type(newName, object)).second )
+    {
+      std::string error=" add : Unable to insert Data Object : '"+newName+"'";
+      g_log.error(error);
+      m_mutex.unlock();
+      throw std::runtime_error(error);
+    }
+    g_log.information("Data Object '"+ foundName +"' renamed to '" + newName + "'");
+    m_mutex.unlock();
 
+    notificationCenter.postNotification(new AfterReplaceNotification(newName,object));
     notificationCenter.postNotification(new RenameNotification(oldName, newName));
 
     return;

Code/Mantid/Framework/Kernel/test/DataServiceTest.h

@@ -161,13 +161,13 @@ class DataServiceTest : public CxxTest::TestSuite
     TSM_ASSERT_THROWS("One should have been renamed to anotherOne", svc.retrieve("one"), Exception::NotFoundError );
     TSM_ASSERT_EQUALS("One should have been renamed to anotherOne", svc.retrieve("anotherOne"), one );
 
-    TSM_ASSERT_EQUALS("The observers should have been called 2 times in total", notificationFlag, 2 );
+    TSM_ASSERT_EQUALS("The observers should have been called 4 times in total", notificationFlag, 4 );
 
     svc.rename("Two","anotherOne");
     TS_ASSERT_EQUALS( svc.size(), 1);
     TSM_ASSERT_THROWS("Two should have been renamed to anotherOne", svc.retrieve("two"), Exception::NotFoundError );
     TSM_ASSERT_EQUALS("Two should have been renamed to anotherOne", svc.retrieve("anotherOne"), two );
-    TSM_ASSERT_EQUALS("The observers should have been called 4 times in total", notificationFlag, 4 );
+    TSM_ASSERT_EQUALS("The observers should have been called 6 times in total", notificationFlag, 6 );
 
     svc.notificationCenter.removeObserver(observer);
     svc.notificationCenter.removeObserver(observer2);