Update signal to remove a connection when disconnected (#1462)

CHANGELOG.md

@@ -5,6 +5,7 @@
 * Common
 
   * Removed use of boost::filesystem in public APIs: [#1417](https://github.com/dartsim/dart/pull/1417)
+  * Changed Signal to remove connection when they're being disconnected: [#1462](https://github.com/dartsim/dart/pull/1462)
 
 * Collision
 

dart/common/Signal.cpp

@@ -94,25 +94,14 @@ Connection::~Connection()
 //==============================================================================
 bool Connection::isConnected() const
 {
-  std::shared_ptr<signal::detail::ConnectionBodyBase> connectionBody(
-      mWeakConnectionBody.lock());
-
-  if (nullptr == connectionBody)
-    return false;
-
-  return connectionBody->isConnected();
+  return !mWeakConnectionBody.expired();
 }
 
 //==============================================================================
 void Connection::disconnect() const
 {
-  std::shared_ptr<signal::detail::ConnectionBodyBase> connectionBody(
-      mWeakConnectionBody.lock());
-
-  if (nullptr == connectionBody)
-    return;
-
-  connectionBody->disconnect();
+  if (auto connectionBody = mWeakConnectionBody.lock())
+    connectionBody->disconnect();
 }
 
 //==============================================================================

dart/common/Signal.hpp

@@ -37,6 +37,7 @@
 #include <memory>
 #include <set>
 
+#include "dart/common/Deprecated.hpp"
 #include "dart/common/detail/ConnectionBody.hpp"
 
 namespace dart {
@@ -69,6 +70,7 @@ class Connection
 
   /// Disconnect the connection
   void disconnect() const;
+  // TODO(JS): Make this non-const in the next major release
 
   template <typename _Signature, template <class> class Combiner>
   friend class Signal;
@@ -115,7 +117,7 @@ class Signal<_Res(_ArgTypes...), Combiner>
   using SlotType = std::function<ResultType(_ArgTypes...)>;
   using SignalType = Signal<_Res(_ArgTypes...), Combiner>;
 
-  using ConnectionBodyType = signal::detail::ConnectionBody<SlotType>;
+  using ConnectionBodyType = signal::detail::ConnectionBody<Signal>;
   using ConnectionSetType = std::set<
       std::shared_ptr<ConnectionBodyType>,
       std::owner_less<std::shared_ptr<ConnectionBodyType>>>;
@@ -135,11 +137,17 @@ class Signal<_Res(_ArgTypes...), Combiner>
   /// Disconnect given connection
   void disconnect(const Connection& _connection) const;
 
+  /// Disconnect a connection
+  void disconnect(const std::shared_ptr<ConnectionBodyType>& connectionBody);
+
   /// Disconnect all the connections
   void disconnectAll();
 
   /// Cleanup all the disconnected connections
+  DART_DEPRECATED(6.10)
   void cleanupConnections();
+  // This explicit connection cleaning is no longer necessary because now a
+  // connection gets removed when it's disconnected.
 
   /// Get the number of connections
   std::size_t getNumConnections() const;
@@ -165,7 +173,7 @@ class Signal<void(_ArgTypes...)>
   using SlotType = std::function<void(_ArgTypes...)>;
   using SignalType = Signal<void(_ArgTypes...)>;
 
-  using ConnectionBodyType = signal::detail::ConnectionBody<SlotType>;
+  using ConnectionBodyType = signal::detail::ConnectionBody<Signal>;
   using ConnectionSetType = std::set<
       std::shared_ptr<ConnectionBodyType>,
       std::owner_less<std::shared_ptr<ConnectionBodyType>>>;
@@ -185,11 +193,17 @@ class Signal<void(_ArgTypes...)>
   /// Disconnect given connection
   void disconnect(const Connection& _connection) const;
 
+  /// Disconnect given connection
+  void disconnect(const std::shared_ptr<ConnectionBodyType>& connectionBody);
+
   /// Disconnect all the connections
   void disconnectAll();
 
   /// Cleanup all the disconnected connections
+  DART_DEPRECATED(6.10)
   void cleanupConnections();
+  // This explicit connection cleaning is no longer necessary because now a
+  // connection gets removed when it's disconnected.
 
   /// Get the number of connections
   std::size_t getNumConnections() const;

dart/common/detail/ConnectionBody.cpp

@@ -38,30 +38,12 @@ namespace common {
 namespace signal {
 namespace detail {
 
-//==============================================================================
-ConnectionBodyBase::ConnectionBodyBase() : mIsConnected(true)
-{
-  // Do nothing
-}
-
 //==============================================================================
 ConnectionBodyBase::~ConnectionBodyBase()
 {
   // Do nothing
 }
 
-//==============================================================================
-void ConnectionBodyBase::disconnect()
-{
-  mIsConnected = false;
-}
-
-//==============================================================================
-bool ConnectionBodyBase::isConnected() const
-{
-  return mIsConnected;
-}
-
 } // namespace detail
 } // namespace signal
 

dart/common/detail/ConnectionBody.hpp

@@ -46,70 +46,85 @@ class ConnectionBodyBase
 {
 public:
   /// Constructor
-  ConnectionBodyBase();
+  ConnectionBodyBase() = default;
 
   /// Destructor
   virtual ~ConnectionBodyBase();
 
   /// Disconnect
-  void disconnect();
-
-  /// Get true if this connection body is connected to the signal
-  bool isConnected() const;
-
-protected:
-  /// Connection flag
-  bool mIsConnected;
+  virtual void disconnect() = 0;
 };
 
 /// class ConnectionBody
-template <typename SlotType>
-class ConnectionBody : public ConnectionBodyBase
+template <typename SignalType>
+class ConnectionBody final
+  : public ConnectionBodyBase,
+    public std::enable_shared_from_this<ConnectionBody<SignalType>>
 {
 public:
+  using SlotType = typename SignalType::SlotType;
+
   /// Constructor given slot
-  ConnectionBody(const SlotType& _slot);
+  ConnectionBody(SignalType& signal, const SlotType& _slot);
 
   /// Move constructor given slot
-  ConnectionBody(SlotType&& _slot);
+  ConnectionBody(SignalType& signal, SlotType&& _slot);
 
   /// Destructor
   virtual ~ConnectionBody();
 
+  /// Disconnect
+  void disconnect() override;
+
   /// Get slot
-  const SlotType& getSlot();
+  const SlotType& getSlot() const;
 
 private:
+  /// Signal of this connection
+  SignalType& mSignal;
+  // Holding the reference of the Signal is safe because the lifetime of this
+  // ConnectionBody is always shorter than the Signal since the Signal has the
+  // ownership of this ConnectionBody.
+
   /// Slot
   SlotType mSlot;
 };
 
 //==============================================================================
-template <typename SlotType>
-ConnectionBody<SlotType>::ConnectionBody(const SlotType& _slot)
-  : ConnectionBodyBase(), mSlot(_slot)
+template <typename SignalType>
+ConnectionBody<SignalType>::ConnectionBody(
+    SignalType& signal, const SlotType& _slot)
+  : ConnectionBodyBase(), mSignal(signal), mSlot(_slot)
 {
   // Do nothing
 }
 
 //==============================================================================
-template <typename SlotType>
-ConnectionBody<SlotType>::ConnectionBody(SlotType&& _slot)
-  : ConnectionBodyBase(), mSlot(std::forward<SlotType>(_slot))
+template <typename SignalType>
+ConnectionBody<SignalType>::ConnectionBody(SignalType& signal, SlotType&& _slot)
+  : ConnectionBodyBase(), mSignal(signal), mSlot(std::forward<SlotType>(_slot))
 {
   // Do nothing
 }
 
 //==============================================================================
-template <typename SlotType>
-ConnectionBody<SlotType>::~ConnectionBody()
+template <typename SignalType>
+ConnectionBody<SignalType>::~ConnectionBody()
 {
   // Do nothing
 }
 
 //==============================================================================
-template <typename SlotType>
-const SlotType& ConnectionBody<SlotType>::getSlot()
+template <typename SignalType>
+void ConnectionBody<SignalType>::disconnect()
+{
+  mSignal.disconnect(this->shared_from_this());
+}
+
+//==============================================================================
+template <typename SignalType>
+const typename ConnectionBody<SignalType>::SlotType&
+ConnectionBody<SignalType>::getSlot() const
 {
   return mSlot;
 }