D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton
This revision was automatically updated to reflect the committed changes. Closed by commit R237:4c3d37519684: Add convenience for defaults/dirty states to KCoreConfigSkeleton (authored by ervin). REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24494?vs=67597&id=67692 REVISION DETAIL https://phabricator.kde.org/D24494 AFFECTED FILES autotests/kconfigskeletontest.cpp src/core/kcoreconfigskeleton.cpp src/core/kcoreconfigskeleton.h src/core/kcoreconfigskeleton_p.h To: ervin, #plasma, #frameworks, dfaure, mart, davidedmundson, apol Cc: apol, kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton
apol accepted this revision. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D24494 To: ervin, #plasma, #frameworks, dfaure, mart, davidedmundson, apol Cc: apol, kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton
davidedmundson accepted this revision. This revision is now accepted and ready to land. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D24494 To: ervin, #plasma, #frameworks, dfaure, mart, davidedmundson Cc: apol, kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton
ervin updated this revision to Diff 67597. ervin added a comment. Add the KF6 comment REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24494?vs=67522&id=67597 REVISION DETAIL https://phabricator.kde.org/D24494 AFFECTED FILES autotests/kconfigskeletontest.cpp src/core/kcoreconfigskeleton.cpp src/core/kcoreconfigskeleton.h src/core/kcoreconfigskeleton_p.h To: ervin, #plasma, #frameworks, dfaure, mart Cc: apol, kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton
apol added a comment. Looks better, it could make sense to add a KF6 TODO/Warning. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D24494 To: ervin, #plasma, #frameworks, dfaure, mart Cc: apol, kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton
ervin added a comment. Any chance to get another round of reviews now that this patch changed quite a bit? REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D24494 To: ervin, #plasma, #frameworks, dfaure, mart Cc: kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton
ervin added inline comments.
INLINE COMMENTS
> davidedmundson wrote in kcoreconfigskeleton.cpp:140
> Do we need to make this
>
> if (d->mIsDefaultImpl){
> return d->mIsDefaultImpl();
> }
> return false;
>
> and initialize mIsDefaultImpl to nullptr
>
> so that it doesn't crash if someone subclasses KConfigSkeletonItem directly
> and doesn't implement this?
Initializing to nullptr wouldn't help, you would get the same behavior than
with the default constructor.
It means it doesn't crash but raises a std::bad_function_call exception, which
I think is fine... it's the next best thing to a pure virtual, but it's caught
at runtime. I don't think we can do better than this in the current situation.
> davidedmundson wrote in kcoreconfigskeleton.h:218
> Is this an ABI break?
>
> :/
*gasp* you're right, how stupid of me... we can't merge that...
REPOSITORY
R237 KConfig
REVISION DETAIL
https://phabricator.kde.org/D24494
To: ervin, #plasma, #frameworks, dfaure, mart
Cc: kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh,
ngraham, bruns
D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton
ervin updated this revision to Diff 67522. ervin added a comment. 5.63 -> 5.64 REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24494?vs=67507&id=67522 REVISION DETAIL https://phabricator.kde.org/D24494 AFFECTED FILES autotests/kconfigskeletontest.cpp src/core/kcoreconfigskeleton.cpp src/core/kcoreconfigskeleton.h src/core/kcoreconfigskeleton_p.h To: ervin, #plasma, #frameworks, dfaure, mart Cc: kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton
kossebau added inline comments. INLINE COMMENTS > kcoreconfigskeleton.h:216 > + * > + * @since 5.63 > + */ -> 5.64 REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D24494 To: ervin, #plasma, #frameworks, dfaure, mart Cc: kossebau, davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton
davidedmundson added a comment.
Looks good to me
INLINE COMMENTS
> kcoreconfigskeleton.cpp:140
> +{
> +return d->mIsDefaultImpl();
> +}
Do we need to make this
if (d->mIsDefaultImpl){
return d->mIsDefaultImpl();
}
return false;
and initialize mIsDefaultImpl to nullptr
so that it doesn't crash if someone subclasses KConfigSkeletonItem directly and
doesn't implement this?
REPOSITORY
R237 KConfig
REVISION DETAIL
https://phabricator.kde.org/D24494
To: ervin, #plasma, #frameworks, dfaure, mart
Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham,
bruns
D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton
ervin updated this revision to Diff 67507. ervin added a comment. Second try, without breaking ABI... it's the best workaround I found in the current situation, I admit I died a bit inside. REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24494?vs=67496&id=67507 REVISION DETAIL https://phabricator.kde.org/D24494 AFFECTED FILES autotests/kconfigskeletontest.cpp src/core/kcoreconfigskeleton.cpp src/core/kcoreconfigskeleton.h src/core/kcoreconfigskeleton_p.h To: ervin, #plasma, #frameworks, dfaure, mart Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton
davidedmundson added a comment. In principle + INLINE COMMENTS > kcoreconfigskeleton.h:218 > + */ > +virtual bool isDefault() const = 0; > + Is this an ABI break? :/ REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D24494 To: ervin, #plasma, #frameworks, dfaure, mart Cc: davidedmundson, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D24494: Add convenience for defaults/dirty states to KCoreConfigSkeleton
ervin created this revision. ervin added reviewers: Plasma, Frameworks, dfaure, mart. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. ervin requested review of this revision. REVISION SUMMARY It allows to verify if all the items of the skeleton are in their default values or if they hold any value deviating from the latest loaded values from KConfig. We didn't really need this during the KCModule/QtWidgets time since we could write KConfigDialogManager just fine without it. But for use with QML and aiming at having similar magic in KQuickAddons::ConfigModule such convenience functions will be needed. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D24494 AFFECTED FILES autotests/kconfigskeletontest.cpp src/core/kcoreconfigskeleton.cpp src/core/kcoreconfigskeleton.h To: ervin, #plasma, #frameworks, dfaure, mart Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
