Review Request 124123: Feedback request. Problems ToolView GUI changes and ProblemModel refactoring.

Milian Wolff mail at milianw.de
Mon Jul 6 13:53:15 UTC 2015 

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124123/#review82130
-----------------------------------------------------------



interfaces/iproblem.h (line 33)
<https://git.reviewboard.kde.org/r/124123/#comment56433>

    this should not be closed here, but below. I.e. the IProblem should be in the KDevelop namespace



language/duchain/navigation/problemnavigationcontext.cpp (line 34)
<https://git.reviewboard.kde.org/r/124123/#comment56434>

    while at it: const&



language/duchain/problem.cpp (line 138)
<https://git.reviewboard.kde.org/r/124123/#comment56435>

    while at it: const&



plugins/problemreporter/problemreportermodel.h (line 43)
<https://git.reviewboard.kde.org/r/124123/#comment56436>

    explicit



plugins/problemreporter/problemreportermodel.h (line 49)
<https://git.reviewboard.kde.org/r/124123/#comment56438>

    remove get prefix, also below. return QVector, not QList. also both functions should be const



plugins/problemreporter/problemreportermodel.h (line 53)
<https://git.reviewboard.kde.org/r/124123/#comment56439>

    take urls by const&



plugins/problemreporter/problemreportermodel.h (line 73)
<https://git.reviewboard.kde.org/r/124123/#comment56440>

    remove get prefix.



plugins/problemreporter/problemreportermodel.cpp (line 45)
<https://git.reviewboard.kde.org/r/124123/#comment56441>

    style:
    
        Foo::Foo(...)
            : Base(...)
            , m_foo(...)
            , ...
        {
            ...
        }



plugins/problemreporter/problemreportermodel.cpp (line 67)
<https://git.reviewboard.kde.org/r/124123/#comment56443>

    style: add a `using namespace KDevelop;` to your .cpp files and remove the `KDevelop::` qualification from the .cpp files then.



plugins/problemreporter/problemreportermodel.cpp (line 81)
<https://git.reviewboard.kde.org/r/124123/#comment56442>

    style: space after foreach, if and similar keywords



plugins/problemreporter/problemreporterplugin.h (line 55)
<https://git.reviewboard.kde.org/r/124123/#comment56444>

    remove get prefix



plugins/problemreporter/problemsview.h (line 71)
<https://git.reviewboard.kde.org/r/124123/#comment56445>

    this class is not exported, why do you pimpl it? useless overhead -> remove



plugins/problemreporter/problemsview.cpp (line 77)
<https://git.reviewboard.kde.org/r/124123/#comment56446>

    style: join { to same line. space after keyworks like while, if, ...



plugins/problemreporter/problemsview.cpp (line 171)
<https://git.reviewboard.kde.org/r/124123/#comment56447>

    join these statements for speed thanks to QStringBuilder:
    
        const QString text = '(' + nameFromLabel(d->tabWidget->tabText(idx)) + ')';



plugins/problemreporter/problemtreeview.h (line 52)
<https://git.reviewboard.kde.org/r/124123/#comment56448>

    why not use `model()->rowCount()` where required?



plugins/problemreporter/problemtreeview.cpp (line 66)
<https://git.reviewboard.kde.org/r/124123/#comment56449>

    style: space after if, join { to same line



plugins/problemreporter/problemtreeview.cpp (line 183)
<https://git.reviewboard.kde.org/r/124123/#comment56450>

    ugly, better do something like this:
    
        for (auto action : {noGroupingAction, pathGroupingAction, severityGroupingAction}) {
            ...
        }



plugins/problemreporter/problemtreeview.cpp (line 208)
<https://git.reviewboard.kde.org/r/124123/#comment56452>

    the slots all just emit changed, remove them and connect directly to the signal here instead



plugins/problemreporter/problemtreeview.cpp (line 229)
<https://git.reviewboard.kde.org/r/124123/#comment56451>

    style:
    
        if (!problem);



plugins/problemreporter/problemtreeview.cpp (line 241)
<https://git.reviewboard.kde.org/r/124123/#comment56453>

    you'll remove this (see above), but in general the style guide would dictate not to ues Q_UPPERCASE keywords, but the lowercase ones, like emit, signal, slot, foreach etc. pp.



plugins/problemreporter/problemtreeview.cpp (line 296)
<https://git.reviewboard.kde.org/r/124123/#comment56454>

    I'd remove this full function, but if you really see yourself calling it so often, then fix the style similar to above:
    
        return model() ? model()->rowCount() : 0;



shell/filteredproblemstore.h (line 35)
<https://git.reviewboard.kde.org/r/124123/#comment56455>

    style: use nullptr instead of NULL



shell/filteredproblemstore.h (line 58)
<https://git.reviewboard.kde.org/r/124123/#comment56456>

    take by const&, and could the function be const as well?



shell/filteredproblemstore.cpp (line 27)
<https://git.reviewboard.kde.org/r/124123/#comment56457>

    wrap in an anonymous namespace



shell/filteredproblemstore.cpp (line 30)
<https://git.reviewboard.kde.org/r/124123/#comment56459>

    style
    
        Foo::Foo()
            : m_asdf(...)
        {
            ...
        }



shell/filteredproblemstore.cpp (line 37)
<https://git.reviewboard.kde.org/r/124123/#comment56458>

    use a QScopedPointer instead



shell/filteredproblemstore.cpp (line 44)
<https://git.reviewboard.kde.org/r/124123/#comment56460>

    here and elsewhere: NULL -> nullptr, but in the conditionals just use the simple `if (parent)` or similar



shell/filteredproblemstore.cpp (line 75)
<https://git.reviewboard.kde.org/r/124123/#comment56463>

    final?



shell/filteredproblemstore.cpp (line 82)
<https://git.reviewboard.kde.org/r/124123/#comment56471>

    = default or remove



shell/filteredproblemstore.cpp (line 85)
<https://git.reviewboard.kde.org/r/124123/#comment56461>

    const&



shell/filteredproblemstore.cpp (line 95)
<https://git.reviewboard.kde.org/r/124123/#comment56464>

    final?



shell/filteredproblemstore.cpp (line 103)
<https://git.reviewboard.kde.org/r/124123/#comment56470>

    = default or remove



shell/filteredproblemstore.cpp (line 107)
<https://git.reviewboard.kde.org/r/124123/#comment56462>

    const&



shell/filteredproblemstore.cpp (line 113)
<https://git.reviewboard.kde.org/r/124123/#comment56465>

    space after foreach/if/... and join with { line



shell/filteredproblemstore.cpp (line 137)
<https://git.reviewboard.kde.org/r/124123/#comment56466>

    final?



shell/filteredproblemstore.cpp (line 151)
<https://git.reviewboard.kde.org/r/124123/#comment56467>

    i18n?



shell/filteredproblemstore.cpp (line 156)
<https://git.reviewboard.kde.org/r/124123/#comment56469>

    = default or remove



shell/filteredproblemstore.cpp (line 160)
<https://git.reviewboard.kde.org/r/124123/#comment56468>

    const ref



shell/filteredproblemstore.cpp (line 187)
<https://git.reviewboard.kde.org/r/124123/#comment56473>

    use direct member initialization



shell/filteredproblemstore.cpp (line 193)
<https://git.reviewboard.kde.org/r/124123/#comment56472>

    QScopedPointer



shell/filteredproblemstore.cpp (line 222)
<https://git.reviewboard.kde.org/r/124123/#comment56474>

    here and elsewhere: emit



shell/filteredproblemstore.cpp (line 248)
<https://git.reviewboard.kde.org/r/124123/#comment56476>

    space after switch, join { line



shell/filteredproblemstore.cpp (line 250)
<https://git.reviewboard.kde.org/r/124123/#comment56475>

    indent



shell/languagecontroller.h (line 62)
<https://git.reviewboard.kde.org/r/124123/#comment56477>

    make function const



shell/languagecontroller.cpp (line 192)
<https://git.reviewboard.kde.org/r/124123/#comment56478>

    { on its own line



shell/problem.h (line 42)
<https://git.reviewboard.kde.org/r/124123/#comment56479>

    don't inline anything in exported classes



shell/problem.h (line 62)
<https://git.reviewboard.kde.org/r/124123/#comment56481>

    use QVector



shell/problem.h (line 71)
<https://git.reviewboard.kde.org/r/124123/#comment56480>

    exported classes must be pimpled



shell/problem.cpp (line 40)
<https://git.reviewboard.kde.org/r/124123/#comment56482>

    i18n



shell/problem.cpp (line 61)
<https://git.reviewboard.kde.org/r/124123/#comment56483>

    i18n



shell/problem.cpp (line 79)
<https://git.reviewboard.kde.org/r/124123/#comment56484>

    const&



shell/problem.cpp (line 89)
<https://git.reviewboard.kde.org/r/124123/#comment56485>

    Q_ASSERT(d);



shell/problemmodel.h (line 44)
<https://git.reviewboard.kde.org/r/124123/#comment56486>

    using Features = unsigned char;
    
    should this contain stuff from the FeatureCode? Then use QFlags instead



shell/problemmodel.h (line 79)
<https://git.reviewboard.kde.org/r/124123/#comment56487>

    const&



shell/problemmodel.h (line 85)
<https://git.reviewboard.kde.org/r/124123/#comment56488>

    don't inline any public code (this is exported)



shell/problemmodel.h (line 132)
<https://git.reviewboard.kde.org/r/124123/#comment56490>

    private



shell/problemmodel.h (line 133)
<https://git.reviewboard.kde.org/r/124123/#comment56489>

    must be pimpled



shell/problemmodel.cpp (line 47)
<https://git.reviewboard.kde.org/r/124123/#comment56491>

    QStringLiteral, also below



shell/problemmodel.cpp (line 61)
<https://git.reviewboard.kde.org/r/124123/#comment56494>

    use direct member initialization



shell/problemmodel.cpp (line 64)
<https://git.reviewboard.kde.org/r/124123/#comment56492>

    style



shell/problemmodel.cpp (line 86)
<https://git.reviewboard.kde.org/r/124123/#comment56493>

    QScopedPointer



shell/problemmodel.cpp (line 97)
<https://git.reviewboard.kde.org/r/124123/#comment56495>

    remove get prefix



shell/problemmodelset.h (line 33)
<https://git.reviewboard.kde.org/r/124123/#comment56497>

    add a Q_DECLARE_TYPEINFO(ModelData, Q_MOVABLE_TYPE).
    
    also, don't export this data-only struct.



shell/problemmodelset.h (line 54)
<https://git.reviewboard.kde.org/r/124123/#comment56496>

    use QVector



shell/problemmodelset.h (line 65)
<https://git.reviewboard.kde.org/r/124123/#comment56498>

    QScopedPointer



shell/problemmodelset.cpp (line 28)
<https://git.reviewboard.kde.org/r/124123/#comment56501>

    QVector



shell/problemmodelset.cpp (line 37)
<https://git.reviewboard.kde.org/r/124123/#comment56499>

    = default;



shell/problemmodelset.cpp (line 43)
<https://git.reviewboard.kde.org/r/124123/#comment56500>

    const& the name



shell/problemmodelset.cpp (line 53)
<https://git.reviewboard.kde.org/r/124123/#comment56504>

    const& the name



shell/problemmodelset.cpp (line 56)
<https://git.reviewboard.kde.org/r/124123/#comment56503>

    join with next line, space after while, if.
    
    also, why not simple:
    
        auto it = find(d->data.begin(), d->data.end(), [name] (const ModelData& data) {
            return data.name == name;
        });
        if (it != d->data.end()) {
            ...
        }



shell/problemmodelset.cpp (line 69)
<https://git.reviewboard.kde.org/r/124123/#comment56502>

    don't return by const&



shell/problemstore.h (line 39)
<https://git.reviewboard.kde.org/r/124123/#comment56506>

    here and elsewhere: no NULL, use nullptr



shell/problemstore.h (line 46)
<https://git.reviewboard.kde.org/r/124123/#comment56505>

    const the problems, make it a QVector



shell/problemstore.h (line 58)
<https://git.reviewboard.kde.org/r/124123/#comment56507>

    must not inline anything in a public class



shell/problemstore.h (line 91)
<https://git.reviewboard.kde.org/r/124123/#comment56508>

    private, and pimpl the data



shell/problemstore.cpp (line 29)
<https://git.reviewboard.kde.org/r/124123/#comment56509>

    direct member initialization



shell/problemstore.cpp (line 38)
<https://git.reviewboard.kde.org/r/124123/#comment56510>

    QScopedPointer



shell/problemstorenode.h (line 121)
<https://git.reviewboard.kde.org/r/124123/#comment56511>

    QVector



shell/problemstorenode.h (line 132)
<https://git.reviewboard.kde.org/r/124123/#comment56512>

    const& the QString, style of the direct member initialization as I mentioned before



shell/problemstorenode.h (line 158)
<https://git.reviewboard.kde.org/r/124123/#comment56513>

    style of members, also const& the problem



shell/problemstorenode.h (line 173)
<https://git.reviewboard.kde.org/r/124123/#comment56514>

    const&


- Milian Wolff


On July 3, 2015, 1:30 p.m., Laszlo Kis-Adam wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124123/
> -----------------------------------------------------------
> 
> (Updated July 3, 2015, 1:30 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> Hi there!
> Here's the bottomline of what I've been doing:
> Please DO note that this is NOT done yet, I'm only submitting it to get some feedback.
> So it should NOT be merged!
> 
> (mostly)GUI changes:
> ====================
> - Moved ProblemModel to shell
> - Reworked the Problems toolview a little. Now it works like this:
> -- ProblemModels are added to ProblemModelSet.
> -- ProblemReporterFactory makes instances of ProblemsView.
> -- ProblemsView takes the models from ProblemModelSet (also subscribes for updates about them, so if one is added or removed it can add/remove their views) and it provides a tabbed widget where the views for them can be added. It creates instances of ProblemTreeView which show the problems in ProblemModel, and adds them to the tabs. Also the tabs shows the number of problems in the ProblemModels.
> -- The toolview will only add actions that are supported by the model (for example: filtering, grouping, reparsing, showing imports. Obviously reparsing doesn't make sense for runtime problem checkers)
> 
> See video:
> https://www.youtube.com/watch?v=K-wcoXcz-GM
> 
> 
> ProblemModel details:
> =====================
> - Broke up ProblemModel into 2 parts
> -- Base ProblemModel that provides the QAbstractItemModel interface for views and can use various ProblemStores to store problems. By default it uses FilteredProblemStore.
> -- ProblemReporterModel is basically the old ProblemModel that grabs problems from DUChain, it's a subclass of ProblemModel.
> - ProblemStore simply stores problems as a list (well technically it stores them in a tree, but it only has 1 level, so it's a list). There's no filtering, no grouping. It's perfect for ProblemReporterModel since it does filtering itself when grabbing the problems from DUChain.
> - FilteredProblemStore DOES filtering, and grouping itself. It stores problems in a tree (ProblemStoreNode subclasses). The tree structure depends on the grouping method, which is implemented with GroupingStrategy subclasses.
> - Moved WatchedDocumentSet and it's subclasses from ProblemModel to ProblemStore, as it is really a detail that the model itself doesn't need, however ProblemStore which stores the problems needs it actually.
> - Created a new Problem class, DetectedProblem. The intent here was to create a class with a clear interface for problems, which ProblemStore can simply store. Then I wanted to eventually replace the "old" Problem class with it. Then I realized that it's not practical because of the "show imports" feature. Which shows the problems from imported contexts. Unfortunately DUChain is the class that knows those, and it's way too much work to get it out from it. Not to mention it doesn't even make sense, since it's really something that logically belongs there. 
> Therefore my current plan now is to just bring them under a common interface so that it will not matter at all. The connection to DUchain will be hidden in the implementation.
> So the work that's left is creating this common interface and then making these two problem classes use it.
> 
> Using this new system is fairly straightforward.
> See the test plugin that I've created to test with:
> https://bitbucket.org/dfighter1985/kdev-problemtest/src
> 
> Especially widget.cpp which instantiates the model, and adds problems, or clears them based on which button is clicked.
> 
> 
> Here's a class diagram about all this:
> http://i.imgur.com/eGWR3sO.jpg
> 
> 
> Diffs
> -----
> 
>   interfaces/CMakeLists.txt 11fa364 
>   interfaces/ilanguagecontroller.h d76284c 
>   interfaces/iproblem.h PRE-CREATION 
>   language/backgroundparser/parsejob.cpp f752207 
>   language/codegen/templateengine_p.h fcb5a06 
>   language/duchain/navigation/problemnavigationcontext.h f19aa18 
>   language/duchain/navigation/problemnavigationcontext.cpp 99e2fd0 
>   language/duchain/navigation/problemnavigationcontext.cpp 99e2fd0 
>   language/duchain/problem.h eb3951a 
>   language/duchain/problem.cpp 8cbf45b 
>   language/duchain/tests/test_duchain.cpp 8d49a24 
>   plugins/problemreporter/CMakeLists.txt 39b3643 
>   plugins/problemreporter/problemhighlighter.h 1df7a79 
>   plugins/problemreporter/problemhighlighter.cpp b4a6ad2 
>   plugins/problemreporter/problemmodel.h 0d7d773 
>   plugins/problemreporter/problemmodel.cpp 9043a24 
>   plugins/problemreporter/problemreportermodel.h PRE-CREATION 
>   plugins/problemreporter/problemreportermodel.cpp PRE-CREATION 
>   plugins/problemreporter/problemreporterplugin.h 5afd8ad 
>   plugins/problemreporter/problemreporterplugin.cpp 5ca0c8d 
>   plugins/problemreporter/problemsview.h PRE-CREATION 
>   plugins/problemreporter/problemsview.cpp PRE-CREATION 
>   plugins/problemreporter/problemtreeview.h b9247d4 
>   plugins/problemreporter/problemtreeview.cpp 1c42034 
>   plugins/problemreporter/watcheddocumentset.h 9dda54d 
>   plugins/problemreporter/watcheddocumentset.cpp 143ee58 
>   shell/CMakeLists.txt cffa161 
>   shell/filteredproblemstore.h PRE-CREATION 
>   shell/filteredproblemstore.cpp PRE-CREATION 
>   shell/languagecontroller.h b9a3d06 
>   shell/languagecontroller.cpp d15a14b 
>   shell/problem.h PRE-CREATION 
>   shell/problem.cpp PRE-CREATION 
>   shell/problemmodel.h PRE-CREATION 
>   shell/problemmodel.cpp PRE-CREATION 
>   shell/problemmodelset.h PRE-CREATION 
>   shell/problemmodelset.cpp PRE-CREATION 
>   shell/problemstore.h PRE-CREATION 
>   shell/problemstore.cpp PRE-CREATION 
>   shell/problemstorenode.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124123/diff/
> 
> 
> Testing
> -------
> 
> See the included video.
> 
> 
> Thanks,
> 
> Laszlo Kis-Adam
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20150706/5b5bc857/attachment-0001.html>

More information about the KDevelop-devel mailing list