D28128: Add force save behavior to KEntryMap
This revision was automatically updated to reflect the committed changes. Closed by commit R237:be28e096c533: Add force save behavior to KEntryMap (authored by bport). REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28128?vs=78861=80383 REVISION DETAIL https://phabricator.kde.org/D28128 AFFECTED FILES autotests/kconfigtest.cpp autotests/kconfigtest.h src/core/kconfigdata.cpp src/core/kconfigdata.h src/core/kconfigini.cpp To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
ervin accepted this revision. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D28128 To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
dfaure accepted this revision. This revision is now accepted and ready to land. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D28128 To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
bport updated this revision to Diff 78861. bport marked 3 inline comments as done. bport added a comment. Take in consideration ervin feedback REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28128?vs=78503=78861 REVISION DETAIL https://phabricator.kde.org/D28128 AFFECTED FILES autotests/kconfigtest.cpp autotests/kconfigtest.h src/core/kconfigdata.cpp src/core/kconfigdata.h src/core/kconfigini.cpp To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
ervin added inline comments.
INLINE COMMENTS
> kconfigtest.cpp:1953
> +
> +void KConfigTest::testKdeglobalsVSDefault()
> +{
Seeing how this test confuses everyone (including me and I knew the problem
before hand...), I think it'd benefit greatly from getting comments at the most
important points in its execution (similarly to other tests in that file).
> kconfigtest.h:76
>
> +void testKdeglobalsVSDefault();
> +
nitpick: should be "Vs"
> kconfigdata.cpp:105
> +if (e.bGlobal && !(options & EntryGlobal) && !k.bDefault)
> +{
> +e.bOverridesGlobal = true;
Should be at the end of the previous line
> kconfigdata.h:63
> +/**
> + * Entry will need to be written on a not global file even if it matches
> default value
> + */
nitpick: I think I'd write "non global" rather than "not global"
REPOSITORY
R237 KConfig
REVISION DETAIL
https://phabricator.kde.org/D28128
To: bport, ervin, dfaure, meven, crossi, hchain
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
bport updated this revision to Diff 78503. bport added a comment. Take in consideration dfaure feedback. REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28128?vs=78432=78503 REVISION DETAIL https://phabricator.kde.org/D28128 AFFECTED FILES autotests/kconfigtest.cpp autotests/kconfigtest.h src/core/kconfigdata.cpp src/core/kconfigdata.h src/core/kconfigini.cpp To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
dfaure added a comment.
I did some debugging.
e.bForceSave is set to true at the time of doing the
writeEntry("testRestore", "restore") -- or if I split this and create another
KConfig instance (I thought this would fail...) it is then set while loading
the file (good).
The naming of that bool sounds weird though, because it sounds like it might
force saving in cases where we don't want that.
But in fact this bool is only used when reverting. So it should be renamed to
something like bRevertShouldSave or bSaveWhenReverting. It has no effect on
"normal saving".
Or maybe it should be called based on what we know at the time of parsing,
rather than on what it's going to be used for. Something like bOverridesGlobal?
Sorry to nitpick on naming, but I think the patch would make a lot more
sense, and the next reader would be much less confused by all this.
Also, what happens if both kdeglobals and the local file have the same value?
Should there be a e.mValue != value check after `e = *it`? In fact, the new
code could move there since this can only happen if we *found* an existing
entry, no? The test passes for me with http://www.davidfaure.fr/2020/moved.diff
INLINE COMMENTS
> kconfigdata.h:170
> EntryNotify = 128,
> +EntryForceSave = 256,
> EntryDefault = (SearchDefaults << 16),
This enum value isn't set anywhere except in the unittest, no?
I guess it's here for consistency, but it still seems pretty useless?
REPOSITORY
R237 KConfig
REVISION DETAIL
https://phabricator.kde.org/D28128
To: bport, ervin, dfaure, meven, crossi, hchain
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
bport updated this revision to Diff 78432. bport added a comment. - fix when entry is a string (entry=) lead to empty string not fallback to default value - can't find a way to use bReverted / bDeleted without bForceSave because bReverted is set when we call revertEntry and at this time we don't know if we override the user kdeglobals REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28128?vs=78289=78432 REVISION DETAIL https://phabricator.kde.org/D28128 AFFECTED FILES autotests/kconfigtest.cpp autotests/kconfigtest.h autotests/kentrymaptest.cpp autotests/kentrymaptest.h src/core/kconfigdata.cpp src/core/kconfigdata.h src/core/kconfigini.cpp To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
dfaure added inline comments.
INLINE COMMENTS
> bport wrote in kconfigtest.cpp:1970
> This is a global local file, not system wide and so not considered as default
> cf. https://lxr.kde.org/source/frameworks/kconfig/src/core/kconfig.cpp#0702
> if the entry is set system wide
> /etc/kde5rc
> /etc/xdg/system.kdeglobals
> /etc/xdg/kdeglobals
> /etc/xdg/yourcustomfile
> ~/.config/system.kdeglobals
>
> It will be reverted to the default value specified in the file
Oh I see.
~/.config/kdeglobals is still stuff that was set by the user, as opposed to
system-wide settings that the user cannot modify.
> kconfigini.cpp:432
> if (it->bGlobal == bGlobal) {
> -if (it->bReverted) {
> +if (it->bReverted && !it->bForceSave) {
> writeMap.remove(key);
Wouldn't it be enough to ensure that bReverted is false?
This would make for a much more minimal patch
REPOSITORY
R237 KConfig
REVISION DETAIL
https://phabricator.kde.org/D28128
To: bport, ervin, dfaure, meven, crossi, hchain
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
bport planned changes to this revision. bport added inline comments. INLINE COMMENTS > dfaure wrote in kconfigtest.cpp:1965 > This would pass no matter in which file the write happened, no, due to > caching? > Doesn't this need a generalLocal.reparseConfiguration() to be meaningful? yes indeed > dfaure wrote in kconfigtest.cpp:1970 > Hmm, so this is what this is all about? > > This contradicts the documentation for revertToDefault(). > > - Reverts the entry with key @p key in the current group in the > - application specific config file to either the system wide (default) > - value or the value specified in the global KDE config file. > > The value in the global config file is 10, that's what this is supposed to > revert to. This is a global local file, not system wide and so not considered as default cf. https://lxr.kde.org/source/frameworks/kconfig/src/core/kconfig.cpp#0702 if the entry is set system wide /etc/kde5rc /etc/xdg/system.kdeglobals /etc/xdg/kdeglobals /etc/xdg/yourcustomfile ~/.config/system.kdeglobals It will be reverted to the default value specified in the file REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D28128 To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Sounds like you need another feature, not breaking an existing one.
INLINE COMMENTS
> kconfigtest.cpp:1965
> +QVERIFY(local.sync());
> +QCOMPARE(generalLocal.readEntry("testRestore", 1), 20);
> +
This would pass no matter in which file the write happened, no, due to caching?
Doesn't this need a generalLocal.reparseConfiguration() to be meaningful?
> kconfigtest.cpp:1970
> +local.reparseConfiguration();
> +QCOMPARE(generalLocal.readEntry("testRestore", 1), 1);
> +}
Hmm, so this is what this is all about?
This contradicts the documentation for revertToDefault().
- Reverts the entry with key @p key in the current group in the
- application specific config file to either the system wide (default)
- value or the value specified in the global KDE config file.
The value in the global config file is 10, that's what this is supposed to
revert to.
REPOSITORY
R237 KConfig
REVISION DETAIL
https://phabricator.kde.org/D28128
To: bport, ervin, dfaure, meven, crossi, hchain
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
bport added a dependent revision: D28221: Don't write default value to configuration file when default value came from /etc/* file. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D28128 To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
bport marked 2 inline comments as done. bport added a comment. Added a unittest REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D28128 To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
bport updated this revision to Diff 78289. bport added a comment. Add unit test REPOSITORY R237 KConfig CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28128?vs=77925=78289 REVISION DETAIL https://phabricator.kde.org/D28128 AFFECTED FILES autotests/kconfigtest.cpp autotests/kconfigtest.h autotests/kentrymaptest.cpp autotests/kentrymaptest.h src/core/kconfigdata.cpp src/core/kconfigdata.h src/core/kconfigini.cpp To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. I would prefer a unittest based on public API, showing the actual bug (before the fix) with kdeglobals and a local config file. INLINE COMMENTS > kconfigdata.h:76 > +/** > + * Entry will need to be wrote on a not global file even if it match > default value > + */ s/wrote/written/ s/match/matches/ REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D28128 To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
meven accepted this revision. This revision is now accepted and ready to land. REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D28128 To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
meven added inline comments. INLINE COMMENTS > kconfigdata.cpp:139 > > +// If overidded entry is global and not default. And it's overrided by a > non global > +if (e.bGlobal && !(options & EntryGlobal) && !k.bDefault) s/overidded/overridden REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D28128 To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28128: Add force save behavior to KEntryMap
bport created this revision. bport added reviewers: ervin, dfaure, meven, crossi, hchain. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. bport requested review of this revision. REVISION SUMMARY Fix the following bug, if an entry is set on HOME/.config/kdeglobals and on a specific config file like kcmfonts When you hit restore defaults button and apply, value will be not wrote on the specific file, but then the value is the one from kdeglobals This patch ensure we write the key to the specific configuration file on those case with an empty value. KConfig will take default value automatically TEST PLAN Added a test to ensure flag is working Tested using some KCM to ensure all is working fine REPOSITORY R237 KConfig REVISION DETAIL https://phabricator.kde.org/D28128 AFFECTED FILES autotests/kentrymaptest.cpp autotests/kentrymaptest.h src/core/kconfigdata.cpp src/core/kconfigdata.h src/core/kconfigini.cpp To: bport, ervin, dfaure, meven, crossi, hchain Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
