Fix subsystem-remove bug

simgear/structure/subsystem_mgr.cxx

@@ -455,7 +455,7 @@ SGSubsystemGroup::set_subsystem (const string &name, SGSubsystem * subsystem,
     subsystem->set_group(this);
     notifyDidChange(subsystem, State::ADD);
     
-    if (_state != State::INVALID) {
+    if (_state != State::INVALID && (_state <= State::POSTINIT)) {
         // transition to the correct state
         if (_state >= State::BIND) {
             notifyWillChange(subsystem, State::BIND);
@@ -469,7 +469,7 @@ SGSubsystemGroup::set_subsystem (const string &name, SGSubsystem * subsystem,
             notifyDidChange(subsystem, State::INIT);
         }
         
-        if (_state >= State::POSTINIT) {
+        if (_state == State::POSTINIT) {
             notifyWillChange(subsystem, State::POSTINIT);
             subsystem->postinit();
             notifyDidChange(subsystem, State::POSTINIT);
@@ -515,13 +515,13 @@ SGSubsystemGroup::remove_subsystem(const string &name)
         
         if (_state != State::INVALID) {
             // transition out correctly
-            if (_state >= State::INIT) {
+            if ((_state >= State::INIT) && (_state < State::SHUTDOWN)) {
                 notifyWillChange(sub, State::SHUTDOWN);
                 sub->shutdown();
                 notifyDidChange(sub, State::SHUTDOWN);
             }
             
-            if (_state >= State::BIND) {
+            if ((_state >= State::BIND) && (_state < State::UNBIND)) {
                 notifyWillChange(sub, State::UNBIND);
                 sub->unbind();
                 notifyDidChange(sub, State::UNBIND);

simgear/structure/subsystem_mgr.hxx

@@ -301,6 +301,10 @@ public:
     /// get the parent group of this subsystem
     SGSubsystemGroup* get_group() const;
     
+    // ordering here is exceptionally important, due to
+    // liveness of ranges. If you're extending this consider
+    // carefully where the new state lies and position it correctly.
+    // Also extend the tests to ensure you handle all cases.
     enum class State {
         INVALID = -1,
         ADD = 0,
@@ -310,8 +314,8 @@ public:
         REINIT,
         SUSPEND,
         RESUME,
-        UNBIND,
         SHUTDOWN,
+        UNBIND,
         REMOVE
     };
     

simgear/structure/subsystem_test.cxx

@@ -451,6 +451,11 @@ void testAddRemoveAfterInit()
     auto radio1 = manager->createInstance<FakeRadioSub>("nav1");
     group->set_subsystem(radio1);
 
+    auto com1 = manager->createInstance<FakeRadioSub>("com1");
+    group->set_subsystem(com1);
+    auto com2 = manager->createInstance<FakeRadioSub>("com2");
+    group->set_subsystem(com2);
+    
     manager->bind();
     manager->init();
     
@@ -473,8 +478,34 @@ void testAddRemoveAfterInit()
     SG_VERIFY(d->hasEvent("fake-radio.nav1-will-remove"));
     SG_VERIFY(d->hasEvent("fake-radio.nav1-did-remove"));
     
-   
+    manager->shutdown();
     
+    SG_VERIFY(d->hasEvent("fake-radio.nav2-will-shutdown"));
+    SG_VERIFY(d->hasEvent("fake-radio.nav2-did-shutdown"));
+    SG_VERIFY(d->hasEvent("fake-radio.com1-will-shutdown"));
+    SG_VERIFY(d->hasEvent("fake-radio.com1-did-shutdown"));
+    
+    d->events.clear();
+    
+    ok = manager->remove("fake-radio.com1");
+    SG_VERIFY(d->hasEvent("fake-radio.com1-will-remove"));
+    SG_VERIFY(d->hasEvent("fake-radio.com1-did-remove"));
+    SG_VERIFY(d->hasEvent("fake-radio.com1-will-unbind"));
+    SG_VERIFY(d->hasEvent("fake-radio.com1-did-unbind"));
+    SG_VERIFY(!d->hasEvent("fake-radio.com1-will-shutdown"));
+    SG_VERIFY(!d->hasEvent("fake-radio.com1-did-shutdown"));
+    
+    manager->unbind();
+    
+    SG_VERIFY(d->hasEvent("fake-radio.com2-will-unbind"));
+    SG_VERIFY(d->hasEvent("fake-radio.com2-did-unbind"));
+    
+    d->events.clear();
+    manager->remove("fake-radio.com2");
+    SG_VERIFY(!d->hasEvent("fake-radio.com2-will-unbind"));
+    SG_VERIFY(!d->hasEvent("fake-radio.com2-did-unbind"));
+    SG_VERIFY(d->hasEvent("fake-radio.com2-will-remove"));
+    SG_VERIFY(d->hasEvent("fake-radio.com2-did-remove"));
 }
 
 ///////////////////////////////////////////////////////////////////////////////