Big memory problem

[Alundra]

Hi,
I recently saw the memory of my application grows and never decrease.
It's apparently because when I spawn a QMenu and QDialog I use new operator and never delete them.
I thought close a window deleted them but I was wrong, it's why the DeleteOnClose attribute is there.
But, I saw QMessageBox using static function delete after it's closed, so I guess it has the attribute.
Is QMessageBox the only class which set DeleteOnClose attribute and all other has to be set manually ?
Does QMenu contains the flag by default or it has to be set manually ?
Thanks

[Rondog]

You should not need to create new instances of QMenu every time you use it. For the main menu it is put into the constructor of the parent. If you use QAction in the menus you really don't need to deal with it too much (depends on your goal of course). If you have a context menu you can create it on the stack and not on the heap. The Qt documentation has some good examples: http://doc.qt.io/qt-5/qtwidgets-mainwindows-menus-example.html

For dialogs you can use a combination of methods. They can be on the stack if appropriate (popup message-box type of dialog) or you can create an instance of a dialog and attach it to the parent class (use 'new' in the constructor). If you are constantly creating a new instanace of a dialog then you need to find a way to delete it or you will exhaust memory sooner or later. I have not seen a need to do this yet.

My personal preference for dialogs is to create them on the heap (using operator new) but only on first use. The parent widget will be contructed faster and the resources required by the dialog will only be used if the dialog is actually created. You only create one instance of the dialog so some sort of 'Reset' function is required to ensure it is at a state suitable for use (does not display previous data). This applies to both modal and modeless dialogs.




// Constructor
d_my_dialog = 0; // pointer to instance of my dialog

// inside some function that uses dialog
if(!d_my_dialog)
{
d_my_dialog = new TMyDialog(this); // this open happens on first use
}

d_my_dialog->Reset(); // reset the dialog to a state that is suitable (repeated use)

if(QDialog::Accepted == d_my_dialog->exec())
{
// do something with dialog data or whatever
}

[anda_skoa]

But, I saw QMessageBox using static function delete after it's closed, so I guess it has the attribute.

Some of the static functions set the attribute, some just use a stack allocated messagebox instance.



Is QMessageBox the only class which set DeleteOnClose attribute and all other has to be set manually ?

No widget sets that attribute by default, QMessageBox certainly doesn't.

Cheers,
_

[Alundra]

Maybe have a pool of dialog/widget can avoid some allocation and speed up things, can be a good change.
Is it better to use that attribute or connect the close to deleteLater() ?
Does it end to the same ?

[Rondog]

Do you have any sample code (pseudo code) for what you are doing? Something that shows a reason to create a 'new' dialog each time it is used?

When you create a dialog and provide it a parent widget in the constructor it will be deleted when the parent is deleted. Options like 'Delete On Close' work if you must go this route. You can test this by simply monitoring the constructor/destructor calls and verify the memory is released.

I have never had a reason to do this nor can I picture something that requires this approach.

Note: Using a pool of dialog/widget is the same idea of creating one instance and reusing it as needed. I assume an instance will be requested, used, then returned (which may or may not necessarily result in a new instance being created the next time an instance is requested).

[Alundra]

For example, I have a color picker which is a dialog to control color of one property, I instance using new and I exec.
I also have a custom file browser which are dialog, it's used to show only one folder.
I have multiple QMainWindow tabbed in one window which can be closed too.
Surely the color picker can be reused to avoid to instance a new each time.
Each sub window has to be deleted when closed.

[d_stranz]

Surely the color picker can be reused to avoid to instance a new each time.

The cost to construct a simple dialog is almost zero. The user will spend far more time using the dialog than it takes to create it. I think you misunderstand the point here - most of the time you should not use "new" to create a dialog on the heap, you should simply create it on the stack:



void MyMainWindow::onPickColor()
{
MyColorPickerDlg dlg( this );
dlg.setLastChosenColor( mLastColor ); // for example
if ( QDialog::Accepted == dlg.exec() )
{
mLastColor = dlg.chosenColor();
}
}


The dialog instance is created on the stack and will automatically be destroyed when the onPickColor() method ends.

You could do the same thing with QMenu, but as another poster said, it is easier to create them during construction as members of your window class, and use the member pointers when you need them. They are created with "new" only once (during construction) and are destroyed when the program exits.

[Alundra]

You right about the stack, lifetime of the object is solved, no attribute needed for dialogs.
For QMenu, I checked and I was wrong all QMenu is created on the stack, only QMenu* found are from menubar.
All other QMenu is created on the stack without parent, I don't know if it's an error to not set a parent there, the result is correct visually.
I saw here "this" is used as parent of QMenu in context menu function : http://doc.qt.io/qt-5/qtwidgets-mainwindows-menus-example.html
Maybe it's an error to not set a parent and cause again memory leaks ?
But for sub-window, the attribute is surely needed to delete on close.

[d_stranz]

But for sub-window, the attribute is surely needed to delete on close.

You mean a child window, correct? A child window will be deleted when its parent is deleted; there is no need to set the delete on close flag if you create a child window on the heap with a non-NULL parent.

It is usually better to use hide() and show() instead of creating, deleting, and then re-creating windows. Make the child window in the parent window constructor and save the pointer. When you want to make the window visible, call pointer->show(). To make it invisible, call pointer->hide(). If you need to reset the child window to the same state as it was when it was first constructed, move that initialization code from the constructor into the showEvent().

[anda_skoa]

Maybe it's an error to not set a parent and cause again memory leaks ?

An object on the stack is deleted by code generated by the C++ compiler.
And the compiler will generate that code no matter which type that is or what the programmer passed to the type's constructor.



But for sub-window, the attribute is surely needed to delete on close.
If it has been allocated on the heap and you want to do delete on close, then yes. This is the whole purpose of this flag.

Cheers,
_

[Alundra]

An object on the stack is deleted by code generated by the C++ compiler.
And the compiler will generate that code no matter which type that is or what the programmer passed to the type's constructor.

I thought things inside the QMenu was created and since no parent was set it could cause memory leaks, good thing it doesn't happen on this case.


It is usually better to use hide() and show() instead of creating, deleting, and then re-creating windows. Make the child window in the parent window constructor and save the pointer. When you want to make the window visible, call pointer->show(). To make it invisible, call pointer->hide(). If you need to reset the child window to the same state as it was when it was first constructed, move that initialization code from the constructor into the showEvent().

I understand that point, but on my case it should not stay created because I open a sub-window each time I click on one file in a file browser and then it's tabbed on the main editor, which gives lot of memory used if never deleted when not used, surely the flag is needed on this case.

[d_stranz]

I understand that point, but on my case it should not stay created because I open a sub-window each time I click on one file in a file browser and then it's tabbed on the main editor, which gives lot of memory used if never deleted when not used, surely the flag is needed on this case.

Then you can either use the delete on close flag, or you can call deleteLater() using the sub-window pointer during the closeEvent(). Both of these methods have the same effect. Just be sure you do not try to use the sub-window pointer after that.

[anda_skoa]

I thought things inside the QMenu was created and since no parent was set it could cause memory leaks, good thing it doesn't happen on this case.

What do you mean with "inside the QMenu".
Anything that QMenu does internally is the concern of QMenu.
But you mean actions added to the QMenu, then you need to delete those, a menu does not own the actions.

Cheers,
_

[Alundra]

I add action using addAction of the QMenu class which takes a string as input, is it deleted correctly on this case ?
For menubar, I create QAction on the heap and use "this" as parent then I add this action in the menu created from menubar.

[anda_skoa]

I add action using addAction of the QMenu class which takes a string as input, is it deleted correctly on this case ?

http://doc.qt.io/qt-5/qmenu.html#addAction



For menubar, I create QAction on the heap and use "this" as parent then I add this action in the menu created from menubar.
QObject with a parent.

Cheers,
_

[Alundra]

Thanks for the link, so, for menu all sound good.
Dialogs are now all created on the stack, so, it's solved as well.