Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between
On Jan. 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. 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. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121838/#review73083 --- On Jan. 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 Jan. 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
Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between
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
Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between
On Jan. 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. Albert Astals Cid wrote: 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? I can't answer because you haven't explained to me what this is about, as I requested... Please answer my question, so I can answer yours :) (but I assume read will change member vars in the KConfigSkeleton, for one reason or another, surely it's not a no-op ;) ) - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121838/#review73083 --- On Jan. 4, 2015, 4:04 p.m., Albert
Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between
On gen. 6, 2015, 11:55 p.m., Aleix Pol Gonzalez wrote: CMakeLists.txt, line 5 https://git.reviewboard.kde.org/r/121838/diff/2/?file=338706#file338706line5 Unrelated. And why? Because not all of us are bleeding edge that have everything up to date and requiring ecm 1.6 here is not needed at all so i have to revert it so i can compile without having to install something that is unneeded. And yes, it being part of the diff was a mistake on my side, it hasn't been commited. - Albert --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121838/#review73316 --- On gen. 6, 2015, 11:55 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. 6, 2015, 11:55 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 resetting mLoadedValue to mReference in all the ::writeConfig. I've also refactored the tests so they can be run independently now just fine. Diffs - autotests/kconfigskeletontest.cpp f401b9f src/core/kcoreconfigskeleton.h f8313d1 src/core/kcoreconfigskeleton.cpp e4255a6 CMakeLists.txt 205c38b autotests/kconfigskeletontest.h c54c7b0 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
Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121838/#review73314 --- Ship it! Thanks, LGTM. If possible, could you please split the unit test refactoring out to a separate commit just before the bug fix commit? If not, just push it as is. - Matthew Dawson On Jan. 6, 2015, 5:41 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121838/ --- (Updated Jan. 6, 2015, 5:41 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 resetting mLoadedValue to mReference in all the ::writeConfig. I've also refactored the tests so they can be run independently now just fine. Diffs - autotests/kconfigskeletontest.cpp f401b9f src/core/kcoreconfigskeleton.h f8313d1 src/core/kcoreconfigskeleton.cpp e4255a6 CMakeLists.txt 205c38b autotests/kconfigskeletontest.h c54c7b0 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
Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121838/ --- (Updated Jan. 6, 2015, 11:55 p.m.) Status -- This change has been marked as submitted. 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 resetting mLoadedValue to mReference in all the ::writeConfig. I've also refactored the tests so they can be run independently now just fine. Diffs - autotests/kconfigskeletontest.cpp f401b9f src/core/kcoreconfigskeleton.h f8313d1 src/core/kcoreconfigskeleton.cpp e4255a6 CMakeLists.txt 205c38b autotests/kconfigskeletontest.h c54c7b0 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
Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121838/#review73316 --- CMakeLists.txt https://git.reviewboard.kde.org/r/121838/#comment50989 Unrelated. And why? Other than that, +1. - Aleix Pol Gonzalez On Jan. 6, 2015, 11:55 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121838/ --- (Updated Jan. 6, 2015, 11:55 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 resetting mLoadedValue to mReference in all the ::writeConfig. I've also refactored the tests so they can be run independently now just fine. Diffs - autotests/kconfigskeletontest.cpp f401b9f src/core/kcoreconfigskeleton.h f8313d1 src/core/kcoreconfigskeleton.cpp e4255a6 CMakeLists.txt 205c38b autotests/kconfigskeletontest.h c54c7b0 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
Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121838/ --- (Updated gen. 6, 2015, 10:41 p.m.) Review request for KDE Frameworks and Matthew Dawson. Changes --- Go the mLoadedValue=mReference way Repository: kconfig Description (updated) --- We need to refresh mLoadedValue after a save, otherwise the value is stale. I'm doing this by resetting mLoadedValue to mReference in all the ::writeConfig. I've also refactored the tests so they can be run independently now just fine. Diffs (updated) - autotests/kconfigskeletontest.cpp f401b9f src/core/kcoreconfigskeleton.h f8313d1 src/core/kcoreconfigskeleton.cpp e4255a6 CMakeLists.txt 205c38b autotests/kconfigskeletontest.h c54c7b0 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
Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between
On Jan. 4, 2015, 12: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 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. - Matthew --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121838/#review73083 --- On Jan. 4, 2015, 11:04 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121838/ --- (Updated Jan. 4, 2015, 11:04 a.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
Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between
On Jan. 4, 2015, 12: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. 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? - Matthew --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121838/#review73083 --- On Jan. 4, 2015, 11:04 a.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/121838/ --- (Updated Jan. 4, 2015, 11:04 a.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
Re: Review Request 121838: Fix KCoreConfigSkeleton when toggling a value with saves in between
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 ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
