<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/120287/">https://git.reviewboard.kde.org/r/120287/</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 20th, 2014, 10:20 a.m. CEST, <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/120287/diff/3/?file=313628#file313628line56" style="color: black; font-weight: bold; text-decoration: underline;">kcontrol/krdb/krdb.cpp</a>
<span style="font-weight: normal;">
(Diff revision 3)
</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">56</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></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;">added newline</p></pre>
</blockquote>
<p>On September 20th, 2014, 11:19 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;">Ah, the famous German rigor ... is this really an issue, shouldn't that empty line have been there in the 1st place?!</p></pre>
</blockquote>
<p>On September 20th, 2014, 11:24 a.m. CEST, <b>Martin Gräßlin</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;">yes of course. It's a change which has nothing to do with the overall change. It's not atomic any more.</p></pre>
</blockquote>
<p>On September 20th, 2014, 11:29 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;">shouldn't that empty line have been there in the 1st place?!</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">c++98 newline @ EOF, later versions don't, ie. it's not required to add that line despite you may get warnings about it (and actually kate has an option to automatically add it on saving)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Do you use xcode?</p></pre>
</blockquote>
<p>On September 20th, 2014, 11:30 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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">c++98 <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">required</em> newline @ EOF ...</p></pre>
</blockquote>
<p>On September 20th, 2014, 11:39 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;">But this is not @EOF, it's a newline inserted to detach the 1st function from the last #include statement (or rather, the last conditional #ifdef Q_WS_X11 include).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Given that that's an X11 conditional include, I don't think that cosmetic change (adding a newline) is completely irrelevant either.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Thomas: yes, I use Xcode, but not here. CMake can generate Xcode projects, but that only works for relatively simple ones, so it's not even feasible to use Xcode. Most of the time I use KDevelop (git/kde4-legacy) because of the access to the documentation it gives. I can give less guarantee that I didn't edit the file in vi at some point, though ;)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">[OT]: there's a Google plugin for Xcode that removes white space from empty lines and I think also the final newline. And I do use Xcode for my OS X Keychain work from time to time, because KDevelop can't access the native SDK documentation.[/OT]</p></pre>
</blockquote>
<p>On September 20th, 2014, 12:03 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;">Indeed - then this change is completely unrelated and pointless.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, there probably <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">should</em> have been a newline in the first place, but it's actually irrelevant whether there is and such changes don't belong into this kind of patch.</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;">I don't think that cosmetic change (adding a newline) is completely irrelevant either.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The point is that this is a purely cosmetic change, unrelated to the rest of your patch.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
You are right, that one would demand a newline in <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">new code</em> here, but it doesn't belong in your patch.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you wish to tidy up aged code for KF5, you're very welcome =)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Here's for vim:<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
https://techbase.kde.org/Policies/Kdelibs_Coding_Style#Vim</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For kate, "configure editor..."<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
/"Appearance"<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
o) Highlight Tabs<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
o) Highlight trailign spaces<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
o) Show indention lines<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
/"editing"/"indention"<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
o) Spaces<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Tab width: 4 chars<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Indention width: 4 chars<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
/"Open/Save"<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Remove Trailing space: on modified lines<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
) Append newline at end of file</p></pre>
</blockquote>
<p>On September 20th, 2014, 12:37 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;">Well, if said German rigor has it come to that, I'll remove the newline. But than I'll also keep from changing the Darwin references to APPLE in the CMake files.</p></pre>
</blockquote>
<p>On September 20th, 2014, 1:04 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;">But than I'll also keep from changing the Darwin references to APPLE in the CMake files.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is indeed nothing that belongs into this patch, but may still be a required patch in general (to permit Darwin + X11 in future versions) that Apple users might be interested in.</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 beg to differ; this patch is for building kde-workspace on OS X, and changing CMake keywords to what's currently the appropriate way to detect that platform seems perfectly relevant to me. I'd have done (and commented) that if I'd noticed the use of 2 different keywords.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For the rest, there's almost 0 chance that Apple (as in OS X) users will ever be able again to run KDE under X11 so it's extremely unlikely that future KDE versions will run into a platform that calls itself Darwin but also requires X11.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And if we really can be pedantic, I'd like to point out that <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">Darwin</code> is actually a better keyword than <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">APPLE</code> because it refers to the software environment only and not the vendor. Apple will most likely always remain the sole OS X vendor, but OS X is likely to be replaced some day by something that's not <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">(APPLE AND Darwin)</code>.</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On September 20th, 2014, 12:54 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 and kde-workspace.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Sept. 20, 2014, 12:54 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kde-workspace
</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;">A few rather straightforward patches to make the relevant bits of KDE4's kde-workspace build and function on OS X.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
The main interest is having the systemsettings control panel to control the various relevant KDE settings among which desktop search, fonts, colours and even style.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
The oxygen style builds and looks good but shows some updating glitches due to compositing.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm submitting this patch partly in hope it may be useful in bringing kf5-workspace to OS X, one day.</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 OS X 10.6.8 and 10.9.4 with KDE/MacPorts (4.12.5 and more recently kdelibs git/master, 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>CMakeLists.txt <span style="color: grey">(195f99c)</span></li>
<li>kcontrol/CMakeLists.txt <span style="color: grey">(fc666b1)</span></li>
<li>kcontrol/krdb/krdb.cpp <span style="color: grey">(36fc99c)</span></li>
<li>kcontrol/style/CMakeLists.txt <span style="color: grey">(d832b20)</span></li>
<li>libs/CMakeLists.txt <span style="color: grey">(c0576fe)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120287/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/2014/09/20/b212730f-6258-4277-851c-226bc0673aa1__patchreview-20140920.patch">copy of the diff file saved locally, which had no tabs when I uploaded it. Checksum: 3989cdd46af3c891e570974d66c330403dcd41c4ee5e17a372fa385080cbabd1 </a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>