proper separation of worker and widget

mzimmers

Hi all -

I'm writing a small program with 2 primary objects: a worker and a widget. These are not in separate threads.

I'm often finding myself needing to pass information between the two, and I've been using signals for this, perhaps improperly. One example of something I've done that just doesn't look right is the notification of the worker when a combo box changes. Since the combo box is a member of my widget class, I'm doing this:

CdWidget::CdWidget(QWidget *parent) :
    QWidget(parent),
    ui(new Ui::Widget)
{
    ui->setupUi(this);
    connect(ui->comboBox, SIGNAL(activated(QString)), this, SLOT(on_comboBox_currentIndexChanged(QString)));
}
...
void CdWidget::on_comboBox_currentIndexChanged(const QString &portName)
{
    emit comPortChanged(portName);
}

and in main() I do this:

    QObject::connect(&w, &CdWidget::comPortChanged, &s, &CdSerial::setComPort);

Seems like I'm adding an unnecessary level of complexity, but I don't know how to go about resolving it. Any suggestions? Thanks.

kshegunov

on_comboBox_currentIndexChanged is just superfluous.

connect(ui->comboBox, SIGNAL(activated(const QString &)), this, SIGNAL(comPortChanged(const QString &)));

or if you're using Qt 5, which you should:

connect(ui->comboBox, &QComboBox::activated, this, &CdWidget::comPortChanged);

mzimmers

@kshegunov said in proper separation of worker and widget:

comPortChanged

But comPortChanged is part of the worker object, not the widget.
It's as though I need a containing object that can "connect" the widget and the worker, but I imagine there's a better way to do this that I'm not aware of.

kshegunov

@mzimmers said in proper separation of worker and widget:

But comPortChanged is part of the worker object, not the widget.

Not according to your code:

QObject::connect(&w, &CdWidget::comPortChanged, &s, &CdSerial::setComPort);

mzimmers

I'm sorry for the ambiguity -- CdSerial is my worker class. I need a signal from an object in CdWidget (the combo box) to signal CdSerial.

int main(int argc, char *argv[])
{
     QApplication a(argc, argv);
    CdWidget w;
    CdSerial s;
...
    QObject::connect(&w, &CdWidget::comPortChanged, &s, &CdSerial::setComPort);
...

I may not be going about this the right way...

SGaist

Hi,

From the looks of it, you have a signal named comPortChanged in CdWiget, so @kshegunov just suggested to use signal forwarding by connecting your QComboBox signal to your CdWidget signal.

mzimmers

Sorry, guys - my statement above was wrong. (Trying to solve too many issues at once.)

So, apart from my unnecessary step, do I correctly understand the proper technique as:

comboBox signals CdWidget
CdWidget signals CdSerial

I can imagine this producing a ton of connects in larger projects. Is this a sign of bad design on my part?

Thanks...

SGaist

No, that keeps your different pieces cleanly separated.

Your CdWidget provides a signal that says: "com port name has changed". It's comPortChanged. You connect that signal on your CdSerial object -> simple and clean and done in main.cpp.

Internally in CdWidget, you forward your combobox's indexChanged to the comPortChanged signal -> simple and clean. You could be using something else to select the com port name and maybe have to use a private slot before calling comPortChanged. That would also be OK. The advantage of doing this is that you can easily change CdWidget's internal without disrupting the rest of your code.

mzimmers

OK, thanks guys. Good education. I also didn't realize you could connect a signal to a signal as @kshegunov did in his example.

J.Hilk

@mzimmers QObject::connect is a very, very powerful tool. with Qt5 and c++ 11 you can even connect a lambda function to any Signal:

//Example taken directly from the wiki
connect(    sender, &Sender::valueChanged,  [=]( const QString &newValue ) { receiver->updateValue( "senderValue", newValue ); }
);

If you pass Connect the receiver as reference you will not have to worry about clean up either

connect(    sender, &Sender::valueChanged,  receiver, [=]( const QString &newValue ) { receiver->updateValue( "senderValue", newValue ); }

...
// receiver gets deleted
receiver->deleteLater();

....
//No crash
 emit valueChanged("STRING);

SGaist

@J-Hilk In the case at hand a lambda is really overkill.

Out of curiosity, what wiki page shows that ?

J.Hilk

@SGaist said in proper separation of worker and widget:

@J-Hilk In the case at hand a lambda is really overkill.

Out of curiosity, what wiki page shows that ?

I have the example from here:
https://wiki.qt.io/New_Signal_Slot_Syntax

SGaist

Ok, thanks.

However what you are citing, AFAIK, is a reflection regarding the handling of lambda disconnection.

~~From QObject's documentation there's no overload matching your proposed connect statement.~~

J.Hilk

@SGaist
are you sure? Isn't this one fitting?

connect(const QObject *sender, PointerToMemberFunction signal, const QObject *context, Functor functor, Qt::ConnectionType type)

However it works that way take this simple example:

    QPushButton * pushButton = new QPushButton();
    pushButton->show();
    QObject *obj = new QObject();

    connect(pushButton, &QPushButton::clicked,obj,[=]{
        QLabel * lbl = new QLabel();
        lbl->show();
        lbl->setText(QTime::currentTime().toString());
        connect(qApp,&QApplication::aboutToQuit, lbl,&QLabel::deleteLater);
        obj->deleteLater();
    });

Only 1 Lable will appear no mater how often you press the Button

SGaist

Completely my bad, I missed that one and the worse is that I remember now reading earlier this year about it !

Thanks for pointing it out :)