dfaure added a comment.
In D28221#643799 <https://phabricator.kde.org/D28221#643799>, @bport wrote: > In D28221#641971 <https://phabricator.kde.org/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
