Difficulties connecting signals with slots (static members)

SnuggleKat

Hello,

I have implemented the Lua API into my Qt project. Now I'd like to print error messages when the user entered faulty Lua code into the TextEdit.
The MainWindow class contains the TextEdit where the user enteres their Lua script. It also features the TextEdit where I want errors to be shown if any occure.
The Lua API is implemented within another class. All members of a Lua class must be static. And here's the dead end for me since I can't call any non-static. So I tried solving this with the Signals and Slots mechanism.

I tried the following:

mainwindow.h (excerp)

#ifndef MAINWINDOW_H
#define MAINWINDOW_H

#include <QMainWindow>
#include<QDebug>
#include"cheats.h"
//...

class MainWindow : public QMainWindow
{
    Q_OBJECT

public:
    explicit MainWindow(QWidget *parent = nullptr);
    ~MainWindow();
    inline static Cheats* cheats;
    //...

private slots:
    //...

private:
    Ui::MainWindow *ui;
    QTimer* cheatTimer;
    QTimer* cheatTimerList;
    QStringList selectedLuaCheats;
    QStringList luaCheats;
    //...
    

public slots:
    void cancel(){ qDebug() << "it worked"; }
    //...
};
#endif // MAINWINDOW_H

mainwindow.cpp (excerp)

#include "mainwindow.h"
#include "ui_mainwindow.h"

MainWindow::MainWindow(QWidget *parent) : QMainWindow(parent), ui(new Ui::MainWindow)
{
    //...

    cheatTimer = new QTimer(this);// keeps the Lua scripts being executed (60 times a second)
    cheats = new Cheats(this); //here I pass the this-pointer to my Cheats object
    connect(cheatTimer, &QTimer::timeout, &Cheats::executeCheat); //this works
    connect(cheats, SIGNAL(error()), this, SLOT(cancel())); //connects but doesn't work
//connect(cheats, &Cheats::error, &MainWindow::cancel); //neither works this
}

//...

void MainWindow::on_pushButton_executeCheats_clicked() //enables Lua script. works
{
    //..

    std::string val = ui->plainTextEdit_luaCheat->toPlainText().toStdString();
    cheats->setCheat(val);
    cheats->sendLuaCheat();
    cheatTimer->start(16);

    //...
}


void MainWindow::on_pushButton_terminateCheats_clicked() //terminates Lua scripts. Works
{
    cheatTimer->stop();
    //...
}

//...

cheats.h (excerp)

#ifndef CHEATS_H
#define CHEATS_H
#include<string>
#include<lua.hpp>
#include<QDebug>
#include<QTimer>
#include<QMetaObject>
#include<QObject>
//...

class Cheats : public QObject
{
    Q_OBJECT

private:
    std::string script;
    std::string result;
    inline static QStringList cheatScripts;
    inline static std::string sscript;
    inline static std::string sresult;
    inline static lua_State* L;
    //...

public:
    explicit Cheats(QObject* parent);
    ~Cheats(){}
    static bool checkLua(lua_State* L, int val);
    void sendLuaCheat();
    QString logLua(std::string& result);
    void setCheat(std::string& val){ script = val; }
    static void routine(lua_State* L);
    static int writeToRAM(lua_State* L);
    static int readFromRAM(lua_State* L);
    //...

public slots:
    //slots to be called from the MainWindow class
    static void executeCheat();
    static void executeCheatList();

signals:
    inline void error();
};
#endif // CHEATS_H

cheats.cpp (excerpt)

#include "cheats.h"
#include "mainwindow.h"

Cheats::Cheats(QObject* parent)
{
    this->parent = parent;
    connect(this, SIGNAL(error()), this->parent, SLOT(cancel())); //connects but doesn't work
}

void Cheats::error()
{
    qDebug() << "signal sent";
}

bool Cheats::checkLua(lua_State* L, int val)
{
    if(val != LUA_OK)
    {
        //...
        return false;
    }
    return true;
}

void Cheats::executeCheat()
{
    if(checkLua(L, luaL_dostring(L, sscript.c_str())))
    {
        //...
    }
    else
    {
        checkCheat.ok = false;
        checkCheat.errorStr = "Error Text-Cheat: ";
        checkCheat.errorStr.append(lua_tostring(L, -1));
        Cheats x = Cheats(parent);
        emit x.error(); //error() is called but the slot won't triggered
    }
}

Pl45m4

@SnuggleKat said in Difficulties connecting signals with slots (static members):

connect(this, SIGNAL(error()), this->parent, SLOT(cancel()));

This is bad coding style. It makes the connection parent depending.

@SnuggleKat said in Difficulties connecting signals with slots (static members):

void Cheats::error()
{
qDebug() << "signal sent";
}

Dont implement signals, just define signals in header and emit them, when you need them.

Chris Kawa

@SnuggleKat said:

Cheats x = Cheats(parent);
emit x.error(); //error() is called but the slot won't triggered

This creates an instance and emits a signal from it and then

cheats = new Cheats(this); //here I pass the this-pointer to my Cheats object
connect(cheats, SIGNAL(error()), this, SLOT(cancel())); //connects but doesn't work

this creates a totally different instance and you connect a slot to that. This won't work like that. You need the same Cheats instance in the connection and emitting signals.

If everything in Cheats is static don't make it a QObject. It's a waste. You could simply store a pointer to your cancel() function along with the object pointer in some static array and call it instead of using signals.
If you really want a signal/slot connection you could make an emitter singleton class and use that. For example:

emit CheatsEmitter::instance()->error();

and connect to that same singleton:

connect(CheatsEmitter::instance(), &CheatsEmitter::error, this, &MainWindow::cancel);

SnuggleKat

@Chris-Kawa Thanks, I decided to try the singleton pattern since this class is initialized only once.
For some reason I still can't get it working. The signal won't be triggered.

Cheats.h (excerp)

class Cheats : public QObject
{
    Q_OBJECT

private:
    Cheats(/*QObject* parent*/);
 
    //...

public:
    ~Cheats(){ }
    Cheats(const Cheats&) = delete;
    inline static Cheats& getInstance()
    {
        static Cheats sInstance;
        return sInstance;
    }

    //...

public slots:
    //slots to be called from the MainWindow class
    static void executeCheat();
    static void executeCheatList();

signals:
    inline void error();
};

cheats.cpp (excerp)

Cheats::Cheats(/*QObject* parent*/)
{
    //this->parent = parent;
}

void Cheats::error()
{
    qDebug() << "signal";
}

//...

void Cheats::executeCheat()
{
    if(checkLua(L, luaL_dostring(L, sscript.c_str())))
    {
        checkCheat.ok = true;
    }
    else
    {
        checkCheat.ok = false;
        checkCheat.errorStr = "Error Text-Cheat: ";
        checkCheat.errorStr.append(lua_tostring(L, -1));
        emit Cheats::getInstance().error(); 
    }
}

//...

mainwindow.h (excerp)

class MainWindow : public QMainWindow
{
    Q_OBJECT

public:
    explicit MainWindow(QWidget *parent = nullptr);

    //...
    //no Cheats instance needed due to singleton pattern(?)
public slots:
    void cancel(){ qDebug() << "it works"; }
};

mainwindow.cpp (excerp)

MainWindow::MainWindow(QWidget *parent) : QMainWindow(parent), ui(new Ui::MainWindow)
{
    ui->setupUi(this);
  
    //...   

    connect(&Cheats::getInstance(), &Cheats::error, this, &MainWindow::cancel);
}

I tried the following ways to connect my signal with my slot:

    connect(&Cheats::getInstance(), &Cheats::error, &MainWindow::cancel);
    //Error: no matching function for call to 'QObject::connect(const Object*&, void (Cheats::*&)(), const Object*&, void (MainWindow::*&)(), Qt::ConnectionType)'

    //The following 3 methodes neither result in a compile error or trigger the slot
    connect(&Cheats::getInstance(), &Cheats::error, this, &MainWindow::cancel);
    connect(&Cheats::getInstance(), SIGNAL(error()), SLOT(cancel()));
    connect(&Cheats::getInstance(), SIGNAL(error()), this, SLOT(cancel()));

I'm really clueless right now. Sorry

6thC

Immediate fix:
Drop the double dereference here: connect(Cheats::getInstance(), &Cheats::error, this, &MainWindow::cancel);

Long term, understand the why:
You're trying to define: connnecting from loosly coupled (intrinsically disconnected/disparate) :
object's (memory address) first arg
functions. (memory address) second arg
to (exact same deal but again)
object's (memory address) third arg
functions. (memory address) fourth arg

Even with connect's using lambda's - this is the underlying basic concept/mechanism.
So, just make sure you pass a reference or a pointer, not both (&|&), not none.

I prefer the more modern connection syntax which provides compile time type protection/detection and has proven particularly very useful. Things imo to immediately note as a point of their own:

The signals and slots mechanism is type safe: The signature of a signal must match the signature of the receiving slot. (In fact a slot may have a shorter signature than the signal it receives because it can ignore extra arguments.)

) Since the signatures are compatible, the compiler can help us detect type mismatches when using the function pointer-based syntax.

The string-based SIGNAL and SLOT syntax will detect type mismatches at runtime.

I (personal opinion) would advise against mixing the new and old when you're writing new code yourself as there's no benefit going old and actually is losing capabilities and increasing risk.

connect(&Cheats::getInstance(), &Cheats::error, &MainWindow::cancel);
//Error: no matching function for call to 'QObject::connect(const Object*&, void (Cheats::&)(), const Object&, void (MainWindow::*&)(), Qt::ConnectionType)'

The trick is getting familiar to reading that message and the types it feeds back that it tried.

const Object*&, void (Cheats::*&)

You want to use a reference or ptr not a ref to a ptr, this is actually a good case in point to sell you on how great it is to have the new syntax type checking @ compiler time saving you from runtime fun.
Because you are using the new syntax (kinda, you left out the second/ to object's instance ref/ptr) types are checked and the compiler spewed.

How I started out signals and slots I felt I didn't get it until reading https://doc.qt.io/qt-5/signalsandslots.html many times, just using them, you will get it and once you do it's not hard. I think just by using it and reading about the concept of sig/slots will cause that epiphany, if this didn't put you on course, the documentation really will, you'll get it.

The next really powerful thing would be: connecting across threads. They make threading really nice.

p.s. sorry if this is a bit disorganised I'm @ work and short on time, hope this helps.

Chris Kawa

@SnuggleKat This one is correct:

connect(&Cheats::getInstance(), &Cheats::error, this, &MainWindow::cancel);

and if it's not triggering then either you just don't emit the signal, one of the objects is dead already or you're again connecting different instances.

There are also a couple of other problems:

Don't make static QObjects. Every QObject should be created after QApplication object is created and destroyed before QApplication object is. A static QObject will outlive QApplication object and can lead to leaks or crashes at shutdown. Create your singleton dynamically and destroy it e.g. by connecting its deleteLater() to the application's object aboutToQuit signal.
Don't inline static definitions in a header. If you ever include that in another module you'll again get different instances.

@6thC You can't drop the reference. getInstance returns a reference and connect takes a pointer so you need to take an address of that object. The syntax is ok. The other advice is also not correct - if you drop the reference from getInstance you're returning a copy and this would defeat the purpose of a singleton. Also QObjects are not copyable.

6thC

@Chris-Kawa thanks, I appreciate your comment. I just mostly saw the missing arg and double deref and tried to highlight it.

I'm just trying to keep a dev moving forward too, I was a bit hasty and had an edit in works to drop that recommendation as I saw that later, cheers.

My brain just implied that if you had an instance function of course your pass a ref/ptr but your right - details matter and that was bad advice (cause then it'd be an object/copy ctors invoked/yadda yadda) . I appreciate the pickup, hopefully that doesn't confuse things now.

SnuggleKat

@6thC thanks! Dropping the double dereference gives me: "no matching member function for call to connect"
I will study more about it when I have time. Thanks for the link.

@Chris-Kawa thanks, removing some of the inline definitions result in "multiple definition" errors.
Removed it from my singleton getInstance() which didn't help. Removing it from my signal gives me the "multiple definition" error

EDIT: got it working! Just didn't know signals mustn't be defined...

Pl45m4

@SnuggleKat said in Difficulties connecting signals with slots (static members):

Just didn't know signals mustn't be defined...

If you mean "implemented" by "defined", then I wrote this in my first reply :)
Yes, you dont implement signals. You just set the signal signature in your header file. After that, you can emit your signal to notify connected classes (-> slots)