Review Request 120355: [OS X] prevent a crash when opening konqueror's Help menu

[Thomas Lübking]

> On Sept. 24, 2014, 5:48 nachm., Thomas Lübking wrote:
> > I assume you'd be better off altering KMenu::addTitle() - or even patch Qt (QMenu on mach cannot deal w/ widget actions, at least if used on the global menubar)
> 
> René J.V. Bertin wrote:
>     I agree totally, but for that
>     
>     - I'd have to understand exactly what the addTitle does that makes Qt/Mac crash
>     - Ideally I'd also know how to determine if the menu is in the global menubar or e.g. in a popup menu, where addTitle works perfectly fine. I think we'd want to preserve that because popup menus follow the selected style and not necessarily the OS X style.
>     
>     There's also the point that the addTitle (and addSection, IIRC) in Qt5 don't crash. They have other issues (IIRC you get just a separator, not the title text) but until now I've preferred to handle these crashes on a case-by-case basis.
>     
>     I admit, this RR was also made a bit with the idea of getting a discussion going about this issue. ;)
> 
> Thomas Lübking wrote:
>     Since KMenu is deprecated and the ::addTitle() implementation doesn't differ in KF5, either the applications have simply been ported away from KMenu or QWidgetAction was fixed in Qt5.
>     
>     To know why exactly this crashes for you, i'd need to see a backtrace (paste.kde.org) - Qt4 claimed QWidgetAction support on OSX' global menu - with some caveats.
>     If QMenu::menuAction() is in the action list of the global menu - unfortunately, this menubar is parentless :-(
>     Also there's no guarantee that this assignment won't change at some point in the future to any direction.
>     
>     > IIRC you get just a separator, not the title text
>     
>     What basically means that the QWidget(Action) reparenting doesn't work at all in Qt5 anymore (at best the linked out widget is just hidden)
>     
>     
>     Disclaimer: I'm a bit biased here ;-)
>     Imo using a QWidgetAction as title was a wrong design itfp - I proposed a Qt4 patch to use a leading and entitled separator instead, but it was rejected because not all styles did/do support texted separators. No idea whether that patch was revived for Qt5, never tested. (And, tbh, I don't know whether the native styles, ie. Win and Mac, support texted separators)
> 
> René J.V. Bertin wrote:
>     backtrace: http://paste.kde.org/pvnu8pgui
>     
>     If I recall correctly, Qt5.3's QMenu::addTitle and QMenu::addSection indeed call for what I think you mean with texted separators. And OS X will only render the separator for those. OS X 10.6 in any case, but I don't see why that would have changed in later versions.
> 
> Thomas Lübking wrote:
>     Thanks.
>     QMenu::addTitle() does not exist in 5.3 and ::setTitle() refers to the menubar item text.
>     ::addSection() might work (if the building loop was reversed, making a separator as first element possible ;-)
>     
>     On the crash:
>     It occurs because QWidgetPrivate::setGeometry_sys_helper() in qwidget_mac.mm is not aware that the widget it operates on is a toplevel widget (and has no parent)
>     This seems to be the "QMacNativeWidget(0);" "container" created in qmenu_mac.mm, QMenuPrivate::QMacMenuPrivate::addAction()
>     
>     Why it doesn't figure so, I don't know, but assume that in
>     
>     ```cpp
>     bool QWidgetPrivate::isRealWindow() const
>     {
>         return q_func()->isWindow() && !topData()->embedded;
>     }
>     ```
>     
>     "topData()->embedded" will be true (so the return be false)
> 
> René J.V. Bertin wrote:
>     Hmm, it's been a while that I looked at that - when making kmail "not crash" because of the same reason on OS X. I never submitted a patch for that here because I noticed that kdepim git/master used new QMenu functions. I ported over QMenu::addSection to KMenu, and that's where I saw that a "texted separator" remains just a separator on OS X.
>     
>     Are you sure a menubar becomes the global menubar only when it doesn't have a parent? I seem to recall that the situation is a little bit more complex than that.
> 
> Thomas Lübking wrote:
>     > Are you sure a menubar becomes the global menubar only when it doesn't have a parent? I seem to recall that the situation is a little bit more complex than that.
>     
>     I can only quote the Qt docs on this:
>     
>     > If you want all windows in a Mac application to share one menu bar, you must create a menu bar that does not have a parent. Create a parent-less menu bar this way:
>     > QMenuBar *menuBar = new QMenuBar(0);
>     > Note: Do not call QMainWindow::menuBar() to create the shared menu bar, because that menu bar will have the QMainWindow as its parent. That menu bar would only be displayed for the parent QMainWindow.
>     
>     This has however nothing to do with the crash - it's the "QMacNativeWidget" which has no parent but is treated by ::setGeometry_sys_helper() as if it had.
>     The call to ::invalidateBuffer_resizeHelper() must only happen "if (q->parentWidget())" resp. "if (!q->isWindow())". Whether it's embedded somewhere doesn't matter.
> 
> René J.V. Bertin wrote:
>     It's late here too - you think there's another workaround?

I'd skip efforts to work-a-round and fix qwidget_mac.mm instead.
This bug threatens beyond adding titles to menus and even widget actions.

*Everytime* a parentless, but "embedded" (didn't look into the mac specific code) widget is resized, you'll get a crash.
The buffer invalidation does only make sense for parented widgets and will inevitably crash (there's even a Q_ASSERT for it) without.


- Thomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120355/#review67379
-----------------------------------------------------------


On Sept. 25, 2014, 2 nachm., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120355/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2014, 2 nachm.)
> 
> 
> Review request for KDE Base Apps, KDE Software on Mac OS X, kdelibs, and Qt KDE.
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> Mac OS X cannot handle the formatting used for title menu items when it applies to items in the toplevel menu bar. An application calling KMenu::addTitle on such a menu item will crash immediately, somewhere deep in Qt.
> 
> This patch works around that crash by emulating the addTitle effect.
> 
> Curiously, the addTitle call that causes the crash when clicking on the Help menu concerns a submenu of an item of the Tools menu...
> 
> 
> Diffs
> -----
> 
>   konq-plugins/uachanger/uachangerplugin.cpp 5e2d094 
> 
> Diff: https://git.reviewboard.kde.org/r/120355/diff/
> 
> 
> Testing
> -------
> 
> OS X 10.6.8 with kdelibs 4.14.1
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20140926/62be2659/attachment.htm>