<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/120149/">https://git.reviewboard.kde.org/r/120149/</a>
</td>
</tr>
</table>
<br />
<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 Software on Mac OS X, Marco Martin and Olivier Goffart.</div>
<div>By René J.V. Bertin.</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</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;">Until now, "system tray" menus had some rendering issues on Mac OS X:</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;">The menu title, the 1st menu item that on Linux shows the application name, remained empty</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Menu items that can (in principle, potentially) show an icon always showed the icon</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Point 1 was resolved by emulating the Linux addTitle/setTitle action in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KStatusNotifierItemPrivate::init()</code> : the menu title is implemented as a deactive standard menuitem followed by a separator. This makes the item stand out on a GUI that doesn't support the kind of formatting in menus as used in the Linux implementation.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Point 2 was identified as a Qt issue: <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">isIconVisibleInMenu</code> is ignored for systray menus. It was resolved by adding <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KMenu::addAction</code> methods that overload the ones from QMenu that were hitherto inherited unchanged by KMenu. The only different method is <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">addAction(QAction*)</code> which removes the icon from the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QAction</code> if <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">isIconVisibleInMenu()</code> is false. The other <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">addAction</code> methods are "overloaded with themselves" with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">using QMenu::addAction;</code> in the header file.</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;">Testing was done with kdelibs git/master and KDE/MacPorts on OS X 10.6.8 . The modified code is in compile-time conditional blocks used only on OS X, so no regressions are to be expected on other platforms.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KF5 is not production ready on OS X, so I am not currently able to port these modifications beyond KDE4. However, I did see that Qt5 has a new approach to adding titles to menus, which can be described as a "labelled separator". Backporting that function from the Qt5 source to kdelibs gave menu items that had the separator but not the text (title) label. It is thus likely that some kind of emulation will also be required with KF5, on OS X.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I considered doing the addTitle/setTitle emulation in kmenu.cpp, but decided against that for now. Menu titles are rendered as under Linux in menus that are not attached to the OS X toplevel menubar (say in context menus). Without knowing how to distinguish the kind of menu in KMenu methods the emulation will have to remain in the client code.</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>kdeui/notifications/kstatusnotifieritem.cpp <span style="color: grey">(1b15d40)</span></li>
<li>kdeui/widgets/kmenu.h <span style="color: grey">(f96e263)</span></li>
<li>kdeui/widgets/kmenu.cpp <span style="color: grey">(7dab149)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120149/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>