Re: Review Request 111908: Add KConfigLoader from Plasma Framework to KConfigGui
On Aug. 6, 2013, 4:52 p.m., David Edmundson wrote: tier1/kconfig/src/gui/kconfigloader.h, line 112 http://git.reviewboard.kde.org/r/111908/diff/1/?file=176360#file176360line112 This looks like it should be const I suspect it wasn't because KCoreConfigSeleton::findItem was not const in KDE4, but it appears to be now. Martin Gräßlin wrote: please note that I just moved the code over and changed the name from Plasma::ConfigLoader to KConfigLoader. I did not change the code and I think it would be wrong to change the code in the same step. Because of that I drop the three issues, though it would probably make sense to do these changes after this review is merged. Aleix Pol Gonzalez wrote: Make you can add a comment or warning? Albert Astals Cid wrote: What's the point of this review request if you're not going to accept any review? IOW what are you expecting people to say in this review request? Martin Gräßlin wrote: IOW what are you expecting people to say in this review request? That it's OK to move the code to this location. or other put: I'm not willing to invest time into doing the changes before I know that it will overall be accepted. Because if it doesn't get accepted the changes David requested should be done to the code in Plasma Frameworks. - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111908/#review37219 --- On Aug. 6, 2013, 2:25 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. 6, 2013, 2:25 p.m.) Review request for KDE Frameworks and Plasma. 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 PRE-CREATION 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 PRE-CREATION 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
Re: Review Request 111908: Add KConfigLoader from Plasma Framework to KConfigGui
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111908/#review37284 --- tier1/kconfig/autotests/kconfigloadertest.cpp http://git.reviewboard.kde.org/r/111908/#comment27591 I have trouble understanding the purpose of this class. How is this different from QCOMPARE(configGroup.readEntry(DefaultBoolItem, true), true); ? OK the one difference is that the default value comes from the XML file instead of coming from the code, but apart from that? KConfigXT's entire purpose was to make things statically checked (compile-time), on top of the dynamic (string-based) KConfig. And now this is another layer on top, which makes things dynamic (string-based) again? I'm confused :-) Ah, is this actually only about introspecting KConfigXT xml files, to extract the defaults from it? But what would be the purpose of that? (isn't this accessible in the KConfigXT-generated code too?) Please expand the class documentation to make it clear for dummies like me, what is the actual purpose of the class, and in which case it should be used. - David Faure On Aug. 6, 2013, 12:25 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. 6, 2013, 12:25 p.m.) Review request for KDE Frameworks and Plasma. 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 PRE-CREATION 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 PRE-CREATION 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
Re: Review Request 111908: Add KConfigLoader from Plasma Framework to KConfigGui
On Aug. 7, 2013, 5:41 p.m., David Faure wrote: tier1/kconfig/autotests/kconfigloadertest.cpp, line 56 http://git.reviewboard.kde.org/r/111908/diff/1/?file=176357#file176357line56 I have trouble understanding the purpose of this class. How is this different from QCOMPARE(configGroup.readEntry(DefaultBoolItem, true), true); ? OK the one difference is that the default value comes from the XML file instead of coming from the code, but apart from that? KConfigXT's entire purpose was to make things statically checked (compile-time), on top of the dynamic (string-based) KConfig. And now this is another layer on top, which makes things dynamic (string-based) again? I'm confused :-) Ah, is this actually only about introspecting KConfigXT xml files, to extract the defaults from it? But what would be the purpose of that? (isn't this accessible in the KConfigXT-generated code too?) Please expand the class documentation to make it clear for dummies like me, what is the actual purpose of the class, and in which case it should be used. I think it's best explained to think of cases where you don't have any code in the first place. Examples are plasmoids or KWin scripts which just ship a kconfigxt file and a ui file and with the help of the KConfigLoader we are able to provide a working config interface dynamically loaded. @Aaron: do you have a suggestion on how to improve the documentation? - Martin --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111908/#review37284 --- On Aug. 6, 2013, 2:25 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. 6, 2013, 2:25 p.m.) Review request for KDE Frameworks and Plasma. 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 PRE-CREATION 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 PRE-CREATION 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
Re: Review Request 111908: Add KConfigLoader from Plasma Framework to KConfigGui
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/111908/ --- (Updated Aug. 8, 2013, 6:58 a.m.) Review request for KDE Frameworks, Plasma and Aaron J. Seigo. Changes --- Added Aaron to get some feedback 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 PRE-CREATION 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 PRE-CREATION 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
