Review Request: Share code between makebuilder and external scripts
Milian Wolff
mail at milianw.de
Thu May 3 10:05:38 UTC 2012
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104814/#review13328
-----------------------------------------------------------
phew large patch and still many issues in there but overall the correct direction - many thanks!
besides the small nitpicks below, I think we still need to solve two fundamental issues:
a) user-defined rules. this is nothing I expect to be implemented by this patch, but something that should be kept in mind. imo, the architecture you define here would not allow this without deep changes so, while at it, please adapt the api to take this into account.
b) filter strategies: now, external scripts have a "script" strategy and executescript has the "compiler" strategy... this looks strange and is probably too hard coded
both issues are imo related. I think "strategies", or filters in general, should be extensible. there should be some api to allow one to add filters which would be used to replace the existing filter strategies. then on usage the job would just go over all existing filters and use the first matching one. maybe we can keep the idea of the strategies as categories, such that outputjob users can say "only filters in category 'compiler'" or similar...
outputview/CMakeLists.txt
<http://git.reviewboard.kde.org/r/104814/#comment10449>
lots of trailing spaces ;-) you should configure your editor to show them to you to prevent that in the future
outputview/delegateholder.h
<http://git.reviewboard.kde.org/r/104814/#comment10450>
please add a small apidox that explains what this class does
generally I find it rather ugly that every plugin must now inherit this and set the job's delegate... can't this be automated/abstracted somehow to make this class obsolete?
outputview/delegateholder.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10452>
unindent please and remove trailing spaces
outputview/filtereditem.h
<http://git.reviewboard.kde.org/r/104814/#comment10453>
make it:
/**\n
* Holds all ...
* ...
**/
also remove trailing spaces
outputview/filtereditem.h
<http://git.reviewboard.kde.org/r/104814/#comment10454>
unindent and make it a struct
outputview/filtereditem.h
<http://git.reviewboard.kde.org/r/104814/#comment10456>
please make this a FilteredOutputItemType and convert to qvariant wherever needed
outputview/filtereditem.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10451>
remove trailing spaces and change the style of the mem-init-list:
foo::foo()
: a()
, b()
, ...
(i.e. one item per line, : and , at the beginning of the line)
outputview/ifilterstrategy.h
<http://git.reviewboard.kde.org/r/104814/#comment10457>
remove trailing spaces
outputview/ifilterstrategy.h
<http://git.reviewboard.kde.org/r/104814/#comment10458>
please make this
/**
* useful comments explaining this class
* and what it does
*
* @author ...
*/
outputview/ifilterstrategy.h
<http://git.reviewboard.kde.org/r/104814/#comment10460>
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.
outputview/ifilterstrategy.h
<http://git.reviewboard.kde.org/r/104814/#comment10461>
same as above
outputview/ifilterstrategy.h
<http://git.reviewboard.kde.org/r/104814/#comment10459>
remove this line
outputview/outputdelegate.h
<http://git.reviewboard.kde.org/r/104814/#comment10462>
remove this
outputview/outputdelegate.h
<http://git.reviewboard.kde.org/r/104814/#comment10463>
please pimpl public API
outputview/outputdelegate.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10464>
apply style change as explained above
outputview/outputdelegate.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10465>
please change the style:
if ( foo ) {
...
} else if ( bar ) {
...
}
outputview/outputfilteringstrategies.h
<http://git.reviewboard.kde.org/r/104814/#comment10466>
missing space indentation to line up the *
outputview/outputfilteringstrategies.h
<http://git.reviewboard.kde.org/r/104814/#comment10467>
pimpl public api please
outputview/outputfilteringstrategies.h
<http://git.reviewboard.kde.org/r/104814/#comment10471>
no QLinkedList, you really don't want that 99% of the cases, use QVector
outputview/outputfilteringstrategies.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10468>
remove this
outputview/outputfilteringstrategies.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10473>
remove newline
foreach() {
...
}
outputview/outputfilteringstrategies.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10472>
missing {}
outputview/outputfilteringstrategies.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10469>
style change as explained above:
if ( ... ) {
...
}
outputview/outputfilteringstrategies.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10470>
} else {
outputview/outputmodel.h
<http://git.reviewboard.kde.org/r/104814/#comment10476>
use an enum for this:
enum CustomRoles {
OutputItemTypeRole = Qt::UserRole + 1
};
outputview/outputmodel.h
<http://git.reviewboard.kde.org/r/104814/#comment10474>
use the pimpl! don't remove it!
outputview/outputmodel.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10477>
remove debug output
outputview/tests/filteringstrategytest.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10479>
break long lines please
outputview/tests/outputviewtest.h
<http://git.reviewboard.kde.org/r/104814/#comment10480>
?
outputview/tests/outputviewtest.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10481>
this test should be made functional
plugins/executescript/scriptappconfig.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10484>
unindent, put : at start of line
plugins/executescript/scriptappconfig.cpp
<http://git.reviewboard.kde.org/r/104814/#comment10483>
remove this line
- Milian Wolff
On May 1, 2012, 7:03 p.m., Morten Volden wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104814/
> -----------------------------------------------------------
>
> (Updated May 1, 2012, 7:03 p.m.)
>
>
> Review request for KDevelop.
>
>
> Description
> -------
>
> 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)
>
>
> Diffs
> -----
>
> outputview/CMakeLists.txt 5d9e539
> outputview/delegateholder.h PRE-CREATION
> outputview/delegateholder.cpp PRE-CREATION
> outputview/filtereditem.h PRE-CREATION
> outputview/filtereditem.cpp PRE-CREATION
> outputview/ifilterstrategy.h PRE-CREATION
> outputview/ifilterstrategy.cpp PRE-CREATION
> outputview/outputdelegate.h PRE-CREATION
> outputview/outputdelegate.cpp PRE-CREATION
> outputview/outputfilteringstrategies.h PRE-CREATION
> outputview/outputfilteringstrategies.cpp PRE-CREATION
> outputview/outputformats.h PRE-CREATION
> outputview/outputformats.cpp PRE-CREATION
> outputview/outputmodel.h b4c9631
> outputview/outputmodel.cpp 317e3ee
> outputview/tests/CMakeLists.txt PRE-CREATION
> outputview/tests/filteringstrategytest.h PRE-CREATION
> outputview/tests/filteringstrategytest.cpp PRE-CREATION
> outputview/tests/outputviewtest.h PRE-CREATION
> outputview/tests/outputviewtest.cpp PRE-CREATION
> outputview/tests/projects/onefileproject/main.cpp PRE-CREATION
> plugins/execute/nativeappjob.cpp 3eb654c
> plugins/executescript/executescriptoutputmodel.h 180adbd
> plugins/executescript/executescriptoutputmodel.cpp 1c852e9
> plugins/executescript/executescriptplugin.h 4eea6a4
> plugins/executescript/executescriptplugin.cpp e55b514
> plugins/executescript/scriptappconfig.h 2e401ea
> plugins/executescript/scriptappconfig.cpp d7ff984
> plugins/executescript/scriptappjob.h 48400aa
> plugins/executescript/scriptappjob.cpp 3b68ca5
> plugins/externalscript/externalscriptjob.h 1922469
> plugins/externalscript/externalscriptjob.cpp aeb5a34
> plugins/externalscript/externalscriptoutputmodel.cpp 7a3d5d4
> plugins/externalscript/externalscriptplugin.h c87aaa8
> plugins/externalscript/externalscriptplugin.cpp 6d62b63
> vcs/dvcs/dvcsjob.cpp 1e0f187
>
> Diff: http://git.reviewboard.kde.org/r/104814/diff/
>
>
> Testing
> -------
>
> 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...
>
>
> Screenshots
> -----------
>
> CPPCheckBefore
> http://git.reviewboard.kde.org/r/104814/s/550/
> CppcheckAfter
> http://git.reviewboard.kde.org/r/104814/s/551/
> QuickCompileBefore
> http://git.reviewboard.kde.org/r/104814/s/552/
> QuickCompileAfter
> http://git.reviewboard.kde.org/r/104814/s/553/
>
>
> Thanks,
>
> Morten Volden
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120503/1f03a24f/attachment.html>
More information about the KDevelop-devel
mailing list