diff --git a/simgear/structure/subsystem_mgr.cxx b/simgear/structure/subsystem_mgr.cxx index 474ea3dd..537649e2 100644 --- a/simgear/structure/subsystem_mgr.cxx +++ b/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); diff --git a/simgear/structure/subsystem_mgr.hxx b/simgear/structure/subsystem_mgr.hxx index b3cd71f9..95cd5a46 100644 --- a/simgear/structure/subsystem_mgr.hxx +++ b/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 }; diff --git a/simgear/structure/subsystem_test.cxx b/simgear/structure/subsystem_test.cxx index ed399351..a1136a78 100644 --- a/simgear/structure/subsystem_test.cxx +++ b/simgear/structure/subsystem_test.cxx @@ -451,6 +451,11 @@ void testAddRemoveAfterInit() auto radio1 = manager->createInstance("nav1"); group->set_subsystem(radio1); + auto com1 = manager->createInstance("com1"); + group->set_subsystem(com1); + auto com2 = manager->createInstance("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")); } ///////////////////////////////////////////////////////////////////////////////