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

Fri Sep 26 11:41:45 UTC 2014

> On Sept. 24, 2014, 7:48 p.m., 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?
> 
> Thomas Lübking wrote:
>     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.

Damn, I can foresee hours of qt4-compiling non-fun in my near future ...
I'll take the liberty to ping you per MP should I need some feedback/assistance. I hope it'll be possible to do this without an intimate understanding of the whole Mac-specific Qt implementation, ideally as simple as turning that Q_ASSERT into something that doesn't get compiled away and does something sensible in production/release builds.

(This is why I don't like ASSERTs, they tend to give a false sense of "security", esp. since it's not at all uncommon in my experience that they never get tripped in debug builds because they (apparently) do more variable initialisation leading to all kinds of nice Heisenbugs ;) )

- René J.V.

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

On Sept. 25, 2014, 4 p.m., 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, 4 p.m.)
> 
> 
> 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-mac/attachments/20140926/89f314c3/attachment.html>