Review Request 120317: WIP: Config pages without KCMs

Milian Wolff mail at milianw.de
Thu Oct 9 12:05:11 UTC 2014 

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


Wow, this looks - again - very very nice! Thanks for taking care of this. Below you'll find some nitpicks from my side and some open questions I have. I'd like to see these answered, but overall this will be able to be merged one way or another.

Thanks again!


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

    do we need that list or can we stick to the way KTextEditor is doing it?



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

    please de-inline this



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

    please pimpl this class (it's exported after all).



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

    this and the one below - do we really need to export these two classes? do they need to be public API? could we maybe move them to the place where we actually need them?



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

    these debug statements can probably be removed now, no? otherwise please use qCDebug



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

    maybe store a IProject here instead, would also simplify things below.



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

    +1 very nice!



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

    please break this line up
    
        foo()
          : base()
          , bla(...)
        {
        }



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

    what is that doing? it looks quite strange. maybe we could invert the ownership?



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

    just remove this if its obsoleted



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

    again, just remove old code please



util/configdialog.h
<https://git.reviewboard.kde.org/r/120317/#comment47480>

    can you add some more explanation here on why this class is required? this is all new code right, why did we not need it before? could we maybe reuse some code from elsewhere? could we share maybe code with kate somehow?



util/configdialog.h
<https://git.reviewboard.kde.org/r/120317/#comment47477>

    see below, once you pimpl this, move these private methods into the private class



util/configdialog.h
<https://git.reviewboard.kde.org/r/120317/#comment47476>

    please pimpl public exported classes



util/configdialog.cpp
<https://git.reviewboard.kde.org/r/120317/#comment47478>

    either remove or use qCDebug, also below



util/configdialog.cpp
<https://git.reviewboard.kde.org/r/120317/#comment47479>

    why this? is this reproducing some old behavior or is this new?


- Milian Wolff


On Sept. 25, 2014, 12:31 a.m., Alexander Richardson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120317/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2014, 12:31 a.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
> -----
> 
>   project/projectkcmodule.h fe40e9c3ad05963a5c7986110d1d4cb3e3fa5438 
>   shell/CMakeLists.txt 7847e261f53e4831f7dd662add34a6e6281d5cd4 
>   shell/plugincontroller.cpp 9872b4631fdd6c5e586c97ccee598066c0e2deb0 
>   shell/projectcontroller.h 08cb7cbfec32cac918d674ed9a076ce9286ffd30 
>   shell/projectcontroller.cpp 6c1c3844800d05911003d47c6408ff34d24d5afd 
>   shell/settings/CMakeLists.txt 5760ab4cb6a459ec6f1282ae09bb95f5f6e77892 
>   shell/settings/kcm_kdev_pluginsettings.desktop.cmake 10d126ed24adc10d3e6f7183d7b97677c661287e 
>   shell/settings/kcm_kdev_uisettings.desktop.cmake 336e233c2cb1570a2d8523680938f1d094a9a915 
>   shell/settings/pluginpreferences.h 12f9d233d9a06739ff0e460e5c41ff92f9431af3 
>   CMakeLists.txt 7da5c922f54cad7aafc2452f609ff172fc2214b7 
>   interfaces/CMakeLists.txt 87916216011e4390ba9b637f9ae22fe54f254e86 
>   interfaces/configpage.h PRE-CREATION 
>   interfaces/configpage.cpp PRE-CREATION 
>   interfaces/iplugin.h b101ae46fa430101c21a1c8eb8699674cbb1977b 
>   interfaces/iplugin.cpp 1a0677312485ae47747cc3483ce325fe9890af78 
>   plugins/projectfilter/CMakeLists.txt 7fc4226bc253e033633916d058f0fc32a1f4cb6f 
>   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 
>   project/CMakeLists.txt e484a3c3370db2c5a9f600c3af5f0ef9e0ccb39d 
>   project/projectconfigpage.h PRE-CREATION 
>   shell/settings/pluginpreferences.cpp 776df6360acec177aa6338058ede61ba80fc0b15 
>   shell/settings/uipreferences.h 34c46f3902cf06156c7f8279e6672ebb7a06a31e 
>   shell/settings/uipreferences.cpp c8ba26201516241e29f76db4c6e448de787c4704 
>   shell/uicontroller.h 797fa198ed17608e88f2a9e9a03f8072c5ebf45d 
>   shell/uicontroller.cpp 0eab47ae515f7832ea8b1833252346cec84c8254 
>   util/CMakeLists.txt 96b74b1a22f386f8dc120cebd5924eb37559e47b 
>   util/configdialog.h PRE-CREATION 
>   util/configdialog.cpp PRE-CREATION 
> 
> 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/20141009/9a28f16f/attachment-0001.html>

More information about the KDevelop-devel mailing list