<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 19th, 2014, 10:24 nachm. 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;">overall I rather tend to -1 for these changes. I consider changing the build system in a long term release as way too risky considering that the core development doesn't use this iteration any more. Any unintended breakage (e.g. a component also not being built on X11) would probably not be spotted and that's something we certainly don't want to do in a minor version.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Given that we are already in a late state of the bug fix only development it should not be difficult to keep the patch downstream as a distro-specific patch.</p></pre>
</blockquote>
<p>On September 19th, 2014, 10:54 nachm. 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;">That's re: the changes to the CMakefile? I frankly don't see the risk. I took care to piggyback most changes to those files on WIN32 conditionals, and there are no APPLEs running Qt/X11 that could run this kde-workspace versin. But checking for false positives on Linux is easy enough (I can try to do that over the weekend). Besides, core development may not be using "this iteration" anymore, but that doesn't mean no one else would spot a hypothetical regression...</p></pre>
</blockquote>
<p>On September 19th, 2014, 11:25 nachm. CEST, <b>Ben Cooksley</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;">Martin, can you please clarify how changing an if( NOT WIN32 ) to if( NOT WIN32 AND NOT APPLE ) would make a difference to platforms which are not Windows or Apple based?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">We've been trying to encourage upstreaming of patches from both the Windows and Mac builds to make it easier to build these directly from source. This is particularly crucial with Plasma Next / KF5 / etc.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><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;">can you please clarify how changing an if( NOT WIN32 ) to if( NOT WIN32 AND NOT APPLE ) would make a difference to platforms which are not Windows or Apple based?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">which it isn't. There are also changes in the structure with now more if-else. To me each of them is a risky change as the whole thing is no longer tested.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Personally I always weight the risk of regression against the probably gain for each patch going into the stable branch. For our main platforms in this case there is no gain, only the risk for regression. So with my Linux developer hat on, I don't like the idea of it being touched.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also I do not understand why one would want kde-workspace on MacOS. With Windows maintainers we just concluded that it doesn't make sense to offer it and the new split repositories don't support building for Windows any more. Given that I just fail to see what would be the gain of having kde-workspace being built on MacOS or what advantages this would give to users on said platform. E.g. there is now the launch KCM being built on MacOS. Who will benefit by that? It's a KCM to configure the behavior of X11 startup notifications. The settings are read by KRunner, the taskmanager and KWin. All three are excluded from building, though. Fonts KCM is not being built, but fontinstall is. Again it probably will build, but it's completely useless - I have seen the X11 dependency of it when trying to use it on Wayland. krdb is being built, although it's so X11 specific that there is now a huge #ifdef Q_WS_X11.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">While I appreciate the work on making our software work on more systems, I highly doubt there is need in making the LTS kde-workspace module build on MacOS. I think it would be better to look at the now split repositories and provide those which make sense without needing to introduce risky changes.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On September 20th, 2014, 12:05 vorm. 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:05 vorm.</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>
</td>
</tr>
</table>
</div>
</body>
</html>