Does QMenuBar::clear() release memory of the action created with ?

thelfer

Hi,

I update the menu bar of a QMainWindow periodically (let say every 2s). To do this, I call the clear method before populating the menu bar again. Although working, this lead to memory leaks.

Here is a code snipset showing the trouble:

    this->menuBar()->clear();
    this->bm = this->menuBar()->addMenu(QObject::tr("Menu"));
   auto *const qmm = this->bm->addMenu(QObject::tr("Items"));
    for (const auto &mn : items) {
      auto *const qmma = qmm->addAction(mn.name);
      qmma->setIcon(mn.icon);
      // associate an action with qmma
    }

Would somebody tell me what I am doing wrong or if there is a proper way to delete all the items associated with my menu bar.

Regards,

Thomas Helfer

SGaist

Hi,

Not knowing what else your application is doing can't ensure it's the menu bar that's the culprit.

In any case, it's a bit strange to completely clear the menu bar so often especially if it's only to update one submenu. You should rather only manipulate what's really needed.

thelfer

@SGaist Hi,

You are perfecly right on the fact that I shall only remove what really needs to be updated.

However, the main point is that there is a memory leak to I need to solve.

If I remove the update of the QMenuBar, no more memory leak. If I remove the call to the setIcon method, the memory leak is much lower, so I do suspect that the memory leak is related to the fact that the clear method does not do what I believed.

BTW, I use Qt 5.7.1 under debian jessie.

Thanks for you answer.

SGaist

Can you check whether bm is still valid after the call to clear and before overwriting its content ?

thelfer

@SGaist How would I ? this->bm is a pointer, there is no way that the clear method would change it, so could I check if it's still valid ?

SGaist

Use a QPointer

thelfer

@SGaist Thanks for the tip ! I totally forgot about QPointer. I'll do this tomorrow.

thelfer

Well, using QPointer shows that this->bm is not released by QMenuBar::clear(). Shall we consider

that this is normal and one shall release each items (QMenu and QActions) by hand (though I did not think about how to do it in pratice)
this is a bug in QMenuBar
What do you think ?
Regards

Christian Ehrlicher

QMenu *QMenuBar::addMenu(const QString &title)

Appends a new QMenu with title to the menu bar. The menu bar takes ownership of the menu. Returns the new menu.

So when destroying the menu, the submenus will be gone. There is no constraint that those menus are destroyed on clear() esp. since the the documenation does not state anything releated to this:

Removes all the actions from the menu bar.

thelfer

@Christian-Ehrlicher Since the QMenuBar takes the ownership of the sub-menus, I do not feel that deleting them by hand is legal.

So, if clear is not meant to release the memory, how to do it ?

Even the documentation of the removeAction method does not state that the action is deleted. And there is no removeMenu method that would work on submenus.

Any advice is welcomed.

mrjj

@thelfer
Hi
Since Actions can be shared between toolbar and menubar, its reasonable that clear() does not
free them.
Its a bit unusual use case but you could do like.


void enumerateMenu(QMenu* menu) {
  foreach (QAction* action, menu->actions()) {
    if (action->isSeparator()) {
      qDebug("this action is a separator");
    } else if (action->menu()) {
      qDebug("action: %s", qUtf8Printable(action->text()));
      qDebug(">>> this action is associated with a submenu, iterating it recursively...");
      enumerateMenu(action->menu());
      delete action->menu(); // KILL IT
      qDebug("<<< finished iterating the submenu");
    } else {
      qDebug("action: %s", qUtf8Printable(action->text()));
      delete action; // KILL IT
    }
  }
}

void MainWindow::on_btRemove_released() {
  enumerateMenu(bm);
  delete bm; // bm is member
}

Seems to clean up as demostrated in this sample where i subclass Aaction to write to qDebug in dtor.
https://www.dropbox.com/s/jwlpvpva2mx6cy9/FreeMe.zip?dl=0

I also instrumented QIcon but is assigned by copy so that was not informative.

enumerateMenu code credits to
https://stackoverflow.com/questions/9399840/how-to-iterate-through-a-menus-actions-in-qt

Christian Ehrlicher

@thelfer said in Does QMenuBar::clear() release memory of the action created with ?:

So, if clear is not meant to release the memory, how to do it ?

It's clearly documented:

The menu bar takes ownership of the menu.

--> When the QMenuBar is destroyed, the QActions will be deleted too.

mrjj

Hi
Fooling around with it, maybe just total nuke works best

void MainWindow::on_btDelete_released() {
delete ui->menuBar;
this->setMenuBar( new QMenuBar(this) );
}

it dont even flashes and clearly delete all.

thelfer

Well, I tested @mrjj approach, and it led to a segfault right from the start. I'll investigate.

mrjj

@thelfer
Oh.
enumerateMenu crashed ?
I tested it with several subitems.

Do you have multiple "items" ?

alt text

Did sample crash for you? or was in first when used in real code?

thelfer

Well, I found the reason of the segfault: I hold one action as a member. This action was deleted by enumateMenu and the code crashes when this action was added again, which is perfectly normal. I think that I have to refactor my code.
Thanks to everyone.

mrjj

@thelfer
Ok. good found.
I assume its a no go to wipe whole menubar and recreate ?
You are keeping some when u "refresh" ?
delete ui->menuBar;
this->setMenuBar( new QMenuBar(this) );