Correct Suite Filter when replacing Suite ECFLOW-1994

Viewer/ecflowUI/src/SuiteFilter.cpp

@@ -194,8 +194,9 @@ bool SuiteFilter::loadedSameAs(const std::vector<std::string>& loaded) const {
 
 bool SuiteFilter::setLoaded(const std::vector<std::string>& loaded, bool checkDiff) {
     bool same = false;
-    if (checkDiff)
+    if (checkDiff) {
         same = loadedSameAs(loaded);
+    }
 
     if (!checkDiff || !same) {
         return adjustLoaded(loaded);

libs/node/src/ecflow/node/ClientSuiteMgr.cpp

@@ -233,6 +233,14 @@ void ClientSuiteMgr::suite_added_in_defs(suite_ptr suite) {
     }
 }
 
+void ClientSuiteMgr::suite_replaced_in_defs(suite_ptr suite) {
+    size_t client_suites_size = clientSuites_.size();
+    for (size_t i = 0; i < client_suites_size; i++) {
+        clientSuites_[i].suite_replaced_in_defs(suite);
+        clientSuites_[i].update_suite_order();
+    }
+}
+
 void ClientSuiteMgr::suite_deleted_in_defs(suite_ptr suite) {
     size_t client_suites_size = clientSuites_.size();
     for (size_t i = 0; i < client_suites_size; i++) {

libs/node/src/ecflow/node/ClientSuiteMgr.hpp

@@ -89,12 +89,15 @@ class ClientSuiteMgr {
     /// returns the list of suites associated with a handle, Used by ecFlowview
     void suites(unsigned int client_handle, std::vector<std::string>& names) const;
 
-    /// A suite is being added in the definition.
+    /// A suite has been added to the definition.
     /// If the suite was previously registered *UPDATE* its suite_ptr
     /// Otherwise if any ClientSuites registered for automatic inclusion of new suite, add them in
     void suite_added_in_defs(suite_ptr);
 
-    /// The suite is being deleted in the definition, reset the suite_ptr
+    /// A suite has been updated in the definition.
+    void suite_replaced_in_defs(suite_ptr);
+
+    /// The suite has been deleted from the definition, reset the suite_ptr
     /// Deleted suites STAY registered, until explicitly dropped.
     void suite_deleted_in_defs(suite_ptr);
 

libs/node/src/ecflow/node/ClientSuites.cpp

@@ -105,6 +105,15 @@ void ClientSuites::suite_added_in_defs(suite_ptr suite) {
     }
 }
 
+void ClientSuites::suite_replaced_in_defs(suite_ptr suite) {
+    // *IF* and *ONLY IF* the suite was previously registered added, *UPDATE* its suite_ptr
+    auto i = find_suite(suite->name());
+    if (i != suites_.end()) {
+        // previously registered suite, update
+        add_suite(suite);
+    }
+}
+
 void ClientSuites::suite_deleted_in_defs(suite_ptr suite) {
     // Deleted suites are *NOT* automatically removed
     // They have to be moved explicitly by the user. Reset to weak ptr
@@ -136,9 +145,9 @@ defs_ptr ClientSuites::create_defs(defs_ptr server_defs) const {
     if (suites_.size() == server_defs->suiteVec().size()) {
         size_t real_suite_count = 0;
         for (auto i = suites_.begin(); i != suites_end; ++i) {
-            suite_ptr suite = (*i).weak_suite_ptr_.lock();
-            if (suite.get())
+            if (suite_ptr suite = (*i).weak_suite_ptr_.lock(); suite.get()) {
                 real_suite_count++;
+            }
         }
         if (real_suite_count == server_defs->suiteVec().size()) {
 

libs/node/src/ecflow/node/ClientSuites.hpp

@@ -83,12 +83,15 @@ class ClientSuites {
     void remove_suite(const std::string&);
     bool remove_suite(suite_ptr);
 
-    /// A new suite is being added in the definition.
+    /// A new suite has been added to the definition.
     /// If it was already registered update the suite ptr
     /// If auto add new suite enabled,register it
     void suite_added_in_defs(suite_ptr);
 
-    /// The suite is being deleted, update modify_change_no. So we do a full sync
+    /// A suite has been updated in the definition.
+    void suite_replaced_in_defs(suite_ptr);
+
+    /// The suite has been deleted, update modify_change_no. So we do a full sync
     /// RESETs suite ptr. Deleted suites are *NOT* automatically removed
     void suite_deleted_in_defs(suite_ptr);
 

libs/node/src/ecflow/node/Defs.cpp

@@ -409,9 +409,20 @@ suite_ptr Defs::add_suite(const std::string& name) {
         ss << "Add Suite failed: A Suite of name '" << name << "' already exists";
         throw std::runtime_error(ss.str());
     }
-    suite_ptr the_suite = Suite::create(name);
-    add_suite_only(the_suite, std::numeric_limits<std::size_t>::max());
-    return the_suite;
+    suite_ptr s = Suite::create(name);
+
+    if (s->defs()) {
+        std::stringstream ss;
+        ss << "Place Suite failed: The suite of name '" << s->name() << "' already owned by another Defs ";
+        throw std::runtime_error(ss.str());
+    }
+
+    insert_suite(s, std::numeric_limits<std::size_t>::max());
+
+    Ecf::incr_modify_change_no();
+    client_suite_mgr_.suite_added_in_defs(s);
+
+    return s;
 }
 
 void Defs::addSuite(const suite_ptr& s, size_t position) {
@@ -420,25 +431,48 @@ void Defs::addSuite(const suite_ptr& s, size_t position) {
         ss << "Add Suite failed: A Suite of name '" << s->name() << "' already exists";
         throw std::runtime_error(ss.str());
     }
-    add_suite_only(s, position);
+
+    if (s->defs()) {
+        std::stringstream ss;
+        ss << "Place Suite failed: The suite of name '" << s->name() << "' already owned by another Defs ";
+        throw std::runtime_error(ss.str());
+    }
+
+    insert_suite(s, position);
+
+    Ecf::incr_modify_change_no();
+    client_suite_mgr_.suite_added_in_defs(s);
 }
 
-void Defs::add_suite_only(const suite_ptr& s, size_t position) {
+void Defs::placeSuite(const suite_ptr& s, size_t position) {
+    if (findSuite(s->name()).get()) {
+        std::stringstream ss;
+        ss << "Place Suite failed: A Suite of name '" << s->name() << "' already exists";
+        throw std::runtime_error(ss.str());
+    }
+
     if (s->defs()) {
         std::stringstream ss;
-        ss << "Add Suite failed: The suite of name '" << s->name() << "' already owned by another Defs ";
+        ss << "Place Suite failed: The suite of name '" << s->name() << "' already owned by another Defs ";
         throw std::runtime_error(ss.str());
     }
 
+    insert_suite(s, position);
+
+    Ecf::incr_modify_change_no();
+    client_suite_mgr_.suite_replaced_in_defs(s);
+}
+
+void Defs::insert_suite(const suite_ptr& s, size_t position) {
+    assert(!s->defs()); // the suite to be inserted should still not have an associated defs
+
     s->set_defs(this);
     if (position >= suiteVec_.size()) {
         suiteVec_.push_back(s);
     }
     else {
         suiteVec_.insert(suiteVec_.begin() + position, s);
     }
-    Ecf::incr_modify_change_no();
-    client_suite_mgr_.suite_added_in_defs(s);
 }
 
 suite_ptr Defs::removeSuite(suite_ptr s) {
@@ -505,6 +539,17 @@ bool Defs::addChild(const node_ptr& child, size_t position) {
     return true;
 }
 
+bool Defs::placeChild(const node_ptr& child, size_t position) {
+    LOG_ASSERT(child.get(), "");
+    LOG_ASSERT(child->isSuite(), "");
+
+    // *** CANT construct shared_ptr from a raw pointer, must use dynamic_pointer_cast,
+    // *** otherwise the reference counts will get messed up.
+    // If the suite of the same exists, it is deleted first
+    placeSuite(std::dynamic_pointer_cast<Suite>(child), position);
+    return true;
+}
+
 void Defs::add_extern(const std::string& ex) {
     if (ex.empty()) {
         throw std::runtime_error("Defs::add_extern: Cannot add empty extern");
@@ -1321,16 +1366,18 @@ node_ptr Defs::replaceChild(const std::string& path,
         // transfer ownership to the server
         bool addOk                  = false;
         node_ptr client_node_to_add = clientNode->remove();
-        if (parentNodeOnServer)
+        if (parentNodeOnServer) {
             addOk = parentNodeOnServer->addChild(client_node_to_add, child_pos);
-        else
-            addOk = addChild(client_node_to_add, child_pos);
+        }
+        else {
+            addOk = placeChild(client_node_to_add, child_pos); // this is the second part of replacing a suite!
+        }
         LOG_ASSERT(addOk, "");
 
-        // preserve begun status. Note: we should't call begin() on the client side. As the suites will not have been
-        // begun. This can cause assert, because begin time attributes assumes that calendar has been initialised. This
-        // is not the case for client ASSERT failure: !c.suiteTime().is_special() at ../core/src/TimeSeries.cpp:526
-        // init has not been called on calendar. TimeSeries::duration ECFLOW-1612
+        // preserve begun status. Note: we should't call begin() on the client side. As the suites will not have
+        // been begun. This can cause assert, because begin time attributes assumes that calendar has been
+        // initialised. This is not the case for client ASSERT failure: !c.suiteTime().is_special() at
+        // ../core/src/TimeSeries.cpp:526 init has not been called on calendar. TimeSeries::duration ECFLOW-1612
         if (begin_node)
             client_node_to_add->begin();
 
@@ -1403,10 +1450,10 @@ node_ptr Defs::replaceChild(const std::string& path,
     bool addOk                  = server_parent->addChild(client_node_to_add, client_child_pos);
     LOG_ASSERT(addOk, "");
 
-    // preserve begun status. Note: we should't call begin() on the client side. As the suites will not have been begun.
-    // This can cause assert, because begin time attributes assumes that calendar has been initialised. This is not the
-    // case for client ASSERT failure: !c.suiteTime().is_special() at ../core/src/TimeSeries.cpp:526 init has not been
-    // called on calendar. TimeSeries::duration ECFLOW-1612
+    // preserve begun status. Note: we should't call begin() on the client side. As the suites will not have been
+    // begun. This can cause assert, because begin time attributes assumes that calendar has been initialised. This
+    // is not the case for client ASSERT failure: !c.suiteTime().is_special() at ../core/src/TimeSeries.cpp:526 init
+    // has not been called on calendar. TimeSeries::duration ECFLOW-1612
     if (begin_node)
         client_node_to_add->begin();
 

libs/node/src/ecflow/node/Defs.hpp

@@ -144,6 +144,7 @@ class Defs {
     /// Add a suite to the definition, will throw std::runtime_error if duplicate
     suite_ptr add_suite(const std::string& name);
     void addSuite(const suite_ptr&, size_t position = std::numeric_limits<std::size_t>::max());
+    void placeSuite(const suite_ptr&, size_t position = std::numeric_limits<std::size_t>::max());
     size_t child_position(const Node*) const;
 
     /// Externs refer to Nodes or, variable, events, meter, repeat, or generated variable
@@ -349,12 +350,45 @@ class Defs {
     void write_state(std::string&) const;
     void collate_defs_changes_only(DefsDelta&) const;
     void setupDefaultEnv();
-    void add_suite_only(const suite_ptr&, size_t position);
+
+    /**
+     * @brief Insert a suite at the specified position in the suite vector of the Defs
+     *
+     * This function inserts the given suite at the requested position. If position is greater than the
+     * current size of the suites vector the given suite is added to the end of the vector, otherwise
+     * at the requested position and all all suites after the position will be shifted.
+     *
+     * Note: This function does not check if the suite already exists in the defs,
+     *       it is the responsibility of the caller to ensure this.
+     *
+     * @param position the position to insert the suite at
+     */
+    void insert_suite(const suite_ptr&, size_t position);
 
     /// Removes the suite, from defs returned as suite_ptr, asserts if suite does not exist
     suite_ptr removeSuite(suite_ptr);
     node_ptr removeChild(Node*);
+
+    /**
+     * @brief Add a new suite to the defs at the specified position in the suite vector of the Defs
+     *
+     * This function inserts the given suite at the requested position, and notifies ClientSuiteMgr of a new suite.
+     *
+     * @param position the position to insert the suite at
+     * @return true if the suite was successfully added, false otherwise
+     */
     bool addChild(const node_ptr&, size_t position = std::numeric_limits<std::size_t>::max());
+
+    /**
+     * @brief Add a suite to the defs at the specified position in the suite vector of the Defs
+     *
+     * This function acts the 2nd part of the "remove/add" approach to implement ::replaceChild.
+     * It inserts the given suite at the requested position, but does *not* notify ClientSuiteMgr!
+     *
+     * @param position the position to insert the suite at
+     * @return true if the suite was successfully placed, false otherwise
+     */
+    bool placeChild(const node_ptr&, size_t position = std::numeric_limits<std::size_t>::max());
     friend class Node;
 
     /// For use by python interface,