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
Details
Added a test to ensure flag is working
Tested using some KCM to ensure all is working fine
Diff Detail
- Repository
- R237 KConfig
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 23875 Build 23893: arc lint + arc unit
src/core/kconfigdata.cpp | ||
---|---|---|
139 | s/overidded/overridden |
I would prefer a unittest based on public API, showing the actual bug (before the fix) with kdeglobals and a local config file.
src/core/kconfigdata.h | ||
---|---|---|
76 | s/wrote/written/ |
Sounds like you need another feature, not breaking an existing one.
autotests/kconfigtest.cpp | ||
---|---|---|
1965 ↗ | (On Diff #78289) | This would pass no matter in which file the write happened, no, due to caching? |
1970 ↗ | (On Diff #78289) | Hmm, so this is what this is all about? This contradicts the documentation for revertToDefault().
The value in the global config file is 10, that's what this is supposed to revert to. |
autotests/kconfigtest.cpp | ||
---|---|---|
1965 ↗ | (On Diff #78289) | yes indeed |
1970 ↗ | (On Diff #78289) | This is a global local file, not system wide and so not considered as default It will be reverted to the default value specified in the file |
autotests/kconfigtest.cpp | ||
---|---|---|
1970 ↗ | (On Diff #78289) | 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. |
src/core/kconfigini.cpp | ||
445 | Wouldn't it be enough to ensure that bReverted is false? |
- 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
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
src/core/kconfigdata.h | ||
---|---|---|
183 | This enum value isn't set anywhere except in the unittest, no? |
autotests/kconfigtest.cpp | ||
---|---|---|
1953 ↗ | (On Diff #78503) | 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). |
autotests/kconfigtest.h | ||
76 ↗ | (On Diff #78503) | nitpick: should be "Vs" |
src/core/kconfigdata.cpp | ||
118 | Should be at the end of the previous line | |
src/core/kconfigdata.h | ||
76 | nitpick: I think I'd write "non global" rather than "not global" |