proper separation of worker and widget

[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:

1. comboBox signals CdWidget
2. 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 :)