Review Request 120317: WIP: Config pages without KCMs

Alexander Richardson arichardson.kde at gmail.com
Wed Sep 24 20:34:02 UTC 2014



> On Sept. 24, 2014, 4:26 nachm., Kevin Funk wrote:
> > interfaces/configpage.h, line 97
> > <https://git.reviewboard.kde.org/r/120317/diff/3/?file=314659#file314659line97>
> >
> >     Style: Remove whitespace inside <>

Yeah pretty ugly, somehow KDevelop does that automatically when use the virtual override completion. Didn't notice that occurrence.


> On Sept. 24, 2014, 4:26 nachm., Kevin Funk wrote:
> > interfaces/iplugin.h, line 254
> > <https://git.reviewboard.kde.org/r/120317/diff/3/?file=314661#file314661line254>
> >
> >     That's lacking the 'int configPages() const' method then, right?

It inherits from KTextEditor::Plugin which implements this to returns 0


> On Sept. 24, 2014, 4:26 nachm., Kevin Funk wrote:
> > shell/projectcontroller.cpp, line 159
> > <https://git.reviewboard.kde.org/r/120317/diff/3/?file=314673#file314673line159>
> >
> >     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.

I thought adding 5 parameters everywhere seems a bit much so I added this helper structure. Ideally passing an `IProject*` should be enough


> On Sept. 24, 2014, 4:26 nachm., Kevin Funk wrote:
> > shell/uicontroller.h, line 30
> > <https://git.reviewboard.kde.org/r/120317/diff/3/?file=314678#file314678line30>
> >
> >     Needed?

Leftover from initial attempt


On Sept. 24, 2014, 4:26 nachm., Alexander Richardson wrote:
> > 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).

I agree, tests are definitively needed since this is rather complex code.


- Alexander


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


On Sept. 23, 2014, 3:19 vorm., Alexander Richardson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120317/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2014, 3:19 vorm.)
> 
> 
> 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/2b07197a/attachment-0001.html>


More information about the KDevelop-devel mailing list