<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 15th, 2014, 2:19 p.m. CEST, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120149/diff/2/?file=312029#file312029line164" style="color: black; font-weight: bold; text-decoration: underline;">kdeui/actions/kaction.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">164</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">prefsText</span> <span class="o">=</span> <span class="n">i18n</span><span class="p">(</span> <span class="s">"Configure %1..."</span><span class="p">,</span> <span class="p">(</span><span class="n">aboutData</span><span class="p">)</span> <span class="o">?</span> <span class="n">aboutData</span><span class="o">-></span><span class="n">programName</span><span class="p">()</span> <span class="o">:</span> <span class="n">qApp</span><span class="o">-></span><span class="n">applicationName</span><span class="p">()</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<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;">would this eg. work with kwrite ("Configure Editor...")? - or other kpart driven things?</p></pre>
</blockquote>
<p>On September 15th, 2014, 4 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;">The question is, should it work? KDevelop for instance has a Configure Editor item in its Settings menu, added <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">before</em> "Configure KDevelop...", and in that case it's not the editor's configure dialog that should get linked to the Preferences menu item.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
If kwrite is nothing but a wrapper around an editor kpart (kate?) then I guess the end result will depend on how that kpart creates its configure action... If it uses KStandardActions and the wrapper (kwrite) doesn't (or only does after the kpart did), the Preferences menu ought to link to "Configure Editor...".</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In short, it's neigh impossible to cater for all possible cases. In a sense it wouldn't disturb me at all if KDE applications simply used their own menu organisation, but it's Qt that obliges us to take action because otherwise it's there that the organisation can get messed up.</p></pre>
</blockquote>
<p>On September 15th, 2014, 4:16 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;">This is where Qt initialises <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">menuRole</code> with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">TextHeuristicRole</code>, a value only used on Mac:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Qt 4.8.6 :</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%">QActionPrivate<span style="color: #666666">::</span>QActionPrivate() <span style="color: #666666">:</span> group(<span style="color: #666666">0</span>), enabled(<span style="color: #666666">1</span>), forceDisabled(<span style="color: #666666">0</span>),
visible(<span style="color: #666666">1</span>), forceInvisible(<span style="color: #666666">0</span>), checkable(<span style="color: #666666">0</span>), checked(<span style="color: #666666">0</span>), separator(<span style="color: #666666">0</span>), fontSet(<span style="color: #008000">false</span>),
forceEnabledInSoftkeys(<span style="color: #008000">false</span>), menuActionSoftkeys(<span style="color: #008000">false</span>),
iconVisibleInMenu(<span style="color: #666666">-1</span>),
menuRole(QAction<span style="color: #666666">::</span>TextHeuristicRole), softKeyRole(QAction<span style="color: #666666">::</span>NoSoftKey),
priority(QAction<span style="color: #666666">::</span>NormalPriority)
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Qt 5.3.1:</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%">QActionPrivate<span style="color: #666666">::</span>QActionPrivate() <span style="color: #666666">:</span> group(<span style="color: #666666">0</span>), enabled(<span style="color: #666666">1</span>), forceDisabled(<span style="color: #666666">0</span>),
visible(<span style="color: #666666">1</span>), forceInvisible(<span style="color: #666666">0</span>), checkable(<span style="color: #666666">0</span>), checked(<span style="color: #666666">0</span>), separator(<span style="color: #666666">0</span>), fontSet(<span style="color: #008000">false</span>),
iconVisibleInMenu(<span style="color: #666666">-1</span>),
menuRole(QAction<span style="color: #666666">::</span>TextHeuristicRole),
priority(QAction<span style="color: #666666">::</span>NormalPriority)
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Overriding that one way or another will make KDE menu(bars) behave exactly as they do on Linux, with only a few OS-specific items in the Application menu (Hide, Hide others, Show All). The added benefit would be for KDE applications like <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">konsole</code> that do not use the global menubar.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Does KDE have a particular/special status making it easier to propose changes to the Qt API?</p></pre>
</blockquote>
<p>On September 15th, 2014, 11:57 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;"><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;">Does KDE have a particular/special status making it easier to propose changes to the Qt API?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">dfaure, I assume ;-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No - not really. None that i'm aware of.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Would it help to alter KStandardAction::preferences() to explcitly set the PreferencesRole for this one (ie. would an explicit PreferencesRole trump TextHeuristicRole hitting for "configure shortcuts" or similar?</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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've dug into the Qt code (4.8.6 for now), and am now seeing how far just removing the guesstimating on the About and Preferences will take us. </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KStandardAction::preferences()</code> <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">already</em> sets PreferencesRole, that's one reason I moved up the setText call to before the block that sets the MenuRole (even if ATM that indeed doesn't change a thing).<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
And that works ... as long as <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KStandardAction::preferences()</code> is called before any other menu actions are added to a menu. It's the first action that gets added and that matches Qt's heuristics that ends up under the Preference menu item. I'm hoping this is limited to the items of menus belonging to the the global menubar, but I haven't traced the underlying code in full detail so it could even be that any menu action/item could end up there.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 15th, 2014, 2:19 p.m. CEST, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120149/diff/2/?file=312029#file312029line188" style="color: black; font-weight: bold; text-decoration: underline;">kdeui/actions/kaction.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">KAction::KAction(const KIcon &icon, const QString &text, QObject *parent)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">152</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">KAction</span><span class="o">::</span><span class="n">KAction</span><span class="p">(</span><span class="k">const</span> <span class="n">KIcon</span> <span class="o">&</span><span class="n">icon</span><span class="p">,</span> <span class="k">const</span> <span class="n">QString</span> <span class="o">&</span><span class="n">text</span><span class="p">,</span> <span class="n">QObject</span> <span class="o">*</span><span class="n">parent</span><span class="p">)</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">188</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">KAction</span><span class="o">::</span><span class="n">KAction</span><span class="p">(</span><span class="k">const</span> <span class="n">KIcon</span> <span class="o">&</span><span class="n">icon</span><span class="p">,</span> <span class="k">const</span> <span class="n">QString</span> <span class="o">&</span><span class="n">text</span><span class="p">,</span> <span class="n">QObject</span> <span class="o">*</span><span class="n">parent</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<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;">given KAction, KMenu and KMenuBar are all deprecated in KF5, is this actually of any upstream relevance?</p></pre>
</blockquote>
<p>On September 15th, 2014, 4 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 wouldn't know. It's been mentioned often enough that KF5 is still quite a while away from being release-ready on OS X, and in the meantime the best way to maintain momentum in getting it that far is to improve the user experience of what <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">is</em> available on the platform. Meaning KDE4 ...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As to upstream relevance ... it clearly is unfortunate that KAction, KMenu and KMenuBar have been deprecated, because Qt's baked-in heuristics haven't changed in Qt5. From what I see that makes it neigh impossible to address the issues I'm trying to address in KF5 .</p></pre>
</blockquote>
<p>On September 15th, 2014, 11:57 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;">Hint: KStandardAction is <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">not</em> deprecated so if setting the explicit role in KStandardAction::preferences() is sufficient, you won.</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">To rephrase what I was getting at in the issue above: <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KStandardAction::preferences()</code> could work, but only if no other menu action is created first that has a label that starts with "Config" ... and of course just about any slightly elaborate KDE app has several Configure actions in its Settings menu ... with the one that <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">should</em> go under Preferences last (and thus added last).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The more I think about it, the more I feel that Qt goes too far out of its way to bend applications to OS X conventions. Those apps that do want to adhere to those <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">guidelines</em> can always make sure that they create the Preference (and About) action(s) first on OS X. But a functioning UI that doesn't hide a few surprises seems more important overall.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll try to see what dfaure thinks of this - is he already getting updates from this RR through one of the reviewer groups?</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 15th, 2014, 2:19 p.m. CEST, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120149/diff/2/?file=312030#file312030line173" style="color: black; font-weight: bold; text-decoration: underline;">kdeui/actions/kstandardaction.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">KAction *create(StandardAction id, const QObject *recvr, const char *slot, QObject *parent)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">173</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// Set the text before setting the MenuRole, as on OS X setText will do some heuristic setting of itself,</span></pre></td>
</tr>
</tbody>
</table>
<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;">Will it?<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
I don't see such code in QAction, setText is not virtual and why setTextWithCorrectRole itfp then?</p></pre>
</blockquote>
<p>On September 15th, 2014, 4 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;">True.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Does C++ offer any mechanism by which one could overload a class's member function (or extend a class) without creating a child class? That might offer a solution on KF5.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If no, the setText call can go back to where it came from (personally I do find it more logical to set a role after having set the text, though).</p></pre>
</blockquote>
<p>On September 15th, 2014, 11:57 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;">you can simply override a protected or public function by just adding the exact same signature to the derived class, BUT:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KAction <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">ka = new KAction(this);<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
QAction </em>qa = ka;<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
ka->setText("foo"); // calls your reimplementation<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
qa->setText("foo"); // calls the QAction function, despite qa == ka, ie. is actually KAction</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">to have the above example work, the function needed to be virtual (and that means virtual in the base class, ie. QAction)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Q_SLOTS are usually not virtual, but QMetaObject::invokeMethod("slotFunction") will resolve the best matching ::slotFunction() for the object (though by runtime resolution)</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">(Q_SLOTS are usually static member functions, no?)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I know you can override a function through a derived class, and still invoke the overriden version, but that's irrelevant in a situation where there is no derived (child) class. As when KAction is removed, leaving KF5 apps with just QAction. That's why I specifically asked a about overriding a function without deriving the class...</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 15th, 2014, 2:19 p.m. CEST, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/120149/diff/2/?file=312033#file312033line174" style="color: black; font-weight: bold; text-decoration: underline;">kdeui/widgets/kmenu.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">174</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span> <span class="o">!</span><span class="n">action</span><span class="o">-></span><span class="n">isIconVisibleInMenu</span><span class="p">()</span> <span class="p">){</span></pre></td>
</tr>
</tbody>
</table>
<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;">this is <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">utterly</em> wrong - you're manipulating a QAction reference just because it (at this very time!) hints it won't show it's icon in menus?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The only sane approach is to fix the menu to honor this value and if that's not possible, clone the action (w/o icon), watch the original one for changed() and destroyed() for aligning updates and forward the clones signals to the original one.</p></pre>
</blockquote>
<p>On September 15th, 2014, 4 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 don't understand at all what you're trying to say.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Do you mean that the user could change <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">isIconVisibleInMenu()</code> while the app is running, and that the change should be apparent in the menu without relaunching the application, or recreating the menu?<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
If so, is that even possible on OS X? (I could check but I have no idea what kind of application might do such a thing...)<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
And if that's indeed what you mean, what do you think is worse - Qt ignoring <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">isIconVisibleInMenu()</code> altogether, or me not supporting potential changes to the flag and/or icon?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I do agree that my code changes the action object the user passes in. The thing is that it's probably not possible simply to clone the action and add the clone, because then everything the user does to his/her copy after adding it will have no effect ...</p></pre>
</blockquote>
<p>On September 15th, 2014, 7:29 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've played around a bit with Qt's systray example. Here are my findings:</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;">In Qt4, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">isIconVisibleInMenu</code> is ignored completely for system tray menus, be it when adding a QAction or when changing the property later.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Icon changes are respected.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">In Qt 5.3, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">isIconVisibleInMenu</code> is respected when adding a QAction to the system tray menu, but subsequent changes to it have no effect.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Icon changes are respected, unless <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">isIconVisibleinMenu</code> has been turned off, in which case the previously show icon remains visible.</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Based on these findings, I think someone could file an upstream bug report for Qt5, but for KDE/Qt4 it seems that we can stick with my proposal. With the current state of things, the user will have to restart the application if s/he wants to change icon visibility in the systray menu, whether my patch is applied or not (actually, without my patch visibility will never change...)</p></pre>
</blockquote>
<p>On September 15th, 2014, 11:57 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;">You are changing a clients code QAction from with a library, what is completely inacceptable.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Eg. a very common constellation is to use the very same QAction for a toolbutton and a menu entry. When it passed your code, puff, gone is the icon from the toolbutton.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It is not <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">simple</em> to clone the action, but it is possible. You'll have to track it's changes and forward them to your clone as you've to forward your clones triggering to the original action (ie. what Qt should be doing for this ominous systray menus - is it dbusmenu-qt?)</p></pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; 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;">OK, so I did understand you correctly.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If cloning the action is so complicated it is really not the way to go given how Qt 4.8 clearly <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">forgot</em> to check for the flag. I patched the code (1 additional check in 1 <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">if</code> expression), and all of a sudden even dynamic hiding/unhiding of the icon works. I'm pretty sure that Qt5 has another simple omission which explains why the icon isn't hidden when the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">iconVisibleInMenu</code> flag is switched off.</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On September 15th, 2014, 1:03 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 Software on Mac OS X, kdelibs, KDEPIM, Marco Martin, and Olivier Goffart.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Sept. 15, 2014, 1:03 p.m.</i></p>
<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;">This review is for 2 sets of changes; an initial one to the way "system tray" are rendered, and a newer set that protects the Preferences menu from getting linked to any action with an appropriate title.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">-- the system tray:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
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>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">-- the Preferences menu item<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
This is a menu item living in the Application menu, a menu that sits in the menubar between the Apple (?) menu and the File menu. This menu also contains the Quit command.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
KDE and Qt applications typically do not set up their menus in this fashion, so Qt provides an automatic way to put relevant menu items (actions) in the Application menu, using Apple's naming. The algorithm is described under QMenuBar in the Qt documentation: for the Preferences action, it will consider any action that has a text containing <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">config</code>, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">options</code>, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">settings</code> or <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">preferences</code>, and put it under the Preferences label if its menu role is set to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">heuristic</code> (which appears to be the default).<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
In practice, many applications provide a series of menu actions with names that trigger this method, and they do not always create their own preferences/settings/configuration menu first. Yet it is the first menu action that matches that will be installed under the Preferences menu, with the Command-, shortcut. A good example is KDevelop: it will have a Preferences menu that activates the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Configure Selection</code> action - which does not open a settings dialog but launches the configure or cmake procedure for the selected project ...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">My proposed solution overrides this Qt behaviour. On OS X, the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KAction(const QString &text, QObject *parent)</code> constructor calls a modified (static) function <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">setTextWithCorrectMenuRole</code> which checks the text against the patterns Qt will consider for <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">PreferencesRole</code>. If it finds a match, it will force the role to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">NoRole</code>, unless it is a perfect match with the standard KDE configuration action for the application (<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">"&Configuration appName..."</code>) in which case it sets the role to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">PreferencesRole</code>. This latter consideration allows kdelibs to "catch" the configuration menu for applications like KMail, which appear not to be created using KStandardActions.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
This approach can be extended to other menu actions that end up incorrectly in the OS X Application menu.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Applications that create menu actions using QAction or a different KAction constructor will see no change (and should use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">setMenuRole</code> selectively on OS X).</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>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The Preferences menu protection should carry over easily to KF5, supposing Qt5 uses the same heuristics to place relevant menu actions under the OS X application menu, and supposing <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KAction</code> has made the transition to KF5.</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/actions/kaction.cpp <span style="color: grey">(9e8f7fb)</span></li>
<li>kdeui/actions/kstandardaction.cpp <span style="color: grey">(7de0c6f)</span></li>
<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>