Review Request 123562: Add outlineview plugin

[Milian Wolff]

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


awesome! some style nitpicks from my side but overall it looks pretty good already - thanks! this was an often requested feature.


plugins/outlineview/kdevoutlineview.json (line 8)
<https://git.reviewboard.kde.org/r/123562/#comment54510>

    rtrim



plugins/outlineview/outlinemodel.h (line 32)
<https://git.reviewboard.kde.org/r/123562/#comment54511>

    here and everywhere else: put { on its own line



plugins/outlineview/outlinemodel.h (line 52)
<https://git.reviewboard.kde.org/r/123562/#comment54512>

    QVector



plugins/outlineview/outlinemodel.cpp (line 36)
<https://git.reviewboard.kde.org/r/123562/#comment54513>

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



plugins/outlineview/outlinemodel.cpp (line 45)
<https://git.reviewboard.kde.org/r/123562/#comment54516>

    this is actually what should be done, and the documentSaved stuff removed. could you fix this before we merge it?



plugins/outlineview/outlinemodel.cpp (line 54)
<https://git.reviewboard.kde.org/r/123562/#comment54515>

    here and everywhere else: add space after if/while/for/...
    
    if (...)
    
    also, I'd prefer adding braces also for single-line conditionals



plugins/outlineview/outlinemodel.cpp (line 62)
<https://git.reviewboard.kde.org/r/123562/#comment54521>

    just asking: did you test this new model with a ModelTest hooked to it? i.e. add a
    
        #include <tests/modeltest.h>
        ...
        OutlineModel::OutlineModel(...)
        {
            new ModelTest(this);
        }
    
    if this does not lead to assertions, then you implement the model correctly.



plugins/outlineview/outlinemodel.cpp (line 114)
<https://git.reviewboard.kde.org/r/123562/#comment54514>

    here and everywhere else: join line.
    
    if (...) {
      ...
    } else {
      ...
    }



plugins/outlineview/outlinemodel.cpp (line 167)
<https://git.reviewboard.kde.org/r/123562/#comment54520>

    I'd prefer to use scoping here
    
        {
            DUChainReadLocker lock;
            ...
        } //should not be locked anymore when signal is emitted
        m_lastDoc = doc;
        emit endResetModel();



plugins/outlineview/outlinenode.h (line 33)
<https://git.reviewboard.kde.org/r/123562/#comment54518>

    why disable copying? why not make this a simple struct and Q_DECLARE_TYPEINFO(OutlineNode, Q_MOVABLE_TYPE) as well as storing the stuff directly in a QVector? instead of storing pointers to objects in a vector?



plugins/outlineview/outlinenode.h (line 55)
<https://git.reviewboard.kde.org/r/123562/#comment54517>

    QVector



plugins/outlineview/outlinenode.cpp (line 148)
<https://git.reviewboard.kde.org/r/123562/#comment54522>

    here and elsewhere: use foreach, i.e. the lower-case Qt keywords like signals/slots/emit,...



plugins/outlineview/outlineviewplugin.cpp (line 65)
<https://git.reviewboard.kde.org/r/123562/#comment54519>

    remove?


- Milian Wolff


On April 29, 2015, 12:38 p.m., Alex Richardson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123562/
> -----------------------------------------------------------
> 
> (Updated April 29, 2015, 12:38 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> This plugin provides an outline view for the current document. It is
> very similar to the outline view from eclipse and should also work for
> any language.
> 
> I initially wrote this plugin in 2010 and then forgot about it. I've now
> ported it to KF5 and it seems to work without any major issues. The only
> major thing that is missing is that it updates once the document has been
> reparsed and not whenever it is saved.
> 
> 
> Diffs
> -----
> 
>   plugins/CMakeLists.txt 2c004f22976f513eab607762b4e04fc1b5b048fa 
>   plugins/outlineview/CMakeLists.txt PRE-CREATION 
>   plugins/outlineview/debug_outline.h PRE-CREATION 
>   plugins/outlineview/kdevoutlineview.json PRE-CREATION 
>   plugins/outlineview/outlinemodel.h PRE-CREATION 
>   plugins/outlineview/outlinemodel.cpp PRE-CREATION 
>   plugins/outlineview/outlinenode.h PRE-CREATION 
>   plugins/outlineview/outlinenode.cpp PRE-CREATION 
>   plugins/outlineview/outlineproxymodel.h PRE-CREATION 
>   plugins/outlineview/outlineproxymodel.cpp PRE-CREATION 
>   plugins/outlineview/outlineviewplugin.h PRE-CREATION 
>   plugins/outlineview/outlineviewplugin.cpp PRE-CREATION 
>   plugins/outlineview/outlinewidget.h PRE-CREATION 
>   plugins/outlineview/outlinewidget.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/123562/diff/
> 
> 
> Testing
> -------
> 
> works without crashes sofar.
> 
> Is sometimes empty initially, switching to other document and back fixes that
> 
> 
> File Attachments
> ----------------
> 
> outline1.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/04/29/08b90a88-1cbf-4f98-94e8-d7ea1ad02586__outline1.png
> outline2.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2015/04/29/d8ee18c4-495a-44db-a346-803389462c42__outline2.png
> 
> 
> Thanks,
> 
> Alex Richardson
> 
>

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