Persist "mask values" in gui

[jadijadi]

fixes #652

[hernanmarino]

tested ACK

[furszy]

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.

[jadijadi]

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.

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.

[ryanofsky]

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.

Also, instead of using Node::getPersistentSetting and Node::updateRwSetting methods, this should probably use QSettings::value and QSettings::setValue methods to access the setting. If you look at OptionsModel you can see that it uses QSettings to store GUI preferences. The Node methods are only used to store settings which are bitcoind command line or configuration file options (and -privacy is not a bitcoind option).

[jadijadi]

tested ACK

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.

[jadijadi]

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.

Also, instead of using Node::getPersistentSetting and Node::updateRwSetting methods, this should probably use QSettings::value and QSettings::setValue methods to access the setting. If you look at OptionsModel you can see that it uses QSettings to store GUI preferences. The Node methods are only used to store settings which are bitcoind command line or configuration file options (and -privacy is not a bitcoind option).

thanks @ryanofsky & @furszy . I've updated the PR based your comment; via OptionsModel.

[ryanofsky]

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.

[ryanofsky]

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)

[hernanmarino]

re ACK 9fde0ba

[hebasto]

It looks like #701 is being actively reviewed recently. Let's combine all discussions there.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hernanmarino

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.