> On Aug. 9, 2013, 7: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?
Yes, can you investigate if this could use KConfigSkeleton as a member instead of a base class? - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111908/#review37445 ----------------------------------------------------------- On Aug. 9, 2013, 12: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, 12: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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
