Don't write default value to configuration file when default value came from /etc/* file
Changes PlannedPublic

Authored by bport on Mon, Mar 23, 4:26 PM.

Details

Summary

This will allow administrator to change default value and be reflected at user level

Depends on D28128.

Test Plan

added an unit test

Diff Detail

Repository
R237 KConfig
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 24108
Build 24126: arc lint + arc unit
bport created this revision.Mon, Mar 23, 4:26 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMon, Mar 23, 4:26 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
bport requested review of this revision.Mon, Mar 23, 4:26 PM
dfaure requested changes to this revision.Tue, Mar 24, 7:57 PM

Note that the docu for KConfigGroup::hasDefault has this logic too:

if ( (value == computedDefault) && !group.hasDefault(key) )
   group.revertToDefault(key);
else
   group.writeEntry(key, value);

With the reasoning that we want to avoid writing out the value if the value is equal to computedDefault, UNLESS there's a system file that says otherwise.
Your change seems to break this.

I see the overall setup as this, ordered by priority:

[C++ computed default] < [system config files] < [user kdeglobals] < [user app-specific config file]

!hasDefault() checks [system config files] and therefore should stay. Otherwise, when the situation is C++=1 system=2 and value to be written is 1 you'll revert() i.e. not write anything and then re-read 2, oops.
Sounds like you should add a unittest for this case, to detect this regression...

autotests/kconfigskeletontest.cpp
212

Is this needed? Nothing uses glob after this point.

src/core/kconfigdata.cpp
319

This means defaultEntry is now unused, and therefore defaultKey too.

This revision now requires changes to proceed.Tue, Mar 24, 7:57 PM
bport planned changes to this revision.Wed, Mar 25, 8:28 AM

Change will be planed (firstly fix https://phabricator.kde.org/D28128)

ervin requested changes to this revision.Fri, Mar 27, 5:15 PM
ervin added inline comments.
autotests/kconfigskeletontest.cpp
25

{ should be on its own line

29

ditto

190

s/ensure/ensures/

218

More comments welcome in that test as well.

src/core/kcoreconfigskeleton.cpp
13

Looks like this include is unused