Don't write default value to configuration file when default value came from /etc/* file
Needs ReviewPublic

Authored by bport on Mar 23 2020, 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 25504
Build 25522: arc lint + arc unit
bport created this revision.Mar 23 2020, 4:26 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 23 2020, 4:26 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
bport requested review of this revision.Mar 23 2020, 4:26 PM
dfaure requested changes to this revision.Mar 24 2020, 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
214

Is this needed? Nothing uses glob after this point.

src/core/kconfigdata.cpp
316–317

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

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

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

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

{ should be on its own line

29

ditto

192

s/ensure/ensures/

220

More comments welcome in that test as well.

src/core/kcoreconfigskeleton.cpp
13

Looks like this include is unused

bport updated this revision to Diff 78862.Mar 30 2020, 9:09 AM

Take in consideration dfaure and ervin feedback

bport added a comment.Mar 30 2020, 9:14 AM

!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...

I added an unittest to be sure I test the regression (let me know if I don't test what you had in mind). However no code changes are needed because in this case mReference and mDefault are different (mDefault will be equal to the value from system file)

dfaure added a comment.Apr 5 2020, 9:19 AM

I have a hard time accepting that the documentation was wrong -- and if it was, then this commit has to fix it, and port as much of the app code that does exactly this, as possible.

I don't really know what mReference is. What about a test that uses KConfig directly (no skeletons), to test e.g. what kwindowconfig.cpp?
Sorry to be a non-believer, I just sense a very strong risk of regression here, and if not, then a lot of porting effort. Unless there's a good reason for this stuff to be that way, that we still have to find out.

I lack time and concentration right now to dig further, but I wanted to not delay my reply longer.

ervin added a comment.Apr 7 2020, 3:57 PM

LGTM but @dfaure concerns need to be addressed.

bport added a comment.Apr 7 2020, 4:27 PM

I have a hard time accepting that the documentation was wrong -- and if it was, then this commit has to fix it, and port as much of the app code that does exactly this, as possible.

From my POV documentation is not "wrong", but this point of view describe only default from cpp, no default from global file.
The current implementation and I don't know why (perhaps I will need to do some archeology there) don't allow to follow default value if it came from a file and follow changes made in the file over time. Like we do with cpp default.

I don't really know what mReference is. What about a test that uses KConfig directly (no skeletons), to test e.g. what kwindowconfig.cpp?
Sorry to be a non-believer, I just sense a very strong risk of regression here, and if not, then a lot of porting effort. Unless there's a good reason for this stuff to be that way, that we still have to find out.

It's important to consider this kind of stuff. I think we can check to port stuff. However I don't think we can have regression but more incoherence between not yet ported code that will behave like now (don't follow new value in the file providing default) and ported code that will track default if default change in the file.

I lack time and concentration right now to dig further, but I wanted to not delay my reply longer.

No proble, thanks for all your feedback

I have a hard time accepting that the documentation was wrong -- and if it was, then this commit has to fix it, and port as much of the app code that does exactly this, as possible.

From my POV documentation is not "wrong"

I'm talking about this documentation (kconfiggroup.h)

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

You cannot say "this is not wrong" and then port every piece of code that you see(like kwindowconfig.cpp) away from the hasDefault check :-)
Either it's right, or it's wrong, as a general practice for when to call revertToDefault.

but this point of view describe only default from cpp, no default from global file.

Well, we need code that works with both, obviously.

The current implementation and I don't know why (perhaps I will need to do some archeology there) don't allow to follow default value if it came from a file and follow changes made in the file over time. Like we do with cpp default.

I thought the logic was always "if the value matches one coming from a more-global config file, then don't write it".
KConfig can see all layers of files, so it can do that by itself.
What KConfig cannot see (in a writeEntry call) is the cpp-computed-default and that's why this case needs special handling in the app code itself.

AFAICS the heart of the matter is what should happen when both exist.
A C++ default, and a global-file default. If we're saving a value equal to the C++ default, then we have to write it out, otherwise upon reading the global-file default will interfer. That's what the hasDefault() check before revertToDefault() is about, AFAICS. Assuming revertToDefault means "do not write anything in my local config file".

What complicates my ability to review this, is the "mix" between the two layers. Plain KConfig/KConfigGroup, and KConfigSkeletionItem stuff.
Any chance you could add plain-KConfig unittests? This would help making sure that the changes in e.g. kconfigdata.cpp aren't compensated by changes in KConfigSkeletonItem, and that plain KConfig usage works as it should.

bport added a comment.Apr 18 2020, 7:19 PM

Ok I will add a kconfig test

bport updated this revision to Diff 80624.Apr 20 2020, 8:57 AM

Add KConfig unittest

bport added a comment.EditedApr 20 2020, 9:26 AM

Indeed probably need to update documentation too.
Not sure why we didn't allow to track default value from file in the past by the way.

Ah! Now it actually makes sense to me. If we are changing what revertToDefault() does, then it makes sense to change the if() condition for calling it. Basically, now that it does the right thing in both cases (default from C++ and default from system file) it can be called in both cases, while before it was only called if there was no default from a system file. Right?

I'm still wondering about this though: "If we're saving a value equal to the C++ default, then we have to write it out, otherwise upon reading the global-file default will interfer."
Your unittest shows that revertToDefault() will lead to the global-file value being read.
Doesn't this mean that this code is wrong then?

 if (value == "cppdefault") {
    cg.revertToDefault(key);
} else {
    cg.writeEntry(key, value);
}

That's the code you're using everywhere, so that's actually the code that would be good to unittest ;-)
Isn't this buggy when the value actually *is* cppdefault, and there is a system config file with another default value?

Indeed probably need to update configuration too.

Did you mean documentation? I know this is all about configuration ;) but the bit that's missing is to update the documentation, and add a task to get rid of the hasDefault() everywhere before revertToDefault is called. Assuming I'm wrong above about there being a bug left :-)

autotests/kconfigtest.cpp
1991

Bug in the comment, the file isn't called restorerc.

1998

s/wrote/written/

2002

s/wrote/written/

bport updated this revision to Diff 82028.May 5 2020, 8:33 PM
  • Fix comments
  • Ensure we don't have problem if we set value to "defaultcpp" and a global setting override it

Regarding other comments:

  • I guess you mean if (mDefault == mReference) { with if (value == "defaultcpp")

in that case mDefault is equal to either default from cpp value or default from global file.

  • Your first assertion is right.

Still need to fix documentation

bport added a comment.May 5 2020, 8:39 PM

Ah! Now it actually makes sense to me. If we are changing what revertToDefault() does, then it makes sense to change the if() condition for calling it. Basically, now that it does the right thing in both cases (default from C++ and default from system file) it can be called in both cases, while before it was only called if there was no default from a system file. Right?

I'm still wondering about this though: "If we're saving a value equal to the C++ default, then we have to write it out, otherwise upon reading the global-file default will interfer."
Your unittest shows that revertToDefault() will lead to the global-file value being read.
Doesn't this mean that this code is wrong then?

 if (value == "cppdefault") {
    cg.revertToDefault(key);
} else {
    cg.writeEntry(key, value);
}

That's the code you're using everywhere, so that's actually the code that would be good to unittest ;-)
Isn't this buggy when the value actually *is* cppdefault, and there is a system config file with another default value?

Indeed probably need to update configuration too.

Did you mean documentation? I know this is all about configuration ;) but the bit that's missing is to update the documentation, and add a task to get rid of the hasDefault() everywhere before revertToDefault is called. Assuming I'm wrong above about there being a bug left :-)

  • Fix comments
  • Ensure we don't have problem if we set value to "defaultcpp" and a global setting override it

    Regarding other comments:
  • I guess you mean if (mDefault == mReference) { with if (value == "defaultcpp") in that case mDefault is equal to either default from cpp value or default from global file.
  • Your first assertion is right.

    Still need to fix documentation

Indeed there is still some question regarding if (value == "defaultcpp")
kwindowconfig fix seems wrong

bport planned changes to this revision.May 5 2020, 9:56 PM
bport updated this revision to Diff 82612.May 11 2020, 10:47 PM

Simplify the patch and adapt unittest

I expected revertToDefault to revert either to cpp defined default or default from global file, but concept behind is to revert only for cpp based default.