Signal/Slot best practice with composition

DRoscoe

I'm developing a communication class, which is designed to be agnostic to the IPC employed. The constructor looks like this:

TapDisplayCommunicationHandler::TapDisplayCommunicationHandler(
                                  AbstractCommunicationHandler* ipc_handler_in)
: QObject(),
  ipc_handler(ipc_handler_in),
  factory(),
  header (25,0),
  is_compressed_message(false),
  heartbeat_timer()
{
  // this is necessary to ensure proper cleanup and to make sure this object
  // can be moved to another thread along with its parent, if desired
  ipc_handler->setParent(this);

  QObject::connect(this, SIGNAL(newMessage(const QByteArray&)),
                   this, SLOT(handleNewMessage(const QByteArray&)));

  heartbeat_timer.setParent(this);
}

The main app class which creates the instance of TapDisplayCommunicationHandler knows the IPC mechanism to be used, which has a common interface. The common interface of the IPC handler class defines signals such as:

  /// signal indicating loss of connection
  void connectionLost(const QString&);
  
  /// signal indicating successful connection
  void connectionEstablished();

  /// signal indicating that the system failed to connect to hardware (either initial or reactivate)
  void unableToConnect();

[...]

My question is this. Since the main app class creates the IPC handler instance, which is passed to the TapDisplayCommunicationHandler and integrated as a class member, is it acceptable to connect the signals from the IPC handler class to the main class directly, before passing it to the TapCommunicationHandler, or is it a better practice to have slots in TapCommunicationHandler to receive the signals from the IPC handler and then re-emit them to the main class? In the latter case, the main class would define connections with the TapCommunicationHandler instance instead of the IPC Handler instance

SGaist

Hi,

What does TapDisplayCommunicationHandler do with AbstractCommunicationHandler ?

DRoscoe

Hello @SGaist ,

In the current implementation it only implements a slot to handle the newMessage() signal from the abstract handler to decode the received QByteArray into a proper message and emitting it to the main class. It also implements a slot to handle serialization from message classes to QByteArray and emitting to the abstract handler for dispatch

kshegunov

Hi,
Perhaps I'm missing something, but why not just delegate the signals themselves. I.e. define a set of signals in the exposed (TapDisplayCommunicationHandler) class and connect the signals from AbstractCommunicationHandler to the signals of TapDisplayCommunicationHandler. Additionally, if you proceed like this, you can insert a middleman of a slot later (if needed) that can do some stuff before reemiting the signal, but this wouldn't change your exposed class' interface (i.e. the rest of the program wouldn't care).

DRoscoe

@kshegunov said in Signal/Slot best practice with composition:

Hi,
why not just delegate the signals themselves. I.e. define a set of signals in the exposed (TapDisplayCommunicationHandler) class and connect the signals from AbstractCommunicationHandler to the signals of TapDisplayCommunicationHandler.

I guess that's the crux of my question. Your comment about the middleman slot is a good one. My follow-up question is what do you mean about connecting signals?

kshegunov

@DRoscoe said in Signal/Slot best practice with composition:

My follow-up question is what do you mean about connecting signals?

I mean just connecting one signal to another signal. I don't believe that's written clearly enough in the docs, so many people are left with the impression you can only connect signals to slots. Here's an example (with aggregation and delegation of signals):

class A : public QObject
{
    Q_OBJECT

signals:
    void someSignal();
};

class B : public QObject
{
    Q_OBJECT
public:
    B()
    {
        QObject::connect(&a, &A::someSignal, this, &B::someSignal); // We just connect one signal to the other - i.e. delegating
    }

signals:
    void someSignal();

private:
    A a;
};

I hope that helps. As a side note I recommend you convert to the Qt5 style of connecting (i.e. with method pointers).

Kind regards.

DRoscoe

@kshegunov Thanks as usual for your input. This answers my question quite nicely!

SGaist

It even has a name:

signal forwarding

but indeed it looks like it's missing from the documentation

kshegunov

Glad to be of assistance.

@SGaist said in Signal/Slot best practice with composition:

it looks like it's missing from the documentation

I'm pretty sure it's written somewhere (I myself know it from reading the docs), but somehow it's mentioned only in passing and feels it's swept under the rug, which in turn leaves the ever-so-lasting doubt if it's a good practice, bad practice, allowed or discouraged.

DRoscoe

@kshegunov @SGaist You are correct, the documentation says you CAN do it, but doesn't actually elucidate the fact or provide examples