Qt: Setup my own QWidget in the right way

Question

In Qt I'm trying to set up my own QWidget so everything should work good due to memory management and other things. But I can't seem to get it all right with pointers, heap and stack. I have my widget MyWidget that have a QList with some objects. I can't figure out how to set up everything right.

You can see my code below and I have some questions regarding this:

1. The instace variable list is created on the heap, would it be better to create it on the stack?

2. In my list I have pointers, would it be better to just create the object on the stack and append it to the list? (So that I don't have pointers in the list at all)

3. When I append the objects to the list will they get the list as their parent automatically? So when I delete the list all objects inside the list will be deleted?

4. The for each loop I'm trying to use isn't working, I got "a pointer/array type was expected for this operation instead of 'int'"

5. In my code I want to create other widgets that takes the object from the list as parameters. Is it the right way to do it like I have? MyOtherWidget's instance method looks like this: MyOtherWidget(MyObject *myObject, QWidget *parent)

Thanks for your help! I'm new to Qt and C++ so it would be great if you could guide me in the right direction. How can I setup this in the right way to make it easy, don't get memory leaks and use as little memory as needed. How would you setup the same thing?

This is my code:

MyWidget.h:

class MyWidget : public QWidget
{
Q_OBJECT

public:
    MyWidget(QWidget *parent = 0);
    ~MyWidget();

private:
    QList<MyObject*> *list;
};

MyWidget.cpp:

MyWidget::MyWidget(QWidget *parent)
{
    ui.setupUi(this);

    list = new QList<MyObject*>();
    for (int i = 0; i<10; i++) 
    {
        MyObject *myObject = new MyObject("Hello",this);
        list->append(myObject);
    }

    foreach(MyObject *myObject, list)
    {
        //Lets say I want to create other widgets here and that they takes a MyObject as a parameter
        MyOtherWidget *myOtherWidget = new MyOtherWidget(myObject,this);
    }

}

MyWidget::~MyWidget(){
    delete list;
}

Answer 1

Hey,

Yes, the problem is that you're deleting your list's object but not its elements !

I suggest you have a look at :

QList<Employee *> list;
list.append(new Employee("Blackpool", "Stephen"));
list.append(new Employee("Twist", "Oliver"));

qDeleteAll(list.begin(), list.end());
list.clear();

More info here

I would also ask if you really need a pointer to your list ? You could simply have a simple :

QList<MyObject*> list;

Therefore you have one less possible memory leak !

Hope this helps a bit !

Edit :
3. Your MyObject objects have "this" as parents. The list doesnt take the objects ownership when you deal with pointers.
4. For the loop, maybe you should consider Iterators, have a look here qthelp://com.trolltech.qt.460/qdoc/qlist.html.

Answer 2

Ad.1. The lifetime of the list should be the same as the lifetime of the MyWidget instance, so you can safely create the list on the stack.

Ad.2. You could do that, but MyObject class would need to have a default constructor, a copy constructor, and an assignment operator (see http://doc.trolltech.com/4.6/containers.html#container-classes for details).

Ad.3. The ownership of the object is not transferred on append. Just like STL containers, Qt containers doesn't call delete on stored pointers. To delete all pointers stored in QList (or other Qt container) you can use qDeleteAll(list). Mind, that you probably don't want to do so in the code you posted: you pass MyWidget pointer to MyObject constructor and I assume it is then used as QObject parent. So all QObjects will be deleted when MyWidget is deleted.

Ad.4. The second argument of foreach macro should be a container, not pointer to container. So you should call foreach(MyObject *obj, *list) if your list variable is a pointer to QList.

Ad.5. You should be fine as long as MyOtherWidget doesn't delete the passed MyObject (because the MyWidget is already a parent of MyObject and you'd end up deleting the same object twice).

It's a gross simplification, but you should try to write your code in such way that you won't need to call delete at all. Create stuff on the stack or rely on Qt parent-children mechanism (i.e. parents delete their children). Later on you might want to read about smart pointers (QSharedPointer, QScopedPointer, etc.).

EDIT:

Whether the parent of MyObject is set or not depends on what you're doing in MyObject constructor. If you pass parent argument to QObject constructor, i.e. your Myobject constructor looks like this:

MyObject(const QString &text, QObject *parent = 0) : QObject(parent)
{
// more code...
}

the parent will be set, because it will be done in QObject constructor, which will be called because of the ":QObject(parent)" code. What if you don't have this fragment? Since the MyObject inherits QObject, and you don't specify which constructor should be called the default QObject constructor, i.e. QObject(QObject *parent = 0) will be called, so the parent of your MyObject will be NULL and it won't be deleted.

I'd try to avoid setting parent explicitly through setParent method - for basic use cases setting parent in constructor should be enough.

Try to use correct terminology (not "instance method", but "constructor"), read Qt documentation, use common sense and try not to think that anything will be done automatically. The parent is not set "automatically" just because you call one argument "parent" - it is set, because there is a piece of code that does it in QObject constructor and it's your responsibility to call pass proper parent to QObject constructor in the classes that inherit QObject.

Answer 3

You don't need to store the child widgets within a list, as long as you make them parents of the current widget. (Usually you create your widgets on the stack with new).

Qt has a auto-cleanup function, which means, if a widget gets removed all child widgets (widgets whose parent is the widget that gets removed) get removed to.

So the only thing you will have to make sure (especially for temporary popup widgets) to erase/remove the Widget "popup" or however you call your popup widget.

That's it, that's all.