<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/118510/">https://git.reviewboard.kde.org/r/118510/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Awesome work, I am looking at details, what should take some time. First nitpicks below.</pre>
<br />
<div>
<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/118510/diff/3/?file=279161#file279161line29" style="color: black; font-weight: bold; text-decoration: underline;">CalligraProducts.cmake</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">29</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c"># (NEEDED), the whole productset will not be build if a product is missing</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">For me not clear from the 1st second.. how about REQUIRED? "Required" is feels natural and is used in sentences below too. When we need a verb, REQUIRES is ok.
NEEDS does not emphasize whether the "NEED" is a hard requirement.</pre>
</div>
<br />
<div>
<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/118510/diff/3/?file=279161#file279161line30" style="color: black; font-weight: bold; text-decoration: underline;">CalligraProducts.cmake</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">30</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="c"># another internal or external dependency. If optional (WANTED), the rest of the</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">For me not clear from the 1st second.. how about OPTIONAL? "Optional" is feels natural and is used in sentences below too.
Since we need as verb, RECOMMENDS is also good.
"WANTS" does not emphasize optionality.</pre>
</div>
<br />
<div>
<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/118510/diff/3/?file=279161#file279161line90" style="color: black; font-weight: bold; text-decoration: underline;">CalligraProducts.cmake</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">90</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">calligra_define_product</span><span class="p">(</span><span class="s">APP_WORDS</span> <span class="s2">"Words executable (for Desktop)"</span> <span class="s">NEEDS</span> <span class="s">PART_WORDS</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">For APP_* I recommend using descriptions like "Full <appname> app (for Desktop)":
- "executable" is a technical detail
- adding "app" makes it clear what it is</pre>
</div>
<br />
<p>- Jarosław Staniek</p>
<br />
<p>On June 5th, 2014, 5:52 p.m. UTC, Friedrich W. H. Kossebau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Calligra, Yue Liu, Boudewijn Rempt, and Jarosław Staniek.</div>
<div>By Friedrich W. H. Kossebau.</div>
<p style="color: grey;"><i>Updated June 5, 2014, 5:52 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
calligra
</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;">Based on the feedback, adaption and more own tinkering with the concepts, in this patch I propose to do this:
* split the old idea of a "product" into "product", "feature" and "productset"
* make no real difference between user-defined productsets and predefined ones
* move the definition of the products into a separate file, for better overview and discoverability
* put the documentation directly into that separate file instead of hiding it away in some README in some subdirectory noones knows about
* generate a file with the graph of the dependencies, showing also the build state (in DOT notation, see attached SVG file generated from that)
* more patterns for the ids of the products
* allow to have multiple productsets/products/features mentioned in the -DPRODUCTSET flag (taken with WANTED property for now) (as asked for with https://git.reviewboard.kde.org/r/118187)
Surely this whole thing is in danger to be overengineered :) Especially given that Krita devs have not picked up this for all the Sketch, Gemini, Steam etc variants makes me wonder if it is perhaps. But then it might just be underdocumented and uncommon and people have not yet discovered the nice build customizations this offers over what is usually possible. Time to roll the drums about this, mean, blog :)
One thing still missing: proper disabling of products with selected product sets. But lets leave that for a separate RR.</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">(86297a1)</span></li>
<li>CalligraProducts.cmake <span style="color: grey">(PRE-CREATION)</span></li>
<li>README.PACKAGERS <span style="color: grey">(4ebc026)</span></li>
<li>active/CMakeLists.txt <span style="color: grey">(4361fb6)</span></li>
<li>cmake/modules/CalligraProductSetMacros.cmake <span style="color: grey">(697c046)</span></li>
<li>cmake/productsets/README <span style="color: grey">(5460955)</span></li>
<li>cmake/productsets/active.cmake <span style="color: grey">(089a085)</span></li>
<li>cmake/productsets/all.cmake <span style="color: grey">(4619358)</span></li>
<li>cmake/productsets/creative.cmake <span style="color: grey">(2b8fc45)</span></li>
<li>cmake/productsets/desktop.cmake <span style="color: grey">(83db3a1)</span></li>
<li>cmake/productsets/kexi+sheets.cmake <span style="color: grey">(3fe2f48)</span></li>
<li>cmake/productsets/kexi.cmake <span style="color: grey">(7dd2345)</span></li>
<li>cmake/productsets/krita.cmake <span style="color: grey">(2c814d3)</span></li>
<li>cmake/productsets/osx.cmake <span style="color: grey">(8e32b13)</span></li>
<li>cmake/productsets/sheets.cmake <span style="color: grey">(5de2103)</span></li>
<li>devtools/CMakeLists.txt <span style="color: grey">(bfa5599)</span></li>
<li>extras/CMakeLists.txt <span style="color: grey">(a534b0e)</span></li>
<li>extras/quickprint/CMakeLists.txt <span style="color: grey">(9ad61d2)</span></li>
<li>extras/thumbnail/CMakeLists.txt <span style="color: grey">(92156b3)</span></li>
<li>filters/flow/visio/import/CMakeLists.txt <span style="color: grey">(1de4385)</span></li>
<li>filters/flow/wpg/import/CMakeLists.txt <span style="color: grey">(524d1b1)</span></li>
<li>filters/karbon/karbon1.x/CMakeLists.txt <span style="color: grey">(87bdb73)</span></li>
<li>filters/karbon/wmf/CMakeLists.txt <span style="color: grey">(c25ed2c)</span></li>
<li>filters/karbon/wpg/CMakeLists.txt <span style="color: grey">(5f578e9)</span></li>
<li>filters/karbon/xfig/CMakeLists.txt <span style="color: grey">(1e0e2c6)</span></li>
<li>filters/sheets/excel/import/CMakeLists.txt <span style="color: grey">(07aa186)</span></li>
<li>filters/sheets/xlsx/CMakeLists.txt <span style="color: grey">(80d51aa)</span></li>
<li>filters/stage/keynote/import/CMakeLists.txt <span style="color: grey">(1b44b52)</span></li>
<li>filters/stage/kpr2odf/CMakeLists.txt <span style="color: grey">(31c69b0)</span></li>
<li>filters/stage/powerpoint/CMakeLists.txt <span style="color: grey">(60e1ccc)</span></li>
<li>filters/stage/pptx/CMakeLists.txt <span style="color: grey">(31aef35)</span></li>
<li>filters/words/docx/import/CMakeLists.txt <span style="color: grey">(d9f0cec)</span></li>
<li>filters/words/msword-odf/CMakeLists.txt <span style="color: grey">(8bd3806)</span></li>
<li>filters/words/rtf/import/CMakeLists.txt <span style="color: grey">(df6f50d)</span></li>
<li>filters/words/wordperfect/import/CMakeLists.txt <span style="color: grey">(6a0e2d7)</span></li>
<li>filters/words/works/import/CMakeLists.txt <span style="color: grey">(76317ae)</span></li>
<li>kexi/plugins/CMakeLists.txt <span style="color: grey">(bf389f2)</span></li>
<li>krita/plugins/formats/ora/CMakeLists.txt <span style="color: grey">(cd1a605)</span></li>
<li>libs/CMakeLists.txt <span style="color: grey">(d482a4b)</span></li>
<li>libs/kotext/CMakeLists.txt <span style="color: grey">(9f9e506)</span></li>
<li>plan/plugins/CMakeLists.txt <span style="color: grey">(e06e339)</span></li>
<li>plugins/textshape/CMakeLists.txt <span style="color: grey">(f8d75f5)</span></li>
<li>sheets/CMakeLists.txt <span style="color: grey">(a45829d)</span></li>
<li>sheets/plugins/CMakeLists.txt <span style="color: grey">(6a45389)</span></li>
<li>stage/CMakeLists.txt <span style="color: grey">(eaf8ecf)</span></li>
<li>words/CMakeLists.txt <span style="color: grey">(033db4d)</span></li>
<li>words/part/CMakeLists.txt <span style="color: grey">(2bf1b0b)</span></li>
<li>words/part/tests/CMakeLists.txt <span style="color: grey">(6fd9b0c)</span></li>
<li>words/plugins/CMakeLists.txt <span style="color: grey">(229cff8)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118510/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/06/04/b1958536-ca1c-4552-8c3d-120a9d8b786f__dependencies.svg">Dependencies graph</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/06/04/8924bbdf-b05c-426d-b469-d5bceb87bac7__kexisheetdependencies.svg">Dependencies graph with just Kexi & Sheets built</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>