Review Request 120317: WIP: Config pages without KCMs

Wed Sep 24 14:26:26 UTC 2014

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

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

    Style: Please add a newline before access keywords (multiple occasions)

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

    Style: Remove whitespace inside <>

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

    Call this KTextEditorConfigPageAdapter, makes the intention of this class more clear.

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

    That's lacking the 'int configPages() const' method then, right?

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

    I don't like that we need to copy data into a helper struct here.

    I see that you needed to do something along this because IProject doesn't give you that info, though.

    I feel like we should expose at least Project::developerFile in IProject. And I wonder if we could get rid off the temp-file logic altogether. If we end up like this we could just pass an `IProject*` to ConfigPage.

    Anyway, that's just some food for thought, and not really part of that patch. We might have a look at that later on, let's keep it like this for now.

shell/uicontroller.h
<https://git.reviewboard.kde.org/r/120317/#comment47017>

    Needed?

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

    I'd factor those lambdas out into slots for code readability. (you're capturing `this` anyway).

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

    You need to remove the item from m_pages again.

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

    Style: Indentation issue

Patch looks good to me. But I'd let Milian or others have another look at it.

I'd also appreciate some tests here that at least take most of the branches in the config page infrastructure (create config page, show the dialog, close again).

- Kevin Funk

On Sept. 23, 2014, 1:19 a.m., Alexander Richardson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120317/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2014, 1:19 a.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> I only converted the UISettings to the new type for now, will convert the rest if this approach seems sensible
> 
> Editor pages are also added to the main config dialog.
> 
> 
> Diffs
> -----
> 
>   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 
>   project/projectkcmodule.h fe40e9c3ad05963a5c7986110d1d4cb3e3fa5438 
>   shell/CMakeLists.txt 7847e261f53e4831f7dd662add34a6e6281d5cd4 
>   shell/projectcontroller.cpp 6c1c3844800d05911003d47c6408ff34d24d5afd 
>   shell/settings/CMakeLists.txt 5760ab4cb6a459ec6f1282ae09bb95f5f6e77892 
>   shell/settings/kcm_kdev_uisettings.desktop.cmake 336e233c2cb1570a2d8523680938f1d094a9a915 
>   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 
>   CMakeLists.txt 7da5c922f54cad7aafc2452f609ff172fc2214b7 
>   interfaces/CMakeLists.txt 87916216011e4390ba9b637f9ae22fe54f254e86 
> 
> Diff: https://git.reviewboard.kde.org/r/120317/diff/
> 
> 
> Testing
> -------
> 
> Dialog is shown correctly, apply, okay work correctly.
> 
> However when I change the dock position it is only changed in the GUI on the first save, later saves are ignored. Will have to investigate what's going wrong there.
> 
> 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
> 
> 
> Thanks,
> 
> Alexander Richardson
> 
>

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