<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/122737/">https://git.reviewboard.kde.org/r/122737/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 27th, 2015, 5:09 p.m. UTC, <b>Aaron J. Seigo</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;">Too many things in one patch.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Until the frameworks branch is merged into master, I don't want files moving around unless there is a <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">compelling</em> reason for it, and in this case there isn't.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Moving the version definition to the cmake file is fine; adding guards to the generated header is also fine.</p></pre>
</blockquote>
<p>On February 27th, 2015, 5:22 p.m. UTC, <b>Boudhayan Gupta</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;">Thanks for the feedback.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I can revert the moves, and submit a new patch with just moving version definition to CMakeLists and guarding the headers. About the re-factored CMakeLists - should I make only minimal changes (just adding the version definitions), or tidy it up so that if one day the files move around, creating the new CMakeLists for the subdirs is just a matter of cutting out bits from the top level file and pasting them into the subdir ones?</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;">tidying it up is cool, and probably much needed. thanks (in advance) for reworking the patch</p></pre>
<br />
<p>- Aaron J.</p>
<br />
<p>On February 27th, 2015, 7:06 p.m. UTC, Boudhayan Gupta 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 KSnapshot and Aleix Pol Gonzalez.</div>
<div>By Boudhayan Gupta.</div>
<p style="color: grey;"><i>Updated Feb. 27, 2015, 7:06 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
ksnapshot
</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 is a pretty big patch. It does the following:</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;">Move all the .cpp and .h files to an "src" directory</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Move the XDG desktop file to a "desktop" directory</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Move the SVG icon source for the hicolor icon to the icons directory</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Refactor the CMakeLists.txt file</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Give each subdirectory (src, icons, desktop) its own CMakeLists.txt. doc already had one.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Guard config-ksnapshot.h.cmake contents within ifndef-define-endif preprocessor directives</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Move the KSNAPVERSION define from main.cpp to config-ksnapshot.h.cmake</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">The KSnapshot version is now defined in the top-level CMakeLists.txt</li>
</ul></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;">Builds and installs fine. No functionality was added in this patch, so didn't test much beyond build/run/take screenshots.</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">(d174abf)</span></li>
<li>Messages-i18n.sh <span style="color: grey">(PRE-CREATION)</span></li>
<li>Messages-qt.sh <span style="color: grey">(PRE-CREATION)</span></li>
<li>Messages.sh <span style="color: grey">(d242e38)</span></li>
<li>config-ksnapshot.h.cmake <span style="color: grey">(3514dd5)</span></li>
<li>main.cpp <span style="color: grey">(ef03554)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122737/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>