D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport updated this revision to Diff 82612.
bport added a comment.


  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.

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28221?vs=82028&id=82612

REVISION DETAIL
  https://phabricator.kde.org/D28221

AFFECTED FILES
  autotests/kconfigskeletontest.cpp
  autotests/kconfigskeletontest.h
  autotests/kconfigtest.cpp
  autotests/kconfigtest.h
  src/core/kconfigdata.cpp

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport planned changes to this revision.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport added a comment.


  In D28221#657482 , @dfaure wrote:
  
  > 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 :-)
  
  
  
  
  In D28221#664243 , @bport wrote:
  
  > - 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

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport updated this revision to Diff 82028.
bport added a comment.


  - 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

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28221?vs=80624&id=82028

REVISION DETAIL
  https://phabricator.kde.org/D28221

AFFECTED FILES
  autotests/kconfigskeletontest.cpp
  autotests/kconfigskeletontest.h
  autotests/kconfigtest.cpp
  autotests/kconfigtest.h
  src/core/kconfigdata.cpp
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/gui/kwindowconfig.cpp

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[David Faure]

dfaure added a comment.


  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 :-)

INLINE COMMENTS

> kconfigtest.cpp:1991
> +KConfigGroup generalLocal(&local, "General");
> +// Check if we get global and not the default value from cpp 
> (defaultcpp) when reading data from restorerc
> +QCOMPARE(generalLocal.readEntry("testKeyDefault", "defaultcpp"), 
> "configdefault");

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

> kconfigtest.cpp:1998
> +local.reparseConfiguration();
> +// We expect to get configdefault value and configdefault not wrote to 
> local file (file will not exist)
> +QCOMPARE(generalLocal.readEntry("testKeyDefault", "defaultcpp"), 
> "configdefault");

s/wrote/written/

> kconfigtest.cpp:2002
> +
> +// Write a custom value, we expect this value to be wrote to local file
> +generalLocal.writeEntry("testKeyDefault", "custom");

s/wrote/written/

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport added a comment.


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

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport updated this revision to Diff 80624.
bport added a comment.


  Add KConfig unittest

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28221?vs=78862&id=80624

REVISION DETAIL
  https://phabricator.kde.org/D28221

AFFECTED FILES
  autotests/kconfigskeletontest.cpp
  autotests/kconfigskeletontest.h
  autotests/kconfigtest.cpp
  autotests/kconfigtest.h
  src/core/kconfigdata.cpp
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/gui/kwindowconfig.cpp

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport added a comment.


  Ok I will add a kconfig test

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[David Faure]

dfaure added a comment.


  In D28221#643799 , @bport wrote:
  
  > In D28221#641971 , @dfaure wrote:
  >
  > > 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.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport added a comment.


  In D28221#641971 , @dfaure wrote:
  
  > 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

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Kevin Ottens]

ervin added a comment.


  LGTM but @dfaure concerns need to be addressed.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[David Faure]

dfaure added a comment.


  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.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport added a comment.


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

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport updated this revision to Diff 78862.
bport added a comment.


  Take in consideration dfaure and ervin feedback

REPOSITORY
  R237 KConfig

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28221?vs=78292&id=78862

REVISION DETAIL
  https://phabricator.kde.org/D28221

AFFECTED FILES
  autotests/kconfigskeletontest.cpp
  autotests/kconfigskeletontest.h
  src/core/kconfigdata.cpp
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/gui/kwindowconfig.cpp

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.

INLINE COMMENTS

> kconfigskeletontest.cpp:25
>  
> +static inline QString kdeGlobalsPath() {
> +return 
> QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation) + 
> "/kdeglobals";

{ should be on its own line

> kconfigskeletontest.cpp:29
> +
> +static inline QString kdeSystemGlobalsPath() {
> +return 
> QStandardPaths::writableLocation(QStandardPaths::GenericConfigLocation) + 
> "/system.kdeglobals";

ditto

> kconfigskeletontest.cpp:190
> +{
> +// This test ensure we don't override default value when this one came 
> from a file
> +// system.kdeglobals is a global file and can provide default

s/ensure/ensures/

> kconfigskeletontest.cpp:218
> +QCOMPARE(item->value(), 30);
> +}

More comments welcome in that test as well.

> kcoreconfigskeleton.cpp:13
>  #include 
> +#include 
>  

Looks like this include is unused

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport planned changes to this revision.
bport added a comment.


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

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> kconfigskeletontest.cpp:212
> +QVERIFY(glob.sync());
> +glob.reparseConfiguration();
> +auto s2 =  new KConfigSkeleton(QStringLiteral("kconfigskeletontestrc"));

Is this needed? Nothing uses glob after this point.

> kconfigdata.cpp:319
>  //qDebug() << "looking up default entry with key=" << defaultKey;
>  const ConstIterator defaultEntry = constFind(defaultKey);
> +entry->mValue = QByteArray();

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

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28221: Don't write default value to configuration file when default value came from /etc/* file

[Benjamin Port]

bport created this revision.
bport added reviewers: ervin, dfaure, davidedmundson.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
bport requested review of this revision.

REVISION SUMMARY
  This will allow administrator to change default value and be reflected at 
user level
  
  Depends on D28128 .

TEST PLAN
  added an unit test

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D28221

AFFECTED FILES
  autotests/kconfigskeletontest.cpp
  autotests/kconfigskeletontest.h
  src/core/kconfigdata.cpp
  src/core/kcoreconfigskeleton.cpp
  src/core/kcoreconfigskeleton.h
  src/gui/kwindowconfig.cpp

To: bport, ervin, dfaure, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns