<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/120355/">https://git.reviewboard.kde.org/r/120355/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 24th, 2014, 7:48 p.m. CEST, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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)</p></pre>
</blockquote>
<p>On September 24th, 2014, 8:12 p.m. CEST, <b>René J.V. Bertin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I agree totally, but for that</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">I'd have to understand exactly what the addTitle does that makes Qt/Mac crash</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">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.</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I admit, this RR was also made a bit with the idea of getting a discussion going about this issue. ;)</p></pre>
</blockquote>
<p>On September 24th, 2014, 9:19 p.m. CEST, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
If QMenu::menuAction() is in the action list of the global menu - unfortunately, this menubar is parentless :-(<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Also there's no guarantee that this assignment won't change at some point in the future to any direction.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">IIRC you get just a separator, not the title text</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Disclaimer: I'm a bit biased here ;-)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
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)</p></pre>
</blockquote>
<p>On September 24th, 2014, 11:34 p.m. CEST, <b>René J.V. Bertin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">backtrace: http://paste.kde.org/pvnu8pgui</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</blockquote>
<p>On September 25th, 2014, 10:48 p.m. CEST, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
QMenu::addTitle() does not exist in 5.3 and ::setTitle() refers to the menubar item text.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
::addSection() might work (if the building loop was reversed, making a separator as first element possible ;-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On the crash:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
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)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
This seems to be the "QMacNativeWidget(0);" "container" created in qmenu_mac.mm, QMenuPrivate::QMacMenuPrivate::addAction()</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Why it doesn't figure so, I don't know, but assume that in</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #B00040">bool</span> QWidgetPrivate<span style="color: #666666">::</span>isRealWindow() <span style="color: #008000; font-weight: bold">const</span>
{
<span style="color: #008000; font-weight: bold">return</span> q_func()<span style="color: #666666">-></span>isWindow() <span style="color: #666666">&&</span> <span style="color: #666666">!</span>topData()<span style="color: #666666">-></span>embedded;
}
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"topData()->embedded" will be true (so the return be false)</p></pre>
</blockquote>
<p>On September 26th, 2014, 12:07 a.m. CEST, <b>René J.V. Bertin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</blockquote>
<p>On September 26th, 2014, 12:18 a.m. CEST, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I can only quote the Qt docs on this:</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
QMenuBar *menuBar = new QMenuBar(0);<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
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.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
The call to ::invalidateBuffer_resizeHelper() must only happen "if (q->parentWidget())" resp. "if (!q->isWindow())". Whether it's embedded somewhere doesn't matter.</p></pre>
</blockquote>
<p>On September 26th, 2014, 12:25 a.m. CEST, <b>René J.V. Bertin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It's late here too - you think there's another workaround?</p></pre>
</blockquote>
<p>On September 26th, 2014, 1:14 p.m. CEST, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'd skip efforts to work-a-round and fix qwidget_mac.mm instead.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
This bug threatens beyond adding titles to menus and even widget actions.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Everytime</em> a parentless, but "embedded" (didn't look into the mac specific code) widget is resized, you'll get a crash.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
The buffer invalidation does only make sense for parented widgets and will inevitably crash (there's even a Q_ASSERT for it) without.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Damn, I can foresee hours of qt4-compiling non-fun in my near future ...<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(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 ;) )</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On September 25th, 2014, 4 p.m. CEST, René J.V. Bertin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Base Apps, KDE Software on Mac OS X, kdelibs, and Qt KDE.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Sept. 25, 2014, 4 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kde-baseapps
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch works around that crash by emulating the addTitle effect.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Curiously, the addTitle call that causes the crash when clicking on the Help menu concerns a submenu of an item of the Tools menu...</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">OS X 10.6.8 with kdelibs 4.14.1</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>konq-plugins/uachanger/uachangerplugin.cpp <span style="color: grey">(5e2d094)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120355/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>