Set initial value for bsqAverageTrimThreshold at update to 5% #4747
Conversation
update (using persisted preferences which would have value set to 0 as it was not existing in old version)
chimp1984
changed the title
There was a problem hiding this comment.
NACK. This change blindly sets the threshold value to 5% regardless of its previous value. I tested this by bringing up my client, setting the value to 1%, shutting it down and restarting it and noticing that the value reverted to 5%. I tried it again, thinking that perhaps my value didn't persist. I set it to 2%, clicked around to different screens in the UI, came back to settings, confirmed that 2% was still set, shut the client down and restarted, and once again the value was set to 5%. When fixing this, it should also be considered that 0% is a valid value, i.e. if someone sets that value, it should persist and not be reset to 5%. The implementation should check that the preference value is null/absent in order to force it to 5%, not just equivalent to 0.
|
Ah damn... was too much rushed... |
|
Hm... not that easy to fix as default is 0 if protobug field was not set, but we allow 0 as value, so if we just check for 0 it would overwrite it when you set 0. I need to check if the protobuf reveals some more info it the field was undefined so we can set it to -1 and check for that... But busy atm with some issues with seed nodes... |
|
Seems for primitive types there is no way to check if field was set and it delivers 0 if not set. @cbeams Do you know any way to deal with that? Wrapping the double in a protoDouble would work but thats ugly... Or we accept that when setting to 0 it will not persist your custom setting. Not that important feature anyway... |
See comment at change for more background.
|
ACK. I've tested the latest change and can confirm all works as expected. It is indeed unfortunate to not allow 0% as a legitimate value, but I agree that further work on this very minor feature is probably not worth it. The comment in the code is enough for now. Perhaps if someone complains about it, more work would be merited. |
Set initial value for bsqAverageTrimThreshold to 5% in case of an update (using persisted preferences which would
have value set to 0 as it was not existing in old version)
Fixes #4706 (comment)