Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between

[Albert Astals Cid]

> On gen. 4, 2015, 5:02 p.m., David Faure wrote:
> > This undoes 4846b50aea0bc2262238963a85ab3556c22412e4 (https://git.reviewboard.kde.org/r/117010/), basically.
> > 
> > However looking back at the discussions with Alexander Richardson, this might be only a revert of the part that went further than what *I* was asking for ;) My problem was that save called reparseConfiguration (see 7af829a341c1ff04f9499336a28b6a4dd20bdbdc). But nowadays read which doesn't call reparseConfiguration (right?), so maybe it's fine to call it from save. I'll let Alexander remind us why he removed the read call.
> 
> Matthew Dawson wrote:
>     The read call was removed to avoid changing the KCoreConfigSekleton during the save call, as it wasn't documented as doing that and may surprise some people that flushing their changes may load other unrelated changes.  I would prefer to keep that, for the same reason.
>     
>     I do agree there is a bug that needs fixing, I'm just not sure how to fix it.  As the API stands now, the check with mLoadValue only works if the KConfig used with the KCoreConfigSkeletonItem doesn't change (never mind people manually calling those functions).  Otherwise, the value of mReference gets out of sync with KConfig, and breaks saving in general.  It appears removing the mLoadValue variable solves the bug, and avoids changing the underlying KConfig, assuming its loaded value hasn't changed.  The only benfit I see of keeping the check is avoiding the more complicated checks carried out by KConfig to verify the value is unchanged.
>     
>     I think the best course of action is to remove the check for now, as it create subtle issues.  The way KCoreConfigSkeletonItem works may need to be changed, but that can be done later.  Thoughts?

I disagree, the easiest thing is adding back the read, it's been there in kdelibs for years, it works and has been tested by the time. Users can't be "suprised" by it, since it's been there forever. OTOH yes, it changes the KCoreConfigSekleton because someone added the possibility of modifying it, like the removeItem and stuff, before it was all static and fine. 

Honestly before removing the mLoadedValue variable i think i'd prefer adding mLoadedValue = mReference; in all the ::writeConfig


- Albert


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121838/#review73083
-----------------------------------------------------------


On gen. 4, 2015, 4:04 p.m., Albert Astals Cid wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121838/
> -----------------------------------------------------------
> 
> (Updated gen. 4, 2015, 4:04 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> -------
> 
> We need to refresh mLoadedValue after a save, otherwise the value is stale.
> 
> I'm doing this by adding back the read() call in KCoreConfigSkeleton::save (which is what kdelibs did).
> 
> Another option would be adding lots of mLoadedValue = mReference; in all the ::writeConfig, but that seems much more prone.
> 
> I've also refactored the tests so they can be run independently now just fine.
> 
> 
> Diffs
> -----
> 
>   autotests/kconfigskeletontest.h c54c7b0 
>   autotests/kconfigskeletontest.cpp f401b9f 
>   src/core/kcoreconfigskeleton.cpp e4255a6 
> 
> Diff: https://git.reviewboard.kde.org/r/121838/diff/
> 
> 
> Testing
> -------
> 
> My tests now pass.
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150105/74e1c3e4/attachment.html>