<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/123249/">https://git.reviewboard.kde.org/r/123249/</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 9th, 2015, 1:58 a.m. UTC, <b>Jan Kundrát</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;"><h2 style="font-size: 100%;text-rendering: inherit;padding: 0;white-space: normal;margin: 0;line-height: inherit;">-1 from me. The unconditional dep got introduced in 93918b1ec8f585dd135db1449e1b22b878f82fbc, which looks like a mistake to me. Why not:</h2>
<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;">find_package(KF5 CONFIG COMPONENTS Activities)</li>
</ul></pre>
</blockquote>
<p>On April 25th, 2015, 7:17 a.m. UTC, <b>Ivan Čukić</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;">While I don't mind making it again an optional dep, I want to mention that the Gwenview, Kate and KWrite made it a hard requirement. Is there a reason why Okular needs it to be optional?</p></pre>
</blockquote>
<p>On April 25th, 2015, 9:13 a.m. UTC, <b>Jan Kundrát</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;">IMHO reducing dependencies is a good thing in general. I for one do not have a use for KActivities.</pre>
</blockquote>
<p>On April 25th, 2015, 9:32 a.m. UTC, <b>Ivan Čukić</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;">If this is disabled during the build, the following will happen:</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;">Kicker will not show documents opened with okular in the recent documents section</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Share plasmoid will not support sharing the currently opened document in okular</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Tasks applet will not show recent documents for okular</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><img alt="Recent documents in Kicker" src="http://i.imgur.com/TVxCuL1.png" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
<img alt="Recent documents in Tasks" src="http://i.imgur.com/e4N38fQ.png" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
<img alt="Sharing a document" src="http://i.imgur.com/TVxCuL1.png" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" /></p></pre>
</blockquote>
<p>On April 25th, 2015, 9:51 a.m. UTC, <b>Ivan Čukić</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;">Duplicate image above</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><img alt="Recent kickoff" src="http://i.imgur.com/r4VYCs2.png" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" /></p></pre>
</blockquote>
<p>On April 25th, 2015, 10:18 a.m. UTC, <b>Jan Kundrát</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;">Anyway, if the Okular maintainers want to have this unconditional, that's their decision; I have plenty of RAM for those libs that I don't use :). What I was pointing out is just that the commit message (or actually the "text in this review") is not an accurate description of how these things actually happened. Maybe this would be a better description:</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%">Make KActivities a required dependency
Our goal is better integration with the rest of the desktop (FIXME: just KDE or anybody else?).
We have made a conscious decision to reduce our code a bit by removing the legacy #ifdefs. That way we do not have to support a build configuration which is going to be used by just a tiny minority of our users, and one that we are not terribly interested in. Without KActivities, there is for example no support for recording and listing of recent documents, PDF sharing etc etc, and we honestly believe that the majority of our audience wants these features.
</pre></div>
</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;">As I said, I'm fine either way. Just wanted to post what will happen when this is disabled.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The description you wrote is cool I have to say :)</p></pre>
<br />
<p>- Ivan</p>
<br />
<p>On April 3rd, 2015, 8:39 p.m. UTC, Ivan Čukić 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 Okular and Albert Astals Cid.</div>
<div>By Ivan Čukić.</div>
<p style="color: grey;"><i>Updated April 3, 2015, 8:39 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
okular
</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;">Since the a3fb02b881d commit, the KF5::Activities are a required dependency (CMakeLists.txt:31).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Still, the actual code uses ifdefs for KActivities_FOUND macro which no longer exists. This patch removes the macros and restores the feature.</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>autotests/CMakeLists.txt <span style="color: grey">(0a0f546)</span></li>
<li>shell/CMakeLists.txt <span style="color: grey">(ec36582)</span></li>
<li>shell/shell.h <span style="color: grey">(f345cf3)</span></li>
<li>shell/shell.cpp <span style="color: grey">(6bb89c5)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123249/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>