Circular dependency fix/alternative?

[Leutzig]

Hi,

I have this problem with my Qt application that I was hoping one of you could help me with.
In this application I have the classes ColorSelect and DeviceDialog, each with its own ui. But I'm getting errors because I'm trying to call functions from each other.
I know that this isn't allowed because of a thing called circular dependency, so I was wondering what I can do instead that is allowed/won't give me errors?
(I have included devicedialog.h in colorselect.h and vice versa)

in my colorselect.cpp:

void ColorSelect::on_listWidget_itemDoubleClicked(QList WidgetItem *item)
{
//launches the device dialog where data is entered in line edits
DeviceDialog dialog;
dialog.setLineEdits(item->data(Qt::UserRole).toString());
dialog.setModal(true);
dialog.exec();
}

in my devicedialog.cpp:

void DeviceDialog::on_buttonBox_accepted()
{
//sets listWidgetItem data (found in colorselect.ui) with text from line edits (found in devicedialog.ui)
colorSelect->setItemData(ui->houseLineEdit->text(), ui->unitLineEdit->text());
}

//Leutzig

[d_stranz]

(I have included devicedialog.h in colorselect.h and vice versa

Why? Do you have code in the header files that needs to know the signatures for the methods declared in either of these two classes? Or does each class simply need to know that there are classes by the names of ColorSelect and DeviceDialog?

In other words, if the ColorSelect header file only contains something like a pointer or reference to the DeviceDialog, then all you need is to place the statement (called a "forward declaration") before the other class' name is needed:

// ColorSelect .h
 
class DeviceDialog;  // forward declaration
 
class ColorSelect : public QWidget
{
    Q_OBJECT
 
public:
    ColorSelect( QWidget * parent );
 
//...
 
private:
   DeviceDialog * devDlg;
}

// ColorSelect .h

class DeviceDialog;  // forward declaration

class ColorSelect : public QWidget
{
    Q_OBJECT

public:
    ColorSelect( QWidget * parent );

//...

private:
   DeviceDialog * devDlg;
}

To copy to clipboard, switch view to plain text mode 

and the same for DeviceDialog.h. The only place you need to include the header files themselves is in the .cpp files where you need to know the definitions of the classes.

If the only place you reference the name of the other class is to define a variable (as in the example above), you don't need the forward declaration either. You can simply write "class DeviceDialog * devDlg;", which tells the compiler you're going to store a pointer to a class named "DeviceDialog" in that variable.

[Leutzig]

For some reason I get the errors:

error: member access into incomplete type 'class DeviceDialog'
dialog->setLineEdits(item->data(Qt::UserRole).toString());
^

forward declaration of 'DeviceDialog'
class DeviceDialog *dialog;
^

... And a few more of those from the other functions. Also the functions I'm trying to call are no longer "recognised" in the cpp's.

ColorSelect.h:

class ColorSelect : public QWidget
{
Q_OBJECT

public:
explicit ColorSelect(QWidget *parent = 0)
void setItemData(QString, QString);
~ColorSelect();

private slots:
void on_listWidget_itemDoubleClicked(QListWidgetItem *item);

private:
Ui::ColorSelect *ui;
class DeviceDialog *dialog;
};

DeviceDialog.h:

class DeviceDialog : public QDialog
{
Q_OBJECT

public:
explicit DeviceDialog(QWidget *parent = 0);
void setLineEdits(QString);
~DeviceDialog();

private slots:
void on_buttonBox_accepted();

private:
class ColorSelect *colorSelect;
Ui::DeviceDialog *ui;
};

[d_stranz]

Did you #include the header files in the .cpp files? Those compiler complaints indicate that you didn't. The forward declarations simply tell the compiler that classes of those names exist. If you want to use one of the classes, you have to include the header file so the compiler can find the class definitions.

If your compiler doesn't like the "class XYZ * pClass;" style, use the forward declaration version (at the top of the .h file, before the other class' definition.

Please use "CODE" tags when posting source code. When making a post, click "Go Advanced". On the toolbar, click the "#" icon. This will insert a pair of open/close CODE tags. Paste your code between them.

[Leutzig]

Yes, that was it. I didn't realise that both headers should be included in both cpp files. But I'm encountering a problem when using the pointer to the DeviceDialog object in the function on_listWidget_itemDoubleClicked from colorselect. The application crashes when write it like this:

void ColorSelect::on_listWidget_itemDoubleClicked(QListWidgetItem *item)
{
    dialog->setLineEdits(item->data(Qt::UserRole).toString());
    dialog->setModal(true);
    dialog->exec();
}

void ColorSelect::on_listWidget_itemDoubleClicked(QListWidgetItem *item)
{
    dialog->setLineEdits(item->data(Qt::UserRole).toString());
    dialog->setModal(true);
    dialog->exec();
}

To copy to clipboard, switch view to plain text mode 

But this works:

void ColorSelect::on_listWidget_itemDoubleClicked(QListWidgetItem *item)
{
    DeviceDialog dialog;
    dialog.setLineEdits(item->data(Qt::UserRole).toString());
    dialog.setModal(true);
    dialog.exec();
}

void ColorSelect::on_listWidget_itemDoubleClicked(QListWidgetItem *item)
{
    DeviceDialog dialog;
    dialog.setLineEdits(item->data(Qt::UserRole).toString());
    dialog.setModal(true);
    dialog.exec();
}

To copy to clipboard, switch view to plain text mode 

And for some odd reason my application also crashes in another function from colorselect:

void ColorSelect::setItemData(QString house, QString unit)
{
    ui->listWidget->currentItem()->setData(Qt::UserRole, house + unit);
}

void ColorSelect::setItemData(QString house, QString unit)
{
    ui->listWidget->currentItem()->setData(Qt::UserRole, house + unit);
}

To copy to clipboard, switch view to plain text mode 

And in this one I've tried doing other things with ui pointer but it crashes whenever it reaches a line like "ui->something...", even though it's just a simple public function in my colorselect class. All my other ColorSelect functions can do this. So it seems like I'm having some trouble with private pointers to objects in my ColorSelect class.

[sedi]

You write, that your

application crashes when write it like this

I'd think that's quite normal - I don't see anything actually creating the dialog object, you are just using a pointer (hint: if it uses-> it is a pointer) to a random place in your memory.
Before line 3 in your 1st example, create an object for that pointer:

dialog = new DeviceDialog (this);

dialog = new DeviceDialog (this);

To copy to clipboard, switch view to plain text mode 

But beware! If you allocate space that way, you MUST clean up and free the ressource afterwards, so after line 5 you should add

delete dialog;

delete dialog;

To copy to clipboard, switch view to plain text mode 

After that, never use the pointer again, until it is created anew.

In your second example, you create a dialog object (you dont use the pointer that is a member of your class). This "lives" only in the very scope it is created in, c++ deletes it automatically when you leave the method.

As for your third example: is it possible that this method is called earlier in time than the other methods (that are working)? If ui is not yet created when you use it, any use of it will crash your application.
HTH,
Sebastian

[Leutzig]

Thanks Sebastian, I'll be reading up on dynamic allocation. I'm quite certain that I don't call the method from third example before ui is created. It is called when I press the OK button in the DeviceDialog dialog which also closes the dialog. I'm not sure if that could cause the problem since you can't really interact with your main window when the dialog is open (at least not in my application).

void DeviceDialog::on_buttonBox_accepted()
{  
    colorSelect = new ColorSelect(this);
    //When OK is pressed, set item data with house code and unit code    
    colorSelect->setItemData(ui->houseLineEdit->text(), ui->unitLineEdit->text());
    delete colorSelect;
}

void DeviceDialog::on_buttonBox_accepted()
{  
    colorSelect = new ColorSelect(this);
    //When OK is pressed, set item data with house code and unit code    
    colorSelect->setItemData(ui->houseLineEdit->text(), ui->unitLineEdit->text());
    delete colorSelect;
}

To copy to clipboard, switch view to plain text mode 

[d_stranz]

void DeviceDialog::on_buttonBox_accepted()
{  
    colorSelect = new ColorSelect(this);
    //When OK is pressed, set item data with house code and unit code    
    colorSelect->setItemData(ui->houseLineEdit->text(), ui->unitLineEdit->text());
    delete colorSelect;
}

void DeviceDialog::on_buttonBox_accepted()
{  
    colorSelect = new ColorSelect(this);
    //When OK is pressed, set item data with house code and unit code    
    colorSelect->setItemData(ui->houseLineEdit->text(), ui->unitLineEdit->text());
    delete colorSelect;
}

To copy to clipboard, switch view to plain text mode 

No, no, no, you do not want this. This is equivalent to a NO-OP - you create the class instance on the stack, do something with it, and it immediately gets deleted along with whatever you did.

Unfortunately, you cannot create an instance of ColorSelect in the DeviceDialog constructor if you also create an instance of DeviceDialog in the ColorSelect constructor. That sets up an infinite recursion: ColorSelect creates DeviceDialog which creates ColorSelect which creates DeviceDialog ad infinitum until your stack blows up.

BUT I am not at all sure that this code does what you intend in your app. Even if you work around the recursion problem, you will still end up with independent instances of each class, instead of one instance of ColorSelect and one instance of DeviceDialog. Changes made to one instance of either class will be local to that instance only, so for example changing something in the list widget of one DeviceDialog won't have any effect on the list widget in other instances.

Maybe you can show some code on how you create and use these classes and we can give you some advice on how to structure the dependencies between them.

[Leutzig]

Yes I see what you mean. I'm not sure how to get around this recursion problem. I'll greatly appreciate if you could take a look at my ColorSelect class and DeviceDialog class and let me know how to structure these dependencies.

Here's a link to the project folder:
https://mega.nz/#!Rx4FkYyI

Decryption key for download:
!b3qx9qGK5hKq1QqTFapsDRE6bF1om3uoTk3luq6N7r8

It might be easier to follow if you get the whole cpp and header files instead of the snips and pieces of the code above.

[d_stranz]

Wow, no offense, but that's a way more confusing implementation that it needs to be. No wonder you are having trouble with circular dependencies.

I think most of your problem stems from the fact that there is no one "in charge" in your GUI. You have a tabbed dialog (a QWidget, actually - ColorSelect) that contains two tabs, neither of which knows about the other but which need to communicate information. And probably, because you couldn't figure out how to do it in Qt Designer, you create and show() your ColorWheel instance separately.

I am going to be presumptuous and refactor your code a bit to make it less interdependent.

First step is to make ColorWheel a proper and independent child of the ColorSelect widget, by "promoting" the QWidget you have created in Designer. To do this, you open the ui file in Designer, right-click on the place where you inserted the QWidget instead of the ColorWheel, and select "Promote to...". This will open the widget promotion dialog. In the Promoted class name box, type "ColorWheel" and it will automatically insert the header file name in the box below. Click "Add", then click Promote and Close. If you inspect the object list, you will now see that the object type for the color wheel is now ColorWheel instead of QWidget.

Next step is to get rid of the dependence of ColorWheel on ColorSelect. Remove the ColorSelect argument from the constructor, remove the member variable, and remove all references to it from the ColorWheel .cpp and .h files. Remove the include of colorselect.h. Replace the call to colorSelect->updatePalette() in the ColorWheel class with a call to emit colorChange(). Move the QPalette stuff out of main() and into the ColorWheel constructor:

ColorWheel::ColorWheel(QWidget *parent) :
    QWidget(parent),
    initSize(200,200),
    mouseDown(false),
    margin(0),
    wheelWidth(30),
    current(Qt::red),
    inWheel(false),
    inSquare(false)
{
    //    resize(initSize);
    current = current.toHsv();
    //    setMinimumSize(200,200);
    setCursor(Qt::CrossCursor);
 
	 QPalette wheelPalette = palette();
	 wheelPalette.setBrush(QPalette::Window, QBrush(Qt::white));
	 setPalette(wheelPalette);
 
}

ColorWheel::ColorWheel(QWidget *parent) :
    QWidget(parent),
    initSize(200,200),
    mouseDown(false),
    margin(0),
    wheelWidth(30),
    current(Qt::red),
    inWheel(false),
    inSquare(false)
{
    //    resize(initSize);
    current = current.toHsv();
    //    setMinimumSize(200,200);
    setCursor(Qt::CrossCursor);

	 QPalette wheelPalette = palette();
	 wheelPalette.setBrush(QPalette::Window, QBrush(Qt::white));
	 setPalette(wheelPalette);

}

To copy to clipboard, switch view to plain text mode 

Now, ColorWheel is completely independent of ColorSelect. You need to reconnect the notification about color changes in the wheel so that ColorSelect can follow them. In the ColorSelect constructor, add a connect statement:

connect( ui->colorWheelWidget, SIGNAL( colorChange( const QColor & ) ), this, SLOT( updatePalette( const QColor & ) ) );

connect( ui->colorWheelWidget, SIGNAL( colorChange( const QColor & ) ), this, SLOT( updatePalette( const QColor & ) ) );

To copy to clipboard, switch view to plain text mode 

and rewrite the updatePalette() method as a slot:

// .h:
 
private slots:
	 void updatePalette( const QColor & );
 
// .cpp:
 
void ColorSelect::updatePalette( const QColor & color )
{
    //get palette
    QPalette pal = ui->colorPalette->palette();
 
    //update palette with colorToRGB
    pal.setColor(QPalette::Window, color);
 
    ui->colorPalette->setPalette(pal);
    emit colorChanged(color);
    qDebug()<< color;
}

// .h:

private slots:
	 void updatePalette( const QColor & );

// .cpp:

void ColorSelect::updatePalette( const QColor & color )
{
    //get palette
    QPalette pal = ui->colorPalette->palette();

    //update palette with colorToRGB
    pal.setColor(QPalette::Window, color);

    ui->colorPalette->setPalette(pal);
    emit colorChanged(color);
    qDebug()<< color;
}

To copy to clipboard, switch view to plain text mode 

Next, now that you are building the ColorWheel entirely within ColorSelect, you can dramatically simplify main():

#include "colorselect.h"
#include "serialcom.h"
 
int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    SerialCom *serialCom = new SerialCom();
    ColorSelect *colorSelect = new ColorSelect(0,serialCom);
    colorSelect->setWindowTitle("RGB-LED Color Picker");
 
    //show UI
    colorSelect->show();
 
    return a.exec();
}

#include "colorselect.h"
#include "serialcom.h"

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    SerialCom *serialCom = new SerialCom();
    ColorSelect *colorSelect = new ColorSelect(0,serialCom);
    colorSelect->setWindowTitle("RGB-LED Color Picker");

    //show UI
    colorSelect->show();

    return a.exec();
}

To copy to clipboard, switch view to plain text mode 

Next step is to break the dependence of DeviceDialog on ColorSelect. DeviceDialog's entire purpose is to capture two pieces of text from the user. So, it has no business knowing that ColorSelect is going to add those things to a list widget somewhere. It just simply needs to announce to the world, "Hey, I have new text!" So, DeviceDialog becomes this:

// .h:
#ifndef DEVICEDIALOG_H
#define DEVICEDIALOG_H
 
#include <QDialog>
 
namespace Ui {
class DeviceDialog;
}
 
class DeviceDialog : public QDialog
{
    Q_OBJECT
 
public:
    explicit DeviceDialog(QWidget *parent = 0);
    void setLineEdits(QString);
    ~DeviceDialog();
 
signals:
	 void houseAndUnitChanged( const QString & house, const QString & unit );
 
private slots:
    void on_buttonBox_accepted();
 
private:
    Ui::DeviceDialog *ui;
};
 
#endif // DEVICEDIALOG_H

// .h:
#ifndef DEVICEDIALOG_H
#define DEVICEDIALOG_H

#include <QDialog>

namespace Ui {
class DeviceDialog;
}

class DeviceDialog : public QDialog
{
    Q_OBJECT

public:
    explicit DeviceDialog(QWidget *parent = 0);
    void setLineEdits(QString);
    ~DeviceDialog();

signals:
	 void houseAndUnitChanged( const QString & house, const QString & unit );
	 
private slots:
    void on_buttonBox_accepted();

private:
    Ui::DeviceDialog *ui;
};

#endif // DEVICEDIALOG_H

To copy to clipboard, switch view to plain text mode 

// .cpp
 
#include "devicedialog.h"
#include "ui_devicedialog.h"
 
DeviceDialog::DeviceDialog(QWidget *parent) :
    QDialog(parent),
    ui(new Ui::DeviceDialog)
{
    ui->setupUi(this);
}
 
DeviceDialog::~DeviceDialog()
{
    delete ui;
}
 
void DeviceDialog::setLineEdits(QString houseAndUnit)
{
    ui->houseLineEdit->setText(houseAndUnit.mid(0,1));
    ui->unitLineEdit->setText(houseAndUnit.mid(1,1));
}
 
void DeviceDialog::on_buttonBox_accepted()
{  
	emit houseAndUnitChanged( (ui->houseLineEdit->text(), ui->unitLineEdit->text() );
}

// .cpp

#include "devicedialog.h"
#include "ui_devicedialog.h"

DeviceDialog::DeviceDialog(QWidget *parent) :
    QDialog(parent),
    ui(new Ui::DeviceDialog)
{
    ui->setupUi(this);
}

DeviceDialog::~DeviceDialog()
{
    delete ui;
}

void DeviceDialog::setLineEdits(QString houseAndUnit)
{
    ui->houseLineEdit->setText(houseAndUnit.mid(0,1));
    ui->unitLineEdit->setText(houseAndUnit.mid(1,1));
}

void DeviceDialog::on_buttonBox_accepted()
{  
	emit houseAndUnitChanged( (ui->houseLineEdit->text(), ui->unitLineEdit->text() );
}

To copy to clipboard, switch view to plain text mode 

Finally, you have to fix up ColorSelect so it handles the DeviceDialog signal:

// .h
 
private slots:
	 void onHouseAndUnitChanged( const QString & house, const QString & unit );
 
// and delete the "DeviceDialog" member variable.
 
// .cpp
 
void ColorSelect::on_listWidget_itemDoubleClicked(QListWidgetItem *item)
{
	 DeviceDialog dialog(this);
 
	 connect( &dialog, SIGNAL( houseAndUnitChanged( const QString &, const QString & ) ), this, SLOT( onHouseAndUnitChanged( const QString &, const QString & ) ) );
	 dialog.setLineEdits(item->data(Qt::UserRole).toString());
	 dialog.exec();
}
 
void ColorSelect::onHouseAndUnitChanged( const QString & house, const QString & unit )
{
	setItemData( house, unit );
}

// .h

private slots:
	 void onHouseAndUnitChanged( const QString & house, const QString & unit );

// and delete the "DeviceDialog" member variable.

// .cpp

void ColorSelect::on_listWidget_itemDoubleClicked(QListWidgetItem *item)
{
	 DeviceDialog dialog(this);

	 connect( &dialog, SIGNAL( houseAndUnitChanged( const QString &, const QString & ) ), this, SLOT( onHouseAndUnitChanged( const QString &, const QString & ) ) );
	 dialog.setLineEdits(item->data(Qt::UserRole).toString());
	 dialog.exec();
}

void ColorSelect::onHouseAndUnitChanged( const QString & house, const QString & unit )
{
	setItemData( house, unit );
}

To copy to clipboard, switch view to plain text mode 

There's still more cleanup and refactoring you could do, but now you have something where there are no circular dependencies, and in fact very few dependencies at all.

I've attached a zip file with all the code changes.

colorselect_serial.zip

By the way, I am out of the office until the end of the week, so I won't be following the forum.

[Leutzig]

This is just... Absolutely great work, thank you so much, d_stranz. I obviously didn't know to use signals and slots, but I see why it makes so much more sense using them. Hopefully your eyes are alright after looking at that mess, as it is my first actual Qt application. But man... Thank you.

//Leutzig