bport added a comment.
In D28221#657482 <https://phabricator.kde.org/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 <https://phabricator.kde.org/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
