Review Request 120317: WIP: Config pages without KCMs

Milian Wolff mail at milianw.de
Wed Nov 12 18:03:14 UTC 2014 

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

Ship it!


some more nitpicks from my side and then I hope we can merge this and the kdevelop changset as well. Really good work! Will the other plugins that are not ported yet still work or will this mean that they must be ported ASAP as well? It's not a blocker, just want to ask.

Cheers


interfaces/configpage.h
<https://git.reviewboard.kde.org/r/120317/#comment49137>

    missing dot.
    
    Also add a newline that the default implementation returns zero.



interfaces/configpage.h
<https://git.reviewboard.kde.org/r/120317/#comment49138>

    add: The default implementation always returns nullptr.



interfaces/configpage.h
<https://git.reviewboard.kde.org/r/120317/#comment49139>

    move this block down and merge the two public sections



interfaces/configpage.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49140>

    style:
    
    struct Foo 
    {
        Foo()
            : bar(...)
        {}
    };
    
    and yeah, make this a struct, but don't forget to adapt the forward declaration - otherwise clang complains.



interfaces/configpage.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49141>

    too much indentation



interfaces/configpage.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49143>

    completely remove this branch, you are asserting the validity anyways



interfaces/configpage.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49142>

    merge this with the Q_ASSERT at the top, by making this a Q_ASSERT_X.



interfaces/configpage.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49144>

    same as above - assert the validity and remove the branch



interfaces/configpage.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49145>

    and again.



interfaces/configpage.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49146>

    why is it not allowed to unset the config skeleton?



interfaces/iplugin.h
<https://git.reviewboard.kde.org/r/120317/#comment49147>

    add: The default implementation always returns 0.



interfaces/iplugin.h
<https://git.reviewboard.kde.org/r/120317/#comment49148>

    add: The default implementation always returns nullptr.



interfaces/iplugin.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49149>

    too much indentation (was wrong before)



interfaces/iplugin.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49150>

    was wrong before: too much indentation, also add a newline between "," and "d(..." in the next line



plugins/projectfilter/projectfilterkcm.h
<https://git.reviewboard.kde.org/r/120317/#comment49151>

    mark as override



plugins/projectfilter/projectfilterkcm.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49152>

    here and below: why do you call the default implementations? they don't do anything, do they? don't they even assert?



plugins/projectfilter/projectfilterprovider.h
<https://git.reviewboard.kde.org/r/120317/#comment49153>

    add a newline before



project/projectconfigpage.h
<https://git.reviewboard.kde.org/r/120317/#comment49154>

    style: put { on its own line



shell/configdialog.h
<https://git.reviewboard.kde.org/r/120317/#comment49163>

    I'd love to see some unit tests for this code...



shell/configdialog.h
<https://git.reviewboard.kde.org/r/120317/#comment49157>

    take pages by const& and make it a QVector please



shell/configdialog.h
<https://git.reviewboard.kde.org/r/120317/#comment49155>

    please extend this todo, what do you mean by it?



shell/configdialog.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49162>

    add a using namespace KDevelop; here and remove the KDevelop:: below



shell/configdialog.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49156>

    too much indentation



shell/configdialog.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49158>

    here and below: style: remove ()



shell/configdialog.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49160>

    decrease the indendation depth by doing:
    
        if (!m_currentPageHasChanges) {
            return KMessageBox::Yes;
        }
        
        ...



shell/configdialog.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49159>

    add a Q_ASSERT(before)?



shell/configdialog.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49161>

    why not: for (auto item : m_pages) {



shell/editorconfigpage.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49164>

    again, using namespace KDevelop



shell/editorconfigpage.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49169>

    wrap in anonymous namespace



shell/editorconfigpage.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49165>

    too much indentation



shell/editorconfigpage.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49167>

    remove this->



shell/editorconfigpage.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49166>

    add newline before



shell/editorconfigpage.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49168>

    add newline before, also do so in the other occasions below



shell/projectcontroller.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49170>

    couldn't this lambda just capture proj or options and use that?



shell/projectcontroller.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49171>

    use a QVector



shell/projectcontroller.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49172>

    as I said - yes it is. remove this todo.



shell/settings/uipreferences.h
<https://git.reviewboard.kde.org/r/120317/#comment49176>

    typo: customize



shell/uicontroller.cpp
<https://git.reviewboard.kde.org/r/120317/#comment49173>

    use an initializer list:
    
        QVector<ConfigPage*> configPages = {...};


- Milian Wolff


On Nov. 8, 2014, 8:37 p.m., Alexander Richardson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120317/
> -----------------------------------------------------------
> 
> (Updated Nov. 8, 2014, 8:37 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> Everything seems to work fine with the converted config pages.
> 
> This is a huge diff, should I maybe push my changes to a new branch or to some other repository?
> 
> 
> Diffs
> -----
> 
>   shell/settings/kcm_kdev_uisettings.desktop.cmake 336e233c2cb1570a2d8523680938f1d094a9a915 
>   shell/settings/kcm_kdevsourceformattersettings.desktop.cmake 6f22083488be2d4ad350ab5293a25c56e0797e43 
>   shell/settings/pluginpreferences.h 12f9d233d9a06739ff0e460e5c41ff92f9431af3 
>   shell/settings/pluginpreferences.cpp 49a4c4a744095a1030976e5b93d1d16a54531a6e 
>   shell/settings/projectconfig.kcfgc 31355435eff9d2306ce98219095ecf02283cc2e9 
>   shell/settings/projectpreferences.h 18a5357829a4293ba284868ca6be6c33134ef8da 
>   shell/settings/projectpreferences.cpp 807819f8bdb261a24f79056aa4704fc8c38a2c10 
>   shell/settings/bgpreferences.h e516b6395beda6d964cea72c35e694038b84f71a 
>   shell/settings/bgpreferences.cpp 5f5332855d7d6189899cf4e879dd92e3ef48e188 
>   shell/settings/ccpreferences.h 186b62a09e2fd10a9937689d919d05911e3d77e0 
>   shell/settings/ccpreferences.cpp 0817977482f688d1f12d30f4eaa1e67166b70de0 
>   shell/settings/environmentpreferences.h e7c84f4ec750ace28489829eced5bfc0b9c66718 
>   shell/settings/environmentpreferences.cpp 3fdf16b2035c501d60faf2f9003025fd3b757203 
>   shell/settings/kcm_kdev_bgsettings.desktop.cmake 374e041f35bf49422a76096fa15ced9e6dcef587 
>   shell/settings/kcm_kdev_ccsettings.desktop.cmake 56f8394d7ff93bf755f3771471f9c8528bda9d98 
>   shell/settings/kcm_kdev_envsettings.desktop.cmake 5fe88f5a9ddc90ae8cc6078e5b67b159ede5a2eb 
>   shell/settings/kcm_kdev_pluginsettings.desktop.cmake 10d126ed24adc10d3e6f7183d7b97677c661287e 
>   shell/settings/kcm_kdev_projectsettings.desktop.cmake b1ac9bc4d59a2b3128745ceae79a3b66eda08245 
>   shell/settings/bgconfig.kcfgc 8dfe6e314a14972b725f97d341ce6fa5b2795349 
>   util/CMakeLists.txt d88b7e2a30a11073f690007b8fd2f7f577515416 
>   util/environmentconfigurebutton.h 9a6503801cb9cdb9384bb68ac8cc7cd4559c12d9 
>   util/environmentconfigurebutton.cpp 7ff85aae0cb71254fd7b8420b597edb7af195fed 
>   shell/settings/sourceformattersettings.h e27933c45309ed763db24be1d0ed19d4a183e861 
>   shell/settings/sourceformattersettings.cpp 4908eaad29c75206e2443aff617c897fee5b353f 
>   shell/settings/uipreferences.h 34c46f3902cf06156c7f8279e6672ebb7a06a31e 
>   shell/settings/uipreferences.cpp c8ba26201516241e29f76db4c6e448de787c4704 
>   shell/uicontroller.h 797fa198ed17608e88f2a9e9a03f8072c5ebf45d 
>   shell/uicontroller.cpp 0898d333848117dfe6be9cf9b5d03ce0ad62d5ea 
>   shell/projectcontroller.h 08cb7cbfec32cac918d674ed9a076ce9286ffd30 
>   shell/projectcontroller.cpp d6c3e297f157e10a8efa0cbf67e5c70dc131ac83 
>   shell/settings/CMakeLists.txt adde8f22976732e5e5280cea3289caddd40f8b94 
>   shell/configdialog.h PRE-CREATION 
>   shell/configdialog.cpp PRE-CREATION 
>   shell/editorconfigpage.h PRE-CREATION 
>   shell/editorconfigpage.cpp PRE-CREATION 
>   shell/plugincontroller.cpp 898c2e3aeb9ddca1b5bea65680e8003e242f9c07 
>   project/projectkcmodule.h fe40e9c3ad05963a5c7986110d1d4cb3e3fa5438 
>   shell/CMakeLists.txt d30897867af75bda6054852925ae39f8b08c80b7 
>   plugins/templatemanager/Messages.sh  
>   plugins/templatemanager/kdevtemplatemanager_config.desktop.cmake 018b2789eabe97d8ee9f2286783de92526c8a5ba 
>   plugins/templatemanager/templateconfig.h 2dcab9075c16351fcccdf8725f4c96efd2ab1ca9 
>   plugins/templatemanager/templateconfig.cpp 286bd7ccf20df6075eb915a66f9c5ce40c29b3d2 
>   plugins/templatemanager/templateconfig.ui  
>   plugins/templatemanager/templatepage.h ed8a703e6919146b762d657e35d379c33687c74d 
>   plugins/templatemanager/templatepage.cpp 93cff9a030f8bea233008c36710cd312db02b905 
>   plugins/templatemanager/templatepage.ui  
>   project/CMakeLists.txt 5b71fcc0d85712660b7c195bca6399b9dc392a1e 
>   project/interfaces/iprojectbuilder.h c47573a6867c3f81ced411bd973becc29b0c7b94 
>   project/projectconfigpage.h PRE-CREATION 
>   CMakeLists.txt b749e9deeeae164f92d959194302cf892e42bdc7 
>   interfaces/CMakeLists.txt 6763ce44e2d41380820773ed96f14865264b3ffb 
>   interfaces/configpage.h PRE-CREATION 
>   interfaces/configpage.cpp PRE-CREATION 
>   interfaces/iplugin.h 090f2f3c2cb63050958b77951c225b6e8e9ac50a 
>   interfaces/iplugin.cpp 4511a207b38cb331e86838bf3b3bbe957cb3864a 
>   plugins/CMakeLists.txt 46a0b471e6dd06889ef510a1e7b0c18157345157 
>   plugins/execute/CMakeLists.txt 3bb38a5d0279e21997513df205e170183d343b20 
>   plugins/execute/nativeappconfig.cpp bd8883e8093e5dfa5d563907c4fc20c563e77283 
>   plugins/execute/nativeappconfig.ui c7a3540047acf795ce4bab9212242dff274f83c3 
>   plugins/projectfilter/CMakeLists.txt 7021328470cf1698fff01b2084a7458f5cc4f90c 
>   plugins/projectfilter/kcm_kdevprojectfilter.desktop.cmake 2d728b3fcb6b0cd8b6976771a214f270995bc2b7 
>   plugins/projectfilter/projectfilterkcm.h 01d143469528117589c86853e5847fad6bc2aaaf 
>   plugins/projectfilter/projectfilterkcm.cpp 7eaba23ca09bfedb76fde50a4ff02b28b009c31a 
>   plugins/projectfilter/projectfilterprovider.h cb1fa978e0c40fcf8cf5e3fcaf73294c1b9d314e 
>   plugins/projectfilter/projectfilterprovider.cpp e22198349f02675572b1396e7a02bac1b43d2eb8 
>   plugins/templatemanager/CMakeLists.txt 5d0eec1fd6457aefee776a2bff866cf4d27d08a9 
> 
> Diff: https://git.reviewboard.kde.org/r/120317/diff/
> 
> 
> Testing
> -------
> 
> Dialog is shown correctly, apply, okay work correctly.
> 
> Not sure why the first title is translated, AFAIK I don't have any KF5 translations installed (but maybe it loads it from my KDE4 installation?)
> 
> 
> File Attachments
> ----------------
> 
> kdev_config_new.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/09/22/7e219540-1632-4465-9675-ad24a2fde56c__kdev_config_new.png
> kdev_config_new1.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/09/22/f4f16e26-e6dd-4c98-9969-588342f85f4c__kdev_config_new1.png
> plugin enabled -> entry exists
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/09/25/7130af0f-5e23-45cf-b781-ebe318be8369__kdev_config_new2.png
> plugin unloaded -> entry disappears
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/09/25/dff38c1f-2b3d-4ab7-a97d-adff0a99bb3d__kdev_config_new3.png
> 
> 
> Thanks,
> 
> Alexander Richardson
> 
>

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

More information about the KDevelop-devel mailing list