> 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? > > Albert Astals Cid wrote: > 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 > > Matthew Dawson wrote: > Regarding surprise, yes an existing user is unlikely to be surprised as > they have probably been using the api and figured out its warts. I was > firstly thinking more of new users coming to KConfig, who read the > documentation and don't learn of the fact the value can change when saving > (easily fixed with a documentation string). Secondly, I'd expect most people > to expect the save call to not modify the values in any way, but that's my > belief and can be changed. Due to point two, I'd prefer not to put the read > call back in the save path just to fix this bug, but I'm happy to reconsider > as a general aspect of the code. > > I'm not sold on removing either mLoadedValue, as that means KEntryMap can > be modified when it isn't expected. If you think adding that extra line in > all the saves paths is better then removing mLoadedValue checks, I think that > is probably the best route for now. Ideally I think mLoadedValue should be > tied to the KConfig used, but I don't see an easy way to do that while > maintaining binary compatibility so I'm not worried about that to fix this > bug. > > David Faure wrote: > Since you're talking about expectations for users of the API, can you > explain the issue in user terms for those who don't know all the > implementation details? > > This is about what happens when modifying the KConfig object (in memory) > directly (by passing the KConfigXT layer) -- or is this about modifying the > file on disk (from another process)? > > Let's find out what would be the most logical behavior, the reasoning "it > has been in kdelibs for years" doesn't hold given that we renamed the > functions on purpose, to reflect a behavior change.
Honestly i don't understand why issuing a "read" after a "save" would change my object, that's unexpected to me, on every other API I used that had save/read, doing a read after save got me the same, since i had just saved, so why would it be different? I this case seems Matthew says it's not the same? Why? - 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
