diff --git a/Code/Mantid/Framework/API/src/WorkspaceGroup.cpp b/Code/Mantid/Framework/API/src/WorkspaceGroup.cpp index 0fb13f30c155..2d8021b07c0f 100644 --- a/Code/Mantid/Framework/API/src/WorkspaceGroup.cpp +++ b/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::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 ); } /** diff --git a/Code/Mantid/Framework/API/test/AnalysisDataServiceTest.h b/Code/Mantid/Framework/API/test/AnalysisDataServiceTest.h index d6b125b9558d..4a26522d4673 100644 --- a/Code/Mantid/Framework/API/test/AnalysisDataServiceTest.h +++ b/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 diff --git a/Code/Mantid/Framework/Kernel/inc/MantidKernel/DataService.h b/Code/Mantid/Framework/Kernel/inc/MantidKernel/DataService.h index 5ea5c74d3f02..5823cd9da97c 100644 --- a/Code/Mantid/Framework/Kernel/inc/MantidKernel/DataService.h +++ b/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; diff --git a/Code/Mantid/Framework/Kernel/test/DataServiceTest.h b/Code/Mantid/Framework/Kernel/test/DataServiceTest.h index 1de126e0b526..d373c5ba0c52 100644 --- a/Code/Mantid/Framework/Kernel/test/DataServiceTest.h +++ b/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);