Add force save behavior to KEntryMap
ClosedPublic

Authored by bport on Mar 18 2020, 2:49 PM.

Details

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

Diff Detail

Repository
R237 KConfig
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bport created this revision.Mar 18 2020, 2:49 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 18 2020, 2:49 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
bport requested review of this revision.Mar 18 2020, 2:49 PM
meven added inline comments.Mar 20 2020, 10:54 AM
src/core/kconfigdata.cpp
130

s/overidded/overridden

meven accepted this revision.Mar 20 2020, 10:55 AM
This revision is now accepted and ready to land.Mar 20 2020, 10:55 AM
dfaure requested changes to this revision.Mar 21 2020, 11:23 AM

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
63

s/wrote/written/
s/match/matches/

This revision now requires changes to proceed.Mar 21 2020, 11:23 AM
bport updated this revision to Diff 78289.Mar 23 2020, 2:02 PM

Add unit test

bport marked 2 inline comments as done.Mar 23 2020, 2:03 PM

Added a unittest

dfaure requested changes to this revision.Mar 23 2020, 7:56 PM

Sounds like you need another feature, not breaking an existing one.

autotests/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?

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 revision now requires changes to proceed.Mar 23 2020, 7:56 PM
bport planned changes to this revision.Mar 23 2020, 8:06 PM
bport added inline comments.
autotests/kconfigtest.cpp
1965

yes indeed

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

dfaure added inline comments.Mar 24 2020, 1:04 PM
autotests/kconfigtest.cpp
1970

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
432–435

Wouldn't it be enough to ensure that bReverted is false?
This would make for a much more minimal patch....

bport updated this revision to Diff 78432.Mar 25 2020, 8:55 AM
  • 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
170

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?

bport updated this revision to Diff 78503.Mar 25 2020, 9:35 PM

Take in consideration dfaure feedback.

ervin added inline comments.Mar 27 2020, 4:23 PM
autotests/kconfigtest.cpp
1953

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

nitpick: should be "Vs"

src/core/kconfigdata.cpp
105

Should be at the end of the previous line

src/core/kconfigdata.h
63

nitpick: I think I'd write "non global" rather than "not global"

bport updated this revision to Diff 78861.Mar 30 2020, 9:09 AM
bport marked 3 inline comments as done.

Take in consideration ervin feedback

dfaure accepted this revision.Apr 3 2020, 11:22 PM
This revision is now accepted and ready to land.Apr 3 2020, 11:22 PM
ervin accepted this revision.Apr 7 2020, 3:53 PM
This revision was automatically updated to reflect the committed changes.