Fix signal to callable connects

src/PythonQtSignal.cpp

@@ -233,18 +233,71 @@ static PyObject* PythonQtSignalFunction_typeName(PythonQtSignalFunctionObject* t
   return PythonQtMemberFunction_typeName(type->m_ml);
 }
 
+// Find out if we can connect directly to the given Python object, without creating a separate receiver.
+// If yes, provide target object and signature for the connect call.
+static bool extractSignalTarget(PyObject* object, QObject*& targetObj, QByteArray& targetSignature)
+{
+  static PyObject* qtSlots = PyUnicode_FromString("_qtSlots");
+  if (PyObject_TypeCheck(object, &PythonQtSignalFunction_Type)) {
+    PythonQtSignalFunctionObject* type = (PythonQtSignalFunctionObject*)object;
+    PythonQtInstanceWrapper* self = (PythonQtInstanceWrapper*)type->m_self;
+    if (self->_obj) {
+      // connecting to another signal
+      targetObj = self->_obj;
+      targetSignature = QByteArray("2") + type->m_ml->signature();
+      return true;
+    }
+  } else if (PyMethod_Check(object)) {
+    PyObject* instance = PyMethod_Self(object);
+    if (PyObject_TypeCheck(instance, &PythonQtInstanceWrapper_Type)) {
+      PythonQtInstanceWrapper* typedInstance = (PythonQtInstanceWrapper*)instance;
+      if (!typedInstance->_wrappedPtr) {
+        // It's a QObject-derived class
+        targetObj = typedInstance->_obj;
+        PyObject* function = PyMethod_Function(object);
+        if (PyObject_HasAttr(function, qtSlots)) {
+          // connecting to a slot
+          PyObject* signatures = PyObject_GetAttr(function, qtSlots);
+          Py_ssize_t count = PyList_Size(signatures);
+          // TODO: Find the best matching signature;
+          // currently we only connect to the actual slot if only one slot signature is associated
+          // with this callable - if there are more, we just connect to the callable and let
+          // the callable figure the arguments out - but in this case Qt::DirectConnection is
+          // used instead of Qt::AutoConnection, which can be suprising if threads are involved.
+          if (count == 1) {
+            PyObject* signature = PyList_GET_ITEM(signatures, 0);
+            // Retrieve slot signature
+            QByteArray sig = PyUnicode_AsUTF8(signature);
+            targetSignature = QByteArray("1") + sig.split(' ')[1]; // include slot prefix
+            return true;
+          }
+        }
+      }
+    }
+  }
+  return false;
+}
+
 static PyObject* PythonQtSignalFunction_connect(PythonQtSignalFunctionObject* type, PyObject* args)
 {
   if (PyObject_TypeCheck(type->m_self, &PythonQtInstanceWrapper_Type)) {
     PythonQtInstanceWrapper* self = (PythonQtInstanceWrapper*)type->m_self;
     if (self->_obj) {
       Py_ssize_t argc = PyTuple_Size(args);
+      QByteArray sourceSignature = QByteArray("2") + type->m_ml->signature();
       if (argc == 1) {
         // connect with Python callable
         PyObject* callable = PyTuple_GET_ITEM(args, 0);
-        bool result =
-          PythonQt::self()->addSignalHandler(self->_obj, QByteArray("2") + type->m_ml->signature(), callable);
-        return PythonQtConv::GetPyBool(result);
+        QObject* targetObj;
+        QByteArray targetSignature;
+        if (extractSignalTarget(callable, targetObj, targetSignature)) {
+          // Do a regular signal/slot (or signal/signal) connect.
+          QObject::connect(self->_obj, sourceSignature, targetObj, targetSignature, Qt::AutoConnection);
+          return PythonQtConv::GetPyBool(true);
+        } else {
+          bool result = PythonQt::self()->addSignalHandler(self->_obj, sourceSignature, callable);
+          return PythonQtConv::GetPyBool(result);
+        }
       } else {
         PyErr_SetString(PyExc_ValueError, "Called connect with wrong number of arguments");
       }
@@ -259,15 +312,23 @@ static PyObject* PythonQtSignalFunction_disconnect(PythonQtSignalFunctionObject*
     PythonQtInstanceWrapper* self = (PythonQtInstanceWrapper*)type->m_self;
     if (self->_obj) {
       Py_ssize_t argc = PyTuple_Size(args);
-      QByteArray signal = QByteArray("2") + type->m_ml->signature();
+      QByteArray sourceSignature = QByteArray("2") + type->m_ml->signature();
       if (argc == 1) {
         // disconnect with Python callable
         PyObject* callable = PyTuple_GET_ITEM(args, 0);
-        bool result = PythonQt::self()->removeSignalHandler(self->_obj, signal, callable);
-        return PythonQtConv::GetPyBool(result);
+        QObject* targetObj;
+        QByteArray targetSignature;
+        if (extractSignalTarget(callable, targetObj, targetSignature)) {
+          // Do a regular signal/slot (or signal/signal) disconnect.
+          QObject::disconnect(self->_obj, sourceSignature, targetObj, targetSignature);
+          return PythonQtConv::GetPyBool(true);
+        } else {
+          bool result = PythonQt::self()->removeSignalHandler(self->_obj, sourceSignature, callable);
+          return PythonQtConv::GetPyBool(result);
+        }
       } else if (argc == 0) {
-        bool result = PythonQt::self()->removeSignalHandler(self->_obj, signal, nullptr);
-        result |= QObject::disconnect(self->_obj, signal, nullptr, nullptr);
+        bool result = PythonQt::self()->removeSignalHandler(self->_obj, sourceSignature, nullptr);
+        result |= QObject::disconnect(self->_obj, sourceSignature, nullptr, nullptr);
         return PythonQtConv::GetPyBool(result);
       } else {
         PyErr_SetString(PyExc_ValueError, "Called disconnect with wrong number of arguments");

src/PythonQtSignalReceiver.cpp

@@ -46,6 +46,8 @@
 #include <QMetaObject>
 #include <QMetaMethod>
 
+#include <algorithm>
+
 // use -2 to signal that the variable is uninitialized
 int PythonQtSignalReceiver::_destroyedSignal1Id = -2;
 int PythonQtSignalReceiver::_destroyedSignal2Id = -2;
@@ -176,7 +178,8 @@ PythonQtSignalReceiver::PythonQtSignalReceiver(QObject* obj)
   // force decorator/enum creation
   _objClassInfo->decorator();
 
-  _slotCount = staticMetaObject.methodOffset();
+  _nextSlotID = staticMetaObject.methodOffset();
+  _nextTargetIndex = 0;
 }
 
 PythonQtSignalReceiver::~PythonQtSignalReceiver()
@@ -189,20 +192,49 @@ PythonQtSignalReceiver::~PythonQtSignalReceiver()
   }
 }
 
+#define MAX_SLOT_ID 32768
+
 bool PythonQtSignalReceiver::addSignalHandler(const char* signal, PyObject* callable)
 {
   bool flag = false;
   int sigId = getSignalIndex(signal);
   if (sigId >= 0) {
+    // find next free slot ID:
+    const int firstIndex = staticMetaObject.methodOffset();
+    if (firstIndex + _targets.size() >= MAX_SLOT_ID) {
+      // no free slot anymore
+      std::cerr << "Too many callables connected to signals of object " << _obj << std::endl;
+      return false;
+    }
+    // this loop is only triggered in case _nextTargetIndex was reset by removeSignalHandler:
+    while (_nextTargetIndex < _targets.size() && _targets[_nextTargetIndex].slotId() < _nextSlotID) {
+      _nextTargetIndex++;
+    }
+    // find a free slot ID (and the position in the target list)
+    while (_nextTargetIndex < _targets.size() && _targets[_nextTargetIndex].slotId() == _nextSlotID) {
+      _nextTargetIndex++;
+      _nextSlotID++;
+      if (_nextSlotID == MAX_SLOT_ID) {
+        // wrap around
+        _nextSlotID = firstIndex;
+        _nextTargetIndex = 0;
+      }
+    }
     // create PythonQtMethodInfo from signal
     QMetaMethod meta = _obj->metaObject()->method(sigId);
     const PythonQtMethodInfo* signalInfo = PythonQtMethodInfo::getCachedMethodInfo(meta, _objClassInfo);
-    PythonQtSignalTarget t(sigId, signalInfo, _slotCount, callable);
-    _targets.append(t);
+    PythonQtSignalTarget t(sigId, signalInfo, _nextSlotID, callable);
+    _targets.insert(_nextTargetIndex, t);
     // now connect to ourselves with the new slot id
-    QMetaObject::connect(_obj, sigId, this, _slotCount, Qt::AutoConnection, nullptr);
+    QMetaObject::connect(_obj, sigId, this, _nextSlotID, Qt::DirectConnection, nullptr);
 
-    _slotCount++;
+    _nextTargetIndex++;
+    _nextSlotID++;
+    if (_nextSlotID == MAX_SLOT_ID) {
+      // wrap around
+      _nextSlotID = firstIndex;
+      _nextTargetIndex = 0;
+    }
     flag = true;
 
     if (sigId == _destroyedSignal1Id || sigId == _destroyedSignal2Id) {
@@ -242,11 +274,14 @@ bool PythonQtSignalReceiver::removeSignalHandler(const char* signal, PyObject* c
       }
     }
   }
-  if ((foundCount > 0) && ((sigId == _destroyedSignal1Id) || (sigId == _destroyedSignal2Id))) {
-    _destroyedSignalCount -= foundCount;
-    if (_destroyedSignalCount == 0) {
-      // make ourself child of QObject again, to get deleted when the object gets deleted
-      this->setParent(_obj);
+  if (foundCount > 0) {
+    _nextTargetIndex = 0; //< must find _nextTargetIndex anew on next addSignalHandler call
+    if ((sigId == _destroyedSignal1Id) || (sigId == _destroyedSignal2Id)) {
+      _destroyedSignalCount -= foundCount;
+      if (_destroyedSignalCount == 0) {
+        // make ourself child of QObject again, to get deleted when the object gets deleted
+        this->setParent(_obj);
+      }
     }
   }
   return foundCount > 0;
@@ -269,19 +304,22 @@ int PythonQtSignalReceiver::qt_metacall(QMetaObject::Call c, int id, void** argu
     QObject::qt_metacall(c, id, arguments);
   }
 
+  // Get Global Interpreter Lock, as a safeguard against cases when an signal is emitted from a thread
+  // while _targets is modified because a connect/disconnect is done from Python code (which would also hold the GIL)
+  PYTHONQT_GIL_SCOPE
   bool shouldDelete = false;
-  for (const PythonQtSignalTarget& t : qAsConst(_targets)) {
-    if (t.slotId() == id) {
-      const int sigId = t.signalId();
-      t.call(arguments);
-      // if the signal is the last destroyed signal, we delete ourselves
-      if ((sigId == _destroyedSignal1Id) || (sigId == _destroyedSignal2Id)) {
-        _destroyedSignalCount--;
-        if (_destroyedSignalCount == 0) {
-          shouldDelete = true;
-        }
+  auto it = std::lower_bound(_targets.begin(), _targets.end(), id,
+    [](const PythonQtSignalTarget& t, int id) -> bool { return t.slotId() < id; });
+  if (it != _targets.end() && it->slotId() == id) {
+    const PythonQtSignalTarget& t = *it;
+    const int sigId = t.signalId();
+    t.call(arguments);
+    // if the signal is the last destroyed signal, we delete ourselves
+    if ((sigId == _destroyedSignal1Id) || (sigId == _destroyedSignal2Id)) {
+      _destroyedSignalCount--;
+      if (_destroyedSignalCount == 0) {
+        shouldDelete = true;
       }
-      break;
     }
   }
   if (shouldDelete) {

src/PythonQtSignalReceiver.h

@@ -135,9 +135,10 @@ class PythonQtSignalReceiver : public PythonQtSignalReceiverBase
 
   QObject* _obj;
   PythonQtClassInfo* _objClassInfo;
-  int _slotCount;
+  int _nextSlotID;
+  int _nextTargetIndex;
   int _destroyedSignalCount;
-  // linear list may get slow on multiple targets, but I think typically we have many objects and just a few signals
+  // Targets are sorted by slot ID.
   QList<PythonQtSignalTarget> _targets;
 
   static int _destroyedSignal1Id;