<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/126198/">https://git.reviewboard.kde.org/r/126198/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 4th, 2015, 8:26 a.m. CET, <b>Martin Gräßlin</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/126198/diff/6/?file=420926#file420926line67" style="color: black; font-weight: bold; text-decoration: underline;">src/platformtheme/CMakeLists.txt</a>
<span style="font-weight: normal;">
(Diff revision 6)
</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">67</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">elseif</span><span class="p">(</span><span class="s">APPLE</span><span class="p">)</span></pre></td>
</tr>
<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">68</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">target_link_libraries</span><span class="p">(</span><span class="s">KDEPlatformTheme</span> <span class="s">PRIVATE</span> <span class="s2">"-framework AppKit"</span><span class="p">)</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">56</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">endif</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">69</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">endif</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;">don't make thie an elsif, just an if. It's not an elseif condition</p></pre>
</blockquote>
<p>On December 4th, 2015, 2:02 p.m. CET, <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;">In practice it is, I think it has more or less been decided to assume <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">APPLE</code> means "no X11". Qt's XCB QPA can currently still be built on OS X (and is almost completely functional) but there's no guarantee how long that will still be possible.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Which should suit some of you, as full-blown X11 support would definitely give sense to supporting full-blown Plasma sessions (in so-called rooted mode of the XQuartz server). :P</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;">that's not the point I wanted to make. The changes are independent. We search for X11 independent of whether it's apple, so also on linking it should not be an if/else, but two if. Each of the features (X11 support, Apple support) should get it's own block.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It makes things much easier for if it needs to get extended in future. E.g. you could build on Linux without X11 support, then it should not go into any else branch which doesn't matter. Easier to read code and easier to maintain code was the idea I had in mind.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 4th, 2015, 8:26 a.m. CET, <b>Martin Gräßlin</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/126198/diff/6/?file=420929#file420929line41" style="color: black; font-weight: bold; text-decoration: underline;">src/platformtheme/kdeplatformtheme.h</a>
<span style="font-weight: normal;">
(Diff revision 6)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">40</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QVariant</span> <span class="n">themeHint</span><span class="p">(</span><span class="n">ThemeHint</span> <span class="n">hint</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">40</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="hl"> </span><span class="n"><span class="hl">virtual</span></span> <span class="n">QVariant</span> <span class="n">themeHint</span><span class="p">(</span><span class="n">ThemeHint</span> <span class="n">hint</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">41</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="n">QPalette</span> <span class="o">*</span><span class="n">palette</span><span class="p">(</span><span class="n">Palette</span> <span class="n">type</span> <span class="o">=</span> <span class="n">SystemPalette</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">41</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="hl"> </span><span class="n"><span class="hl">virtual</span></span> <span class="k">const</span> <span class="n">QPalette</span> <span class="o">*</span><span class="n">palette</span><span class="p">(</span><span class="n">Palette</span> <span class="n">type</span> <span class="o">=</span> <span class="n">SystemPalette</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">42</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">const</span> <span class="n">QFont</span> <span class="o">*</span><span class="n">font</span><span class="p">(</span><span class="n">Font</span> <span class="n">type</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">42</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="hl"> </span><span class="n"><span class="hl">virtual</span></span> <span class="k">const</span> <span class="n">QFont</span> <span class="o">*</span><span class="n">font</span><span class="p">(</span><span class="n">Font</span> <span class="n">type</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">43</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QIconEngine</span> <span class="o">*</span><span class="n">createIconEngine</span><span class="p">(</span><span class="k">const</span> <span class="n">QString</span> <span class="o">&</span><span class="n">iconName</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">43</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="hl"> </span><span class="n"><span class="hl">virtual</span></span> <span class="n">QIconEngine</span> <span class="o">*</span><span class="n">createIconEngine</span><span class="p">(</span><span class="k">const</span> <span class="n">QString</span> <span class="o">&</span><span class="n">iconName</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">44</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QList</span><span class="o"><</span><span class="n">QKeySequence</span><span class="o">></span> <span class="n">keyBindings</span><span class="p">(</span><span class="n">QKeySequence</span><span class="o">::</span><span class="n">StandardKey</span> <span class="n">key</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">44</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="hl"> </span><span class="n"><span class="hl">virtual</span></span> <span class="n">QList</span><span class="o"><</span><span class="n">QKeySequence</span><span class="o">></span> <span class="n">keyBindings</span><span class="p">(</span><span class="n">QKeySequence</span><span class="o">::</span><span class="n">StandardKey</span> <span class="n">key</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">45</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">45</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">46</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QPlatformDialogHelper</span> <span class="o">*</span><span class="n">createPlatformDialogHelper</span><span class="p">(</span><span class="n">DialogType</span> <span class="n">type</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">46</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="hl"> </span><span class="n"><span class="hl">virtual</span></span> <span class="n">QPlatformDialogHelper</span> <span class="o">*</span><span class="n">createPlatformDialogHelper</span><span class="p">(</span><span class="n">DialogType</span> <span class="n">type</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">47</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">bool</span> <span class="n">usePlatformNativeDialog</span><span class="p">(</span><span class="n">DialogType</span> <span class="n">type</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">47</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="hl"> </span><span class="n"><span class="hl">virtual</span></span> <span class="kt">bool</span> <span class="n">usePlatformNativeDialog</span><span class="p">(</span><span class="n">DialogType</span> <span class="n">type</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">48</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">48</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">49</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#if QT_VERSION >= QT_VERSION_CHECK(5, 3, 0)</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">49</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#if QT_VERSION >= QT_VERSION_CHECK(5, 3, 0)</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">50</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">standardButtonText</span><span class="p">(</span><span class="kt">int</span> <span class="n">button</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">50</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="hl"> </span><span class="n"><span class="hl">virtual</span></span> <span class="n">QString</span> <span class="n">standardButtonText</span><span class="p">(</span><span class="kt">int</span> <span class="n">button</span><span class="p">)</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">51</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#endif</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">51</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cp">#endif</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">52</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">52</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">53</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QPlatformSystemTrayIcon</span> <span class="o">*</span><span class="n">createPlatformSystemTrayIcon</span><span class="p">()</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</span><span class="p">;</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">53</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="hl"> </span><span class="n"><span class="hl">virtual</span></span> <span class="n">QPlatformSystemTrayIcon</span> <span class="o">*</span><span class="n">createPlatformSystemTrayIcon</span><span class="p">()</span> <span class="k">const</span> <span class="n">Q_DECL_OVERRIDE</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;">why did you add the virtual? The methods are marked as Q_DECL_OVERRIDE which implies they are virtual</p></pre>
</blockquote>
<p>On December 4th, 2015, 2:02 p.m. CET, <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;">Q_DECL_OVERRIDE indeed appears to resolve to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">override</code> even with clang; what about when it doesn't? Not supposed to happen and thus not to worry about any issues that might cause?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For now I've left the virtual keywords on definitions that lack Q_DECL_OVERRIDE; should I use Q_DECL_OVERRIDE instead?</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;"><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;">what about when it doesn't?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It doesn't matter. override triggers a compile error if the base method is not defined as virtual. By reimplementing in a subclass without virtual you're not going to turn it non-virtual. We only used to add virtual in derived classed to increase readability, to indicate that it overrides a base method. Now we have language support for that.</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;">For now I've left the virtual keywords on definitions that lack Q_DECL_OVERRIDE; should I use Q_DECL_OVERRIDE instead?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Don't change if it's not needed. If the method is virtual in the base class then don't add virtual.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 4th, 2015, 8:26 a.m. CET, <b>Martin Gräßlin</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/126198/diff/6/?file=420929#file420929line56" style="color: black; font-weight: bold; text-decoration: underline;">src/platformtheme/kdeplatformtheme.h</a>
<span style="font-weight: normal;">
(Diff revision 6)
</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; ">private:</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">protected:</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">55</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nl">private</span><span class="p">:</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">55</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nl">protected</span><span class="p">:</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">56</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="n">loadSettings</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">56</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="n">loadSettings</span><span class="p">();</span></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">57</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KFontSettingsData</span><span class="o">::</span><span class="n">FontTypes</span> <span class="n">fontType</span><span class="p">(</span><span class="n">Font</span> <span class="n">type</span><span class="p">)</span> <span class="k">const</span><span class="p">;</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">57</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">58</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">58</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KHintsSettings</span> <span class="o">*</span><span class="n">m_hints</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">59</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KHintsSettings</span> <span class="o">*</span><span class="n">m_hints</span><span class="p">;</span></pre></td>
</tr>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">59</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KFontSettingsData</span> <span class="o">*</span><span class="n">m_fontsData</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">60</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KFontSettingsData</span> <span class="o">*</span><span class="n">m_fontsData</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;">there is no such thing as a protected variable (see e.g. https://programmers.stackexchange.com/questions/162643/why-is-clean-code-suggesting-avoiding-protected-variables ). If you need to access the variables, please add protected accessor and setter methods</p></pre>
</blockquote>
<p>On December 4th, 2015, 2:02 p.m. CET, <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;">That article suggests protected member variables do exist but should be avoided. Is that what you meant?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The only one I actually used was mKdeGlobals from the fontsettings class</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;">yes they should be avoided up to the degree that one should consider that this is a feature the language doesn't expose. That's what I meant with how I formulated it.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On December 4th, 2015, 2:05 p.m. CET, 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, KDE Frameworks and Valorie Zimmerman.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Dec. 4, 2015, 2:05 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
frameworkintegration
</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;">The KdePlatformTheme plugin can be enabled on OS X with a minimal patch to Qt5; all that is required is to include the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">qgenericunixservices</code> and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">qgenericunixthemes</code> components in the build, and to append <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">"kde"</code> to the list returned by <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QCocoaIntegration::themeNames()</code> for instance under the condition that <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KDE_SESSION_VERSION</code> is set to a suitable value in the environment.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This will allow KF5 and Qt5 applications to use the theme selected through KDE if FrameworkIntegration is installed and KDE_SESSION_VERSION is set, which seems like a sufficiently specific set of conditions that is easy to avoid by users who prefer to use the Mac native theme.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">While requestion the KDE theme is also possible through <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">-style kde</code> or <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">env QT_STYLE_OVERRIDE=kde</code> the use of the KdePlatformTheme plugin appears to be the only way to get the full theme, including the font and colour selection. In my opinion it is above all the font customisation which is a very welcome feature for Qt/Mac; by default Qt applications use the default system font (Lucida Grande 13pt or even 14pt) throughout. This is a good UI font, but not at that size (and most "native" OS X applications indeed use a range of smaller sizes, depending on role.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It does have introduce a number of regressions, which the current patch aims to address. The most visible and problematic of these regressions is the loss of the Mac-style menu bar and thus of all menu items (actions).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The fix is straightforward : on OS X (and similarly affected platforms?), an instance of the native Cocoa platform theme is created through the private API, and used as a fallback rather than immediately falling back to the default implementations from <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QPlatformTheme</code>. In addition, methods missing from (not overridden by) <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KdePlatformTheme</code> are provided on OS X and call the corresponding methods from the native theme. It is this change which restores the menubar and even the Dock menu functionality.
One minor regression remains but should be easy to fix (elsewhere?): the Preferences menu loses its keyboard shortcut (Command-,).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Given the fallback nature of the native platform instance I have preferred to print a warning rather than using something like <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">qFatal</code>, above all because the message printed by qFatal tends to get lost on OS X. I can replace my use of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">qWarning</code> with a dialog giving the choice between continuing or exiting the application - code that would be called in the menu methods because only there is it certain that the application actually needs a menubar.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In line with experience and feedback from the KDE(4)-Mac community I have decided to force the use of native dialogs rather than the ones from the KdePlatformPlugin.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In addition I set the fallback value for <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ShowIconsOnPushButtons</code> to false in line with platform guidelines, and ensure that the autotests are not built as app bundles.</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;">On Mac OS X with Qt 5.5.1, KF5 frameworks 5.16.0 and QtCurve git/head.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have not verified to what extent my use of a private <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QGuiApplication</code> API links builds to a specific Qt version (I consider that nothing shocking and a minor price to pay).</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;">
<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;">
<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;">Do I need to add some glue to the CMake file so that it will warn if the private headerfiles are not available? Apparently no changes were required to find them.</p>
</blockquote>
</blockquote>
</blockquote></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>src/platformtheme/CMakeLists.txt <span style="color: grey">(23f590e)</span></li>
<li>src/platformtheme/kdemactheme.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/platformtheme/kdemactheme.mm <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/platformtheme/kdeplatformtheme.h <span style="color: grey">(97d09df)</span></li>
<li>src/platformtheme/kdeplatformtheme.cpp <span style="color: grey">(80dbcb7)</span></li>
<li>src/platformtheme/kfontsettingsdata.h <span style="color: grey">(4b92c7d)</span></li>
<li>src/platformtheme/kfontsettingsdata.cpp <span style="color: grey">(b0a4bbf)</span></li>
<li>src/platformtheme/kfontsettingsdatamac.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/platformtheme/kfontsettingsdatamac.mm <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/platformtheme/khintssettings.h <span style="color: grey">(ec064d3)</span></li>
<li>src/platformtheme/khintssettingsmac.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/platformtheme/khintssettingsmac.mm <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/platformtheme/main_mac.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126198/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/11/30/650d0da7-52b3-40d1-a1f9-cb610494cf77__Screen_Shot_2015-11-30_at_15.42.31.png">purely native OS X theme</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/11/30/72e2a6aa-2a7c-465b-b404-fc1e52b6fc69__Screen_Shot_2015-11-30_at_15.43.02.png">native theme but with `-style kde`</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/11/30/309e5995-74fa-42fb-a6f3-936cedbf5246__Screen_Shot_2015-11-30_at_15.43.31.png">using the KDEPlatformTheme</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/11/30/eaa1d907-bf05-4ca2-821b-83dc062aea04__QtCreatorNativeLNX.png">on Linux, using a purely "native" theme</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/12/01/de55a91f-3500-4db8-8a3b-d252fd7ea169__Screen_Shot_2015-12-01_at_13.52.35.png">KDEPlatformTheme with the "macintosh" native theme selected</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>