Persist Mask Values option

achow101 commented at 0:13 am on January 24, 2023: member

The mask values option is memory only. If a user has enabled this option, it’s reasonable to expect that they would want to have it enabled on the next start.

DrahtBot commented at 0:13 am on January 24, 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	jarolrod, RandyMcMillan, pablomartin4btc, john-moffett
Concept ACK	instagibbs, hernanmarino, kristapsk

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

jarolrod added the label Feature on Jan 24, 2023

jarolrod added the label UX on Jan 24, 2023

in src/qt/optionsmodel.cpp:621 in ee96a37d85 outdated

615@@ -612,6 +616,9 @@ bool OptionsModel::setOption(OptionID option, const QVariant& value)
616             setRestartRequired(true);
617         }
618         break;
619+    case MaskValues:
620+        m_mask_values = value.toBool();
621+        settings.setValue("mask_values", m_mask_values);

jarolrod commented at 1:12 am on January 24, 2023:

:)

0        settings.setValue("mask_values", m_mask_values);
1        break;

achow101 commented at 1:59 am on January 24, 2023:

Fixed.

instagibbs commented at 1:26 am on January 24, 2023: contributor

concept ACK, this is better for sure

qt: Persist Mask Values option

The mask values option is memory only. If a user has enabled this
option, it's reasonable to expect that they would want to have it
enabled on the next start.

4de02def84

achow101 force-pushed on Jan 24, 2023

jarolrod approved

jarolrod commented at 2:46 am on January 24, 2023: member

tACK 4de02def844102c08b65bf1311a333e7aca482b9

Had been on my list to-do for a while. I agree with this, a follow-up could be to have a setting that would make it so that you’d have to unlock the wallet (if it’s encrypted) in order to unmask values.

RandyMcMillan commented at 3:27 am on January 24, 2023: contributor

tACK https://github.com/bitcoin-core/gui/commit/4de02def844102c08b65bf1311a333e7aca482b9

Obfuscating the amounts in the Transactions panel based on this user preference may be a good follow up to this change. Screen Shot 2023-01-23 at 10 24 26 PM

hernanmarino approved

hernanmarino commented at 8:58 pm on February 1, 2023: contributor

ACK , persisting this value seems like the natural option

hebasto renamed this:
~~qt: Persist Mask Values option~~
Persist Mask Values option
on Feb 2, 2023

hebasto commented at 12:01 pm on February 2, 2023: member

The mask values option is memory only. If a user has enabled this option, it’s reasonable to expect that they would want to have it enabled on the next start.

It would be nice to have some words about how this PR differs from #655.

achow101 commented at 4:07 pm on February 2, 2023: member

It would be nice to have some words about how this PR differs from #655.

Hmm, didn’t realize that PR existed.

The difference is that this PR uses OptionsModel and follows the pattern used for all of our other settings rather than writing to QSettings directly.

jarolrod commented at 2:21 am on February 3, 2023: member

I agree with the direction this takes over the other pr’s approach

pablomartin4btc approved

pablomartin4btc commented at 4:51 am on February 3, 2023: contributor

tested ACK 4de02def844102c08b65bf1311a333e7aca482b9.

Verified that file Bitcoin-Qt.conf gets written with line mask_values=true.

This PR fixed issue #652.

pablomartin4btc commented at 7:06 pm on February 3, 2023: contributor

Also, it set’s the value to false in the config file when the check is unticked.

I agree with the follow-ups (@jarolrod & @RandyMcMillan).

pablomartin4btc cross-referenced this on Feb 6, 2023 from issue Mask values on Transactions View by pablomartin4btc

kristapsk commented at 9:22 am on February 6, 2023: contributor

Concept ACK

pablomartin4btc commented at 5:47 pm on February 6, 2023: contributor

tACK 4de02de

Obfuscating the amounts in the Transactions panel based on this user preference may be a good follow up to this change.

I have done it on PR #708.

john-moffett approved

john-moffett commented at 8:15 pm on February 6, 2023: contributor

tACK 4de02def844102c08b65bf1311a333e7aca482b9

in src/qt/bitcoingui.cpp:651 in 4de02def84

646@@ -647,6 +647,8 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel, interfaces::BlockAndH
647             // initialize the disable state of the tray icon with the current value in the model.
648             trayIcon->setVisible(optionsModel->getShowTrayIcon());
649         }
650+
651+        m_mask_values_action->setChecked(_clientModel->getOptionsModel()->getOption(OptionsModel::OptionID::MaskValues).toBool());

hebasto commented at 1:51 pm on February 7, 2023:

0        m_mask_values_action->setChecked(optionsModel->getMaskValues());

accompanied with

0--- a/src/qt/optionsmodel.h
1+++ b/src/qt/optionsmodel.h
2@@ -98,6 +98,7 @@ public:
3     bool getSubFeeFromAmount() const { return m_sub_fee_from_amount; }
4     bool getEnablePSBTControls() const { return m_enable_psbt_controls; }
5     const QString& getOverriddenByCommandLine() { return strOverriddenByCommandLine; }
6+    bool getMaskValues() const { return m_mask_values; }
7 
8     /* Explicit setters */
9     void SetPruneTargetGB(int prune_target_gb);

pablomartin4btc commented at 0:56 am on February 8, 2023:

Also, you could just use OptionsModel::MaskValues without the OptionID, is this an issue?

pablomartin4btc commented at 8:41 pm on February 8, 2023:

I think @ryanofsky was recommending to remove the case from the SettingsName due to its purpose of mapping OptionsModel to command line options but to add OptionsModel::MaksValues (or leave in this case as @achow101 already added them) to setOption and getOption.

The implementation of the getter looks simpler but then you would need to add the setter too to make it consistent and revert the above (remove the case from setOption and getOption).

Perhaps as a follow-up we could check the model consistent with either solution for the rest of settings (?).

achow101 commented at 7:59 pm on February 9, 2023:

I think I’ll leave this as is since it’s consistent with most of the other settings which use the generic function with the enum.

achow101 commented at 7:59 pm on February 9, 2023:

Not an issue, will change if I need to retouch.

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

Instead of raw "mask_values" literals, suggesting to update the SettingName() function:

0--- a/src/qt/optionsmodel.cpp
1+++ b/src/qt/optionsmodel.cpp
2@@ -54,6 +54,7 @@ static const char* SettingName(OptionsModel::OptionID option)
3     case OptionsModel::ProxyPortTor: return "onion";
4     case OptionsModel::ProxyUseTor: return "onion";
5     case OptionsModel::Language: return "lang";
6+    case OptionsModel::MaskValues: return "mask_values";
7     default: throw std::logic_error(strprintf("GUI option %i has no corresponding node setting.", option));
8     }
9 }

and use its return value.

UPD. It seems @ryanofsky has a different opinion, so feel free to ignore this comment.

hebasto cross-referenced this on Feb 7, 2023 from issue Persist "mask values" in gui by jadijadi

pablomartin4btc cross-referenced this on Feb 8, 2023 from issue Persist "mask values" to prevent shoulder surfing by fanquake

achow101 commented at 7:58 pm on February 9, 2023: member

Instead of raw "mask_values" literals, suggesting to update the SettingName() function: … UPD. It seems @ryanofsky has a different opinion, so feel free to ignore this comment.

Originally I had done that, but was looking into how that function is used and decided that adding the option there doesn’t make sense. It looks like SettingName() is used to map settings that also appear on the command line or in the bitcoin.conf, and the mask_values does not.

hebasto merged this on Feb 9, 2023

hebasto closed this on Feb 9, 2023

sidhujag referenced this in commit 4cda966eae on Feb 10, 2023

hebasto referenced this in commit 460e394625 on Mar 14, 2023

bitcoin-core locked this on Mar 3, 2024

Persist Mask Values option #701

Reviews