Persist "mask values" in gui

jadijadi commented at 11:26 am on August 25, 2022: contributor

fixes #652

hernanmarino approved

hernanmarino commented at 2:48 pm on September 10, 2022: contributor

tested ACK

furszy changes_requested

furszy commented at 3:22 pm on September 10, 2022: member

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 commented at 12:59 pm on September 12, 2022: contributor

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 commented at 5:42 pm on September 12, 2022: contributor

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).

Persist "mask values" in gui

fixes https://github.com/bitcoin-core/gui/issues/652

9fde0bae91

jadijadi force-pushed on Sep 22, 2022

jadijadi commented at 7:45 am on September 23, 2022: contributor

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 commented at 7:46 am on September 23, 2022: contributor

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.

in src/qt/optionsmodel.cpp:57 in 9fde0bae91

53@@ -54,6 +54,7 @@ static const char* SettingName(OptionsModel::OptionID option)
54     case OptionsModel::ProxyPortTor: return "onion";
55     case OptionsModel::ProxyUseTor: return "onion";
56     case OptionsModel::Language: return "lang";
57+    case OptionsModel::Privacy: return "privacy";

ryanofsky commented at 5:00 am on September 24, 2022:

In commit “Persist “mask values” in gui” (9fde0bae916f73b73fcdbeff6469dba72c4be212)

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:

0    case Privacy:
1        return settings.value("privacy");

And the OptionsModel::setOption() function by adding:

0    case Privacy:
1        settings.setValue("privacy", value.toBool());
2        break;

You could grep CoinControlFeatures or EnablePSBTControls to see how another similar GUI settings implemented in OptionsModel.

in src/qt/overviewpage.cpp:182 in 9fde0bae91

177@@ -176,6 +178,9 @@ void OverviewPage::handleTransactionClicked(const QModelIndex &index)
178 
179 void OverviewPage::setPrivacy(bool privacy)
180 {
181+    QSettings settings;
182+    settings.setValue("privacy", privacy);

ryanofsky commented at 5:22 am on September 24, 2022:

In commit “Persist “mask values” in gui” (9fde0bae916f73b73fcdbeff6469dba72c4be212)

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#pullrequestreview-1103143874

hebasto renamed this:
~~qt - Persist "mask values" in gui~~
Persist "mask values" in gui
on Oct 25, 2022

hebasto added the label UX on Oct 26, 2022

hernanmarino commented at 4:36 pm on December 6, 2022: contributor

re ACK 9fde0bae916f73b73fcdbeff6469dba72c4be212

hebasto cross-referenced this on Feb 2, 2023 from issue Persist Mask Values option by achow101

hebasto commented at 2:11 pm on February 7, 2023: member

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

DrahtBot commented at 2:11 pm on February 7, 2023: contributor

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.

hebasto closed this on Feb 7, 2023

bitcoin-core locked this on Feb 7, 2024

Persist “mask values” in gui #655

Reviews