<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/126161/">https://git.reviewboard.kde.org/r/126161/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 10th, 2016, 8:31 a.m. UTC, <b>David Faure</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;">I liked what I was seeing in this patch .... until I got to the horrible code duplication introduced by kinit_mac.mm. This is not the way to go.
Yes #ifdefs are a pain, but code duplication is 1000 times worse. In Qt, the _mac.mm files only contain code that is mac specific.
You could move all the code that is common between mac and linux into a separate helper file (say kinit_common.{cpp,h}), or you could organize it this way:
kinit.cpp/kinit.h -> shared code
kinit_unix.cpp -> linux/bsd-specific
kinit_mac.mm -> OSX specific
(and the windows guys can make a kinit_win.cpp if they need to, although I'm not sure they use kdeinit at all)
Code duplication is the worst enemy of maintenability, I will never accept a patch that duplicates lots and lots of code. But other than that, many of the changes in this patch seem very sensible, I will definitely approve it once the code duplication is gone. Thanks!</pre>
</blockquote>
<p>On April 11th, 2016, 2:37 p.m. UTC, <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 must admit that I hadn't yet done a side-by-side diff of kinit.cpp and kinit_mac.mm . That indeed looks horrible, and makes me wonder why I went this route.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">A quick look suggests that
- <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">launch()</code> could go into a platform-specific file, with an occasional other function like <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">show_message_box()</code>
- most other specific bits in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kinit_mac.mm</code> correspond to parts in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kinit.cpp</code> that already have an <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">#ifdef HAVE_X11</code> or <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">#ifdef Q_OS_LINUX</code> or equivalent. Putting the Mac-specific bits into an <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">#elseif</code> doesn't really make things more complex - qualitatively speaking at least.
- lots of the functions that could go into a common file that would be combined with Linux, Mac/OS X (and MSWin) specific implementation files are marked static. Most if not all of those would have to become non-static, but it seems there's no real point in limiting the visibility of those functions anyway?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Merging kinit.cpp and kinit_mac.mm back together and splitting off just the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">launch()</code> function would certainly be the easier and quicker solution, but which do you prefer?</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yes, merging back and splitting only what is mac-specific sounds good.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Go for the "easier" way, it's only the only way that can really be reviewed (moving code doesn't lead to easy reviewing if the code is also modified on the way, so moving code should be done as a separate commit when needed -- but from what you said it sounds like there isn't much moving needed at all, compared to the code currently in git).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">About visibility: kdeinit5 isn't a shared library, it's an executable, so it doesn't matter, nothing is "visible".</p></pre>
<br />
<p>- David</p>
<br />
<p>On April 6th, 2016, 5:16 p.m. UTC, 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 Frameworks.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated April 6, 2016, 5:16 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kinit
</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 patch addresses several issues with the OS X adaptations:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">-1 replaces the obsolete Q_OS_MAC with Q_OS_OSX
-2 builds the relevant applications <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">nongui</code> instead of as app bundles
-3 turns klauncher into an "agent" by setting <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">LSUIElement</code> to true programmatically
-4 ports a patch that has been in MacPorts' <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">port:kdelibs4</code> since October 14th 2009, which prevents a kdeinit crash that is caused by calling exec after <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">fork()</code> in an application that has used non-POSIX APIs and/or calling those APIs in the exec'ed application. This patch (originally by MacPorts developers Jeremy Lainé and Jeremy Lavergne) rearranges call order and uses a proxy application to do the actual exec.</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.9.5 with Qt 5.5.1 and KF5rameworks 5.16.0 as well as Qt 5.6 and 5.20.0 . With this patch, starting <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kded5</code> will launch kdeinit5 and klauncher5 as expected, but <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kdeinit5 --kded</code> does not yet launch <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">kded5</code>. This is probably acceptable for typical KF5 use on OS X (kded5 can be launched as a login item or as a LaunchAgent) but I will have another look at why the kded isn't started.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I am not yet able to perform further testing; practice will for instance have to show whether point 2 above needs revision (apps that need to be installed as app bundles).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Similarly it will have to be seen whether point 3 above has any drawbacks. Applications running as agents do not show up in the Dock or App Switcher. Thus, klauncher will not be able to "turn itself into" an application that does have a full GUI presence with my current modification. I don't know if that's supposed to be possible though.
NB: I have been building the KDE4 klauncher in a way that makes it impossible to construct a GUI at all, so I'm not expecting issues in KF5 as long as klauncher's role hasn't evolved too much.</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>src/kdeinit/CMakeLists.txt <span style="color: grey">(ae619f7)</span></li>
<li>src/kdeinit/kinit.cpp <span style="color: grey">(ca18603)</span></li>
<li>src/kdeinit/kinit_mac.mm <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/klauncher/CMakeLists.txt <span style="color: grey">(a8e6c3e)</span></li>
<li>src/klauncher/klauncher.h <span style="color: grey">(53c0803)</span></li>
<li>src/klauncher/klauncher.cpp <span style="color: grey">(baa5649)</span></li>
<li>src/klauncher/klauncher_main.cpp <span style="color: grey">(710c889)</span></li>
<li>src/start_kdeinit/CMakeLists.txt <span style="color: grey">(46d6cb3)</span></li>
<li>src/wrapper.cpp <span style="color: grey">(9cb0a71)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126161/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>