D29347: KAuthorized: export method to reload restrictions
dfaure added a comment. Good idea, done in https://commits.kde.org/kconfig/8e0f84030cc1d1e517ca75311ed9850ce88fac41 REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D29347 To: dfaure, aacid, apol, mdawson Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29347: KAuthorized: export method to reload restrictions
mdawson added a comment.
One suggestion for this change:
Instead of exporting a method that takes no parameters and always loads from
configuration file, why not make a new method with the implementation that
takes in a given KConfigGroup. That way unit tests can pass in a KConfigGroup
setup appropriately without having to create a normal configuration file in the
user's home folder (ie use a temporary file). They can also configure the
KConfig to not cascade/use the global configuration so they are isolated from
the environment.
initUrlActionRestrictions can then remain the same, and pass in the
KConfigGroup that would have been used previously.
It would look something like:
KCONFIGCORE_EXPORT void loadUrlActionRestrictions(KConfigGroup cg)
{
// Code as before, without creating the KConfigGroup
}
void initUrlActionRestrictions()
{
KConfigGroup cg(KSharedConfig::openConfig(), "KDE URL Restrictions");
loadUrlActionRestrictions(cg);
}
REPOSITORY
R237 KConfig
REVISION DETAIL
https://phabricator.kde.org/D29347
To: dfaure, aacid, apol, mdawson
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29347: KAuthorized: export method to reload restrictions
dfaure added inline comments. INLINE COMMENTS > kossebau wrote in kauthorized.cpp:247 > Please also add a note next to this that it is for unit test, so people are > not wondering about this random KCONFIGCORE_EXPORT and first have to research > commit history. Good idea, done: https://commits.kde.org/kconfig/311e30857e33f9222b28b6d3b841ab6e61558237 REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D29347 To: dfaure, aacid, apol, mdawson Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29347: KAuthorized: export method to reload restrictions
kossebau added inline comments.
INLINE COMMENTS
> kauthorized.cpp:247
>
> -static void initUrlActionRestrictions()
> +KCONFIGCORE_EXPORT void reloadUrlActionRestrictions()
> {
Please also add a note next to this that it is for unit test, so people are not
wondering about this random KCONFIGCORE_EXPORT and first have to research
commit history.
REPOSITORY
R237 KConfig
REVISION DETAIL
https://phabricator.kde.org/D29347
To: dfaure, aacid, apol, mdawson
Cc: kossebau, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29347: KAuthorized: export method to reload restrictions
dfaure closed this revision. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D29347 To: dfaure, aacid, apol, mdawson Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29347: KAuthorized: export method to reload restrictions
aacid accepted this revision. aacid added a comment. This revision is now accepted and ready to land. a, for using in a different repo, ok. REPOSITORY R237 KConfig BRANCH master REVISION DETAIL https://phabricator.kde.org/D29347 To: dfaure, aacid, apol, mdawson Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29347: KAuthorized: export method to reload restrictions
dfaure added a comment. Yes declaring the function there, as in the code sample shown here. We do the same for internally-exported variables like KSERVICE_EXPORT int ksycoca_ms_between_checks; Qt does the same kind of things. A _p.h header would have to be installed, which we don't normally do. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D29347 To: dfaure, aacid, apol, mdawson Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29347: KAuthorized: export method to reload restrictions
dfaure edited the summary of this revision. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D29347 To: dfaure, aacid, apol, mdawson Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29347: KAuthorized: export method to reload restrictions
aacid added a comment. So how you're going to use it from unittests? declaring the function there? maybe better to declare it to some of the _p.h headers ? REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D29347 To: dfaure, aacid, apol, mdawson Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D29347: KAuthorized: export method to reload restrictions
dfaure created this revision.
dfaure added reviewers: aacid, apol, mdawson.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.
REVISION SUMMARY
This is useful for unittests. Example:
KCONFIGCORE_EXPORT void reloadUrlActionRestrictions();
void someTestMethod()
{
KConfigGroup cg(KSharedConfig::openConfig(), "KDE URL Restrictions");
cg.writeEntry("rule_count", 1);
cg.writeEntry("rule_1", QStringList{"open", {}, {}, {}, "file", "", "",
"false"});
cg.sync();
reloadUrlActionRestrictions();
// Some test for code that uses KUrlAuthorized
cg.deleteEntry("rule_1");
cg.deleteEntry("rule_count");
cg.sync();
reloadUrlActionRestrictions();
}
This is consistent with the fact that other functions used by
KUrlAuthorized are defined here and exported internally.
TEST PLAN
Used this in a KIO unittest I'm writing for the future OpenUrlJob
REPOSITORY
R237 KConfig
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D29347
AFFECTED FILES
src/core/kauthorized.cpp
To: dfaure, aacid, apol, mdawson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
