[Solved] Keeping a menubar for repeated use in QMainWindow::setMenuBar

Harry123

In my user interface, I would like to be able to switch the menubar of QMainWindow among several predefined menus.

Using QMainWindow::setMenuBar() on a menu can only work once with any QMenuBar* pointer, as the documentation for setMenuBar says this :
"Note: QMainWindow takes ownership of the menuBar pointer and deletes it at the appropriate time".

This seems to be the case, since when I tried, the second time that the same QMenuBar* pointer was used, setMenuBar crashed on invalid memory reference.

I have solved the problem by recreating the menubar before each call to setMenuBar.

However, is there a way of preserving the menubar, or of copying it without writing complex recursive code, so it could be more easily reset multiple times?

raven-worx

@Harry123
show your code please
Are you deleting the old menubar by yourself?

A more elegant way of recreating the menubar everytime again is to clear it's actions and add them as needed again.

Harry123

@raven-worx
There is nothing special about my code : Just doing setMenuBar with pointer1, followed by pointer2, then by pointer1 again, causes the crash.

I don't suppose that there is any way to prevent setMenuBar from deleting the menubar, short of modifying Qt code, so my question is probably about copying a menubar easily.

If there is no better solution, I will keep on recreating the menubar at each time. It just doesn't seem very elegant.

raven-worx

@Harry123 said:

There is nothing special about my code : Just doing setMenuBar with pointer1, followed by pointer2, then by pointer1 again, causes the crash.

nevermind, it seems i rushed too fast over your first post.

I don't suppose that there is any way to prevent setMenuBar from deleting the menubar, short of modifying Qt code, so my question is probably about copying a menubar easily.

no, since you already mentioned the ownership of the .

If there is no better solution, I will keep on recreating the menubar at each time. It just doesn't seem very elegant.

as i said it's probably way better to reuse the one menubar and just reinitialize it with the desired actions everytime. So avoid unnecessary memory allocations and deletions.

Harry123

@raven-worx

The menubar itself is just one class instance. Creating the actions requires allocating many more class instances, so just economizing on one allocate/delete, that of one QMenuBar, does not seem significant.

In any case, I'm not at all sure that clear() will not also crash. It seems to me that calling setMenuBar then clear may cause clear to crash on freeing memory that was already freed. Calling clear then setMenuBar may cause setMenuBar to crash.

kshegunov

@Harry123
Yes, unfortunately QMainWindow::setMenuBar will call QObject::deleteLater for the old menu. I don't think that this was a good design choice, but well, it's how it's implemented currently. You could try intercepting the QEvent::DeferredDelete events with an event filter and discard their processing for your menus. At least I see no other way, beside creating your menu bars anew.

Kind regards.

raven-worx

@Harry123 said:

The menubar itself is just one class instance. Creating the actions requires allocating many more class instances, so just economizing on one allocate/delete, that of one QMenuBar, does not seem significant.

i never said delete the actions. Unlike with the menubar the ownership of QActions isn't transferred to the widget when you call addAction().

So just create every instance once and compose them together as you need.

In any case, I'm not at all sure that clear() will not also crash. It seems to me that calling setMenuBar then clear may cause clear to crash on freeing memory that was already freed. Calling clear then setMenuBar may cause setMenuBar to crash.

what makes you think that?!

Chris Kawa

It would be nice if there was takeMenuBar(), but that's not a big problem. It's 2 lines of code.
Before you set new bar make sure to set the parent of the current one to null i.e.

if(mainWindow->menuBar())
   mainWindow->menuBar()->setParent(nullptr);
mainWindow->setMenuBar(anotherBar);

One thing to note is that with this you need to keep track of the lifetime of the bars because the unused ones are no longer managed by a parent. This is as easy as putting them in some container and calling qDeleteAll(barsContainer) at the end.

raven-worx

@Chris-Kawa
this is not a matter of QObject parent-child relationship.
The mainwindow keeps the menubar as a pointer and deletes it when a new one is set.

Chris Kawa

@raven-worx Setting a null parent "detaches" the bar from main window. It won't be deleted by the consecutive setMenuBar(). I'm not guessing. I've checked ;)

raven-worx

@Chris-Kawa
i see. Because it's contained in a layout.
Sry for that.

Harry123

@Chris-Kawa
That's a very simple solution to the problem.
Thank you, I'll be trying it out very soon.

kshegunov

@raven-worx, @chris-kawa
http://code.qt.io/cgit/qt/qtbase.git/tree/src/widgets/widgets/qmainwindow.cpp?h=5.5#n517
I'm sorry to argue, but look up here. Line 535 explicitly deletes the menu!

Chris Kawa

@kshegunov That's true, but note that deletion happens in if (topLayout->menuBar() .... block.
When you set parent of the bar to nullptr it is taken out of the layout. But don't take my word for it, check:

qDebug() << mainWindow->layout()->menuBar();  //prints out some pointer
mainWindow->menuBar()->setParent(nullptr);
qDebug() << mainWindow->layout()->menuBar();  //prints 0, the bar was taken out

kshegunov

@Chris-Kawa
Ah! Yes, you're correct of course. This was my initial idea (reparenting the menu bar) but I discarded it because I didn't realize that the menu will be taken out of the layout, silly me! :)

Kind regards.

Harry123

Solution verified as simple and perfectly working.

What I did was to override QMainWindow::setMenuBar as follows :

void myMainWindow::setMenuBar(QMenuBar * bar)
{
  if (menuBar())
    menuBar()->setParent(0);      // detach current menubar so it won't get deleted
  QMainWindow::setMenuBar(bar);
}

raven-worx

@Harry123
please be advised that this only works as long as the instance is of the type myMainWindow, since setMenuBar() is not virtual.

Meaning the following wont work, even the object behind the pointer has the correct type:

QMainWindow* mw = new myMainWindow;  // pointer-type is different from instance-type!
mw->setMenuBar(...);

Harry123

@raven-worx

I have "mw = this;" in the constructor for myMainWindow, where mw is a global variable, and I can now successfully do "mw->setMenuBar(..)" from several other classes with no problem.

It currently does work - why shouldn't it ?

Chris Kawa

@Harry123 It might work for your case but is very dangerous in general. As @raven-worx mentioned setMenuBar() is not virtual. This means that if anyone calls it through a pointer to base class the base class version will be called (which deletes the bar).
In general you can't guarantee that your class will only by accessed via pointer to derived class and in fact you should never assume that.
Example:

QMainWindow* gotcha = mw;
mw->setMenuBar(someBar); //ok
gotcha->setMenuBar(someOtherBar); //someBar is deleted!

Long story short - don't override non-virtual methods. Create a new one with descriptive name.

Harry123

@Chris-Kawa
I don't have this gotcha - my variable is of type myMainWindow*.
This can only not work if there is a bug in the C++ compiler.
But perhaps I will rename it, just in case.