Persist "mask values" in gui #655
Conversation
There was a problem hiding this comment.
Need to add an option inside the OptionsModel and use setOption to store/update it. Not call directly the backend function, it breaks the layers division.
Essentially, OptionsModel class is the interface from the GUI to the node's configuration data structure.
In other words, GUI widgets/models uses the OptionsModel to access/modifiy the node settings.
Thanks for the important note. I'm traveling this week with limited access to my dev machine / internet but will try to provide a fix per your suggestion soon. Thanks again. |
Also, instead of using |
1bcfcb0 to
9fde0ba
Compare
Thanks for the tests & ACK. I've done some changes based on the comments from @ryanofsky and @furszy . Can you please have another look? Thank you in advance. |
thanks @ryanofsky & @furszy . I've updated the PR based your comment; via OptionsModel. |
| @@ -54,6 +54,7 @@ static const char* SettingName(OptionsModel::OptionID option) | |||
| case OptionsModel::ProxyPortTor: return "onion"; | |||
| case OptionsModel::ProxyUseTor: return "onion"; | |||
| case OptionsModel::Language: return "lang"; | |||
| case OptionsModel::Privacy: return "privacy"; | |||
There was a problem hiding this comment.
In commit "Persist "mask values" in gui" (9fde0ba)
This change to SettingName function doesn't do anything and should be reverted. SettingName maps OptionModel enum values to bitcoin.conf / command line option names, and there is no "-privacy" option, so this wouldn't do the right thing even if the case was ever used.
If you want to use OptionsModel to read and write this setting, instead of changing the SettingName function, you should change the OptionsModel::getOption() function by adding:
case Privacy:
return settings.value("privacy");And the OptionsModel::setOption() function by adding:
case Privacy:
settings.setValue("privacy", value.toBool());
break;You could grep CoinControlFeatures or EnablePSBTControls to see how another similar GUI settings implemented in OptionsModel.
| @@ -176,6 +178,9 @@ void OverviewPage::handleTransactionClicked(const QModelIndex &index) | |||
|
|
|||
| void OverviewPage::setPrivacy(bool privacy) | |||
| { | |||
| QSettings settings; | |||
| settings.setValue("privacy", privacy); | |||
There was a problem hiding this comment.
In commit "Persist "mask values" in gui" (9fde0ba)
I don't think this is too important, but I'm not sure if I agree with furszy's comment that this setting needs to be part of OptionsModel. It seems nice to make it part of OptionsModel but also reasonable to not make it part of OptionsModel and just read/write it with QSettings directly.
But probably it would be better to pick one approach or the other. Either don't add the setting to OptionsModel and use QSettings to read and write. Or do add the setting to OptionsModel and use OptionsModel to read and write. Otherwise you wind up causing inconsistent access to the setting across layers that furszy was trying to avoid #655 (review)
|
It looks like #701 is being actively reviewed recently. Let's combine all discussions there. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
fixes #652