proper separation of worker and widget



  • 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.


  • Qt Champions 2016

    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);
    


  • @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.


  • Qt Champions 2016

    @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);
    


  • 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...


  • Lifetime Qt Champion

    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.



  • 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...


  • Lifetime Qt Champion

    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.



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



  • @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);
    
    

  • Lifetime Qt Champion

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

    Out of curiosity, what wiki page shows that ?



  • @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


  • Lifetime Qt Champion

    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.



  • @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


  • Lifetime Qt Champion

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


Log in to reply
 

Looks like your connection to Qt Forum was lost, please wait while we try to reconnect.