Review Request 111908: Add KConfigLoader from Plasma Framework to KConfigGui

[Martin Gräßlin]

> On Aug. 9, 2013, 9:27 p.m., David Faure wrote:
> > tier1/kconfig/src/gui/kconfigloader.h, line 99
> > <http://git.reviewboard.kde.org/r/111908/diff/4/?file=177609#file177609line99>
> >
> >     I wonder if this should really derive from KConfigSkeleton, rather than encapsulate it.
> >     
> >     For instance it means you can't get a "core only" version of it (see KCoreConfigSkeleton).
> >     
> >     It doesn't reimplement any virtual methods, so this inheritance seems unnecessary. On the other hand I'm not sure how it would work by composition anyway; maybe the caller would have to create it themselves (which would be a bit cumbersome?).
> >     
> >     The use of QColor in the code makes core/gui separation difficult anyway, it would need to be provided by a runtime hook.
> >     
> >     Just an idea.
> >     Don't know how much core/gui separation is useful for this class.
> 
> Martin Gräßlin wrote:
>     anything else I should do here or can I merge the patch in?
> 
> David Faure wrote:
>     Yes, can you investigate if this could use KConfigSkeleton as a member instead of a base class?

I think it's not possible as it needs to implement the protected method KCoreConfigSkeleton::usrWriteConfig(). Only solution would be to have a private extended class and use that as a member. But I think that this wouldn't be worth the effort.


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111908/#review37445
-----------------------------------------------------------


On Aug. 9, 2013, 2:38 p.m., Martin Gräßlin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111908/
> -----------------------------------------------------------
> 
> (Updated Aug. 9, 2013, 2:38 p.m.)
> 
> 
> Review request for KDE Frameworks, Plasma and Aaron J. Seigo.
> 
> 
> Description
> -------
> 
> Add KConfigLoader from Plasma Framework to KConfigGui
> 
> The ConfigLoader is way to awesome to not be directly in KConfig.
> 
> 
> Diffs
> -----
> 
>   tier1/kconfig/autotests/CMakeLists.txt c913da3 
>   tier1/kconfig/autotests/kconfigloadertest.h PRE-CREATION 
>   tier1/kconfig/autotests/kconfigloadertest.cpp PRE-CREATION 
>   tier1/kconfig/autotests/kconfigloadertest.xml PRE-CREATION 
>   tier1/kconfig/src/gui/CMakeLists.txt 0913349 
>   tier1/kconfig/src/gui/kconfigloader.h PRE-CREATION 
>   tier1/kconfig/src/gui/kconfigloader.cpp PRE-CREATION 
>   tier1/kconfig/src/gui/kconfigloader_p.h PRE-CREATION 
>   tier1/kconfig/src/gui/kconfigloaderhandler_p.h PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/111908/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Martin Gräßlin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130912/599fe4e8/attachment.html>