<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="http://git.reviewboard.kde.org/r/104814/">http://git.reviewboard.kde.org/r/104814/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 3rd, 2012, 10:05 a.m., <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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="http://git.reviewboard.kde.org/r/104814/diff/1/?file=61753#file61753line45" style="color: black; font-weight: bold; text-decoration: underline;">outputview/ifilterstrategy.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">public:</pre></td>
</tr>
</tbody>
<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">45</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * @param item Where all the metadata about the current line is put after the given filter is applied</span></pre></td>
</tr>
</tbody>
</table>
<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 severly dislike this api. please change it to something like
virtual FilteredItem errorInLine(const QString& line) const = 0;
then, adapt the FilteredItem to have a
bool isValid() const;
furthermore, make it so that a default-constructed FilteredItem() is not seen as valid.</pre>
</blockquote>
<p>On May 7th, 2012, 6:58 p.m., <b>Morten Volden</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;">If I understand you correctly. You would like this (in outputmodel.cpp):
FilteredItem item( line );
bool matched = m_filter->isErrorInLine(line, item);
if( !matched )
{
matched = m_filter->isActionInLine(line, item);
}
to be replaced by this:
FilteredItem item = m_filter->errorInLine(line);
if( !item.isValid ) {
item = m_filter->actionInLine(line);
}
But wouldn't returning a Filtered item (for both error and action) force a construction of such an object twice (instead of once) pr. input line? </pre>
</blockquote>
<p>On May 8th, 2012, 6:05 p.m., <b>Milian Wolff</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;">yes but this is a cheap struct, don't overoptimize at the cost of readability please
also, it should be .isValid() - i.e. call a function here that should decide whether it's valid or not (this should not require an additional data member imo).</pre>
</blockquote>
<p>On May 8th, 2012, 7:45 p.m., <b>Morten Volden</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 the first is just as readable as the other. However, I realize that I am entering a discussion about aesthetics - which is probably pointless. How about I make some unittest(s) where we can do some measuring. If the first is not significantly faster than the other I'd be happy to change it to what you suggested. </pre>
</blockquote>
<p>On May 8th, 2012, 8:17 p.m., <b>Milian Wolff</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;">if you really want to spent the time on measuring the time difference, sure - do that. be sure to compile it in release mode to get compiler optimizations etc.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Constructed a few tests to benchmark the outputmodel with different filtering strategies. The constructed tests do not show any noticeable difference between the two approaches. Changed the code to address the points mentioned in this item. Same applies to the issue below. </pre>
<br />
<p>- Morten</p>
<br />
<p>On May 6th, 2012, 8:59 p.m., Morten Volden wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Morten Volden.</div>
<p style="color: grey;"><i>Updated May 6, 2012, 8:59 p.m.</i></p>
<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;">So far I have:
* Moved some of the filtering code
- More specifically some of the code for filtering (Compiler output, script output )
* Extended the outputmodel
- It now inherits from QAbstractListModel and IOutputview model
- It is now possible to choose a filtering strategy on the outputview outputmodel
* Plus a few other things to make things work
* Started doing some Unit test of the different filters
The solution currently has four filtering strategies:
* No filter
* Compiler filter
* Script Error flter
* StaticAnalysis Filter (for cpp check, krazy, etc)
I think I have now come to a point where it would be good to get some feedback on the proposed solution
Work identified that remains to be done (Input is welcome):
* Finish tests for filter strategies
* Move MakeoutputModel tests to outputview and extend them
* Make Makebuildder use the code in outputview.
* Make NofilteringStrategy faster (i.e. do not create a filtered item for each line)
* Make custom buildsystem use the code in outputview - Done (But Needs to be reviewed)
* Extend filterstrategies with other filters?? (Input on that one is most welcome)
* Discuss how the filters should be selected via the GUI (and in which use-cases it makes sense to do so)
</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;">Started working on Unittest for filtering strategies. Plan to move most tests from Makeoutputmodel at some point. Plus used the patch for a week now and nothing seemingly breaks...</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>outputview/CMakeLists.txt <span style="color: grey">(5d9e539)</span></li>
<li>outputview/delegateholder.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/delegateholder.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/filtereditem.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/filtereditem.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/ifilterstrategy.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/ifilterstrategy.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/outputdelegate.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/outputdelegate.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/outputfilteringstrategies.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/outputfilteringstrategies.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/outputformats.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/outputformats.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/outputjob.h <span style="color: grey">(c17b592)</span></li>
<li>outputview/outputjob.cpp <span style="color: grey">(5b9db06)</span></li>
<li>outputview/outputmodel.h <span style="color: grey">(b4c9631)</span></li>
<li>outputview/outputmodel.cpp <span style="color: grey">(317e3ee)</span></li>
<li>outputview/tests/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/tests/filteringstrategytest.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/tests/filteringstrategytest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/tests/outputviewtest.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/tests/outputviewtest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>outputview/tests/projects/onefileproject/main.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>plugins/execute/nativeappjob.cpp <span style="color: grey">(3eb654c)</span></li>
<li>plugins/executescript/executescriptoutputmodel.h <span style="color: grey">(180adbd)</span></li>
<li>plugins/executescript/executescriptoutputmodel.cpp <span style="color: grey">(1c852e9)</span></li>
<li>plugins/executescript/executescriptplugin.h <span style="color: grey">(4eea6a4)</span></li>
<li>plugins/executescript/executescriptplugin.cpp <span style="color: grey">(e55b514)</span></li>
<li>plugins/executescript/scriptappconfig.h <span style="color: grey">(2e401ea)</span></li>
<li>plugins/executescript/scriptappconfig.cpp <span style="color: grey">(d7ff984)</span></li>
<li>plugins/executescript/scriptappjob.h <span style="color: grey">(48400aa)</span></li>
<li>plugins/executescript/scriptappjob.cpp <span style="color: grey">(3b68ca5)</span></li>
<li>plugins/externalscript/externalscriptjob.h <span style="color: grey">(1922469)</span></li>
<li>plugins/externalscript/externalscriptjob.cpp <span style="color: grey">(aeb5a34)</span></li>
<li>plugins/externalscript/externalscriptoutputmodel.cpp <span style="color: grey">(7a3d5d4)</span></li>
<li>plugins/externalscript/externalscriptplugin.h <span style="color: grey">(c87aaa8)</span></li>
<li>plugins/externalscript/externalscriptplugin.cpp <span style="color: grey">(6d62b63)</span></li>
<li>vcs/dvcs/dvcsjob.cpp <span style="color: grey">(1e0f187)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104814/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/104814/s/550/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/05/01/externalscriptCPPcheckBefore_400x100.png" style="border: 1px black solid;" alt="CPPCheckBefore" /></a>
<a href="http://git.reviewboard.kde.org/r/104814/s/551/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/05/01/ExternalscriptCPPcheckAfter_400x100.png" style="border: 1px black solid;" alt="CppcheckAfter" /></a>
<a href="http://git.reviewboard.kde.org/r/104814/s/552/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/05/01/ExternalScriptQuickCompileBefore_400x100.png" style="border: 1px black solid;" alt="QuickCompileBefore" /></a>
<a href="http://git.reviewboard.kde.org/r/104814/s/553/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/05/01/ExternalScriptQuickCompileAfter_400x100.png" style="border: 1px black solid;" alt="QuickCompileAfter" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>