Persist Mask Values option #701

achow101 · 2023-01-24T00:13:32Z

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 · 2023-01-24T00:13:35Z

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.

src/qt/optionsmodel.cpp

instagibbs · 2023-01-24T01:26:48Z

concept ACK, this is better for sure

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.

jarolrod

tACK 4de02de

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 · 2023-01-24T03:27:40Z

tACK 4de02de

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

hernanmarino

ACK , persisting this value seems like the natural option

hebasto · 2023-02-02T12:01:54Z

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 · 2023-02-02T16:07:54Z

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 · 2023-02-03T02:21:47Z

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

pablomartin4btc

tested ACK 4de02de.

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

This PR fixed issue #652.

pablomartin4btc

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

kristapsk · 2023-02-06T09:22:02Z

Concept ACK

pablomartin4btc

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

tACK 4de02de

hebasto

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

--- a/src/qt/optionsmodel.cpp
+++ b/src/qt/optionsmodel.cpp
@@ -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::MaskValues: return "mask_values";
     default: throw std::logic_error(strprintf("GUI option %i has no corresponding node setting.", option));
     }
 }

and use its return value.

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

hebasto · 2023-02-07T13:51:58Z

src/qt/bitcoingui.cpp

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


Suggested change

m_mask_values_action->setChecked(_clientModel->getOptionsModel()->getOption(OptionsModel::OptionID::MaskValues).toBool());

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

accompanied with

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

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

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.

pablomartin4btc · 2023-02-08T00:56:56Z

src/qt/bitcoingui.cpp

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


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

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

achow101 · 2023-02-09T19:58:30Z

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.

4de02de qt: Persist Mask Values option (Andrew Chow) Pull request description: 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. ACKs for top commit: RandyMcMillan: tACK bitcoin-core/gui@4de02de jarolrod: tACK 4de02de pablomartin4btc: > tACK [4de02de](bitcoin-core/gui@4de02de) john-moffett: tACK 4de02de Tree-SHA512: 247deb78df4911516625bf8b25d752feb480ce30eb31335cf9baeb07b7c6c225fcc37d5c45de62d6e6895ec10c7eefabb15527e3c9723a3b8ddda1e12ebbf46b

4492de1 qt: mask values on transactions view (pablomartin4btc) Pull request description: Currently the mask values option (Settings menu->Mask values) hides the wallet balances shown on the Overview page including the recent transactions list from the right panel but it doesn't hide the amounts from the transaction view. ![mask values - hiding wallet balances on overview tab but not on transactions tab](https://user-images.githubusercontent.com/110166421/216876325-56a68006-1be0-4b3f-b1e2-a0575c377cf5.gif) This enhancement has been mentioned on PR #701 as a [desirable follow-up](#701 (comment)). First approach was to hide the amounts on the transactions view when mask values option is checked: ![mask values - hiding amounts on transactions tab](https://user-images.githubusercontent.com/110166421/216876440-0ff1a2ec-2ef2-405c-8b62-e4a94b9221cc.gif) But later on as reviewer **furszy** recommended, I've disabled the Transaction tab directly and switch to the Overview tab if the mask values option is set, check the new screenshots in the [comment below](#708 (comment)). ACKs for top commit: achow101: ACK 4492de1 furszy: ACK 4492de1 hernanmarino: ACK 4492de1 Tree-SHA512: 94c04064cde456ab916cb91fc4619353cf4c8ef0e92aa1522a6b2c18a8d0fa641db9fddac04c2e1a290128879db598a9dfc4a0003dcf6e3413543908ada95afb

jarolrod added Feature UX All about "how to get things done" labels Jan 24, 2023

jarolrod reviewed Jan 24, 2023

View reviewed changes

src/qt/optionsmodel.cpp Show resolved Hide resolved

qt: Persist Mask Values option

4de02de

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.

achow101 force-pushed the persist-mask-value branch from ee96a37 to 4de02de Compare January 24, 2023 01:59

jarolrod approved these changes Jan 24, 2023

View reviewed changes

hernanmarino approved these changes Feb 1, 2023

View reviewed changes

hebasto changed the title ~~qt: Persist Mask Values option~~ Persist Mask Values option Feb 2, 2023

pablomartin4btc approved these changes Feb 3, 2023

View reviewed changes

pablomartin4btc reviewed Feb 3, 2023

View reviewed changes

pablomartin4btc mentioned this pull request Feb 6, 2023

Mask values on Transactions View #708

Merged

pablomartin4btc reviewed Feb 6, 2023

View reviewed changes

john-moffett approved these changes Feb 6, 2023

View reviewed changes

hebasto reviewed Feb 7, 2023

View reviewed changes

hebasto mentioned this pull request Feb 7, 2023

Persist "mask values" in gui #655

Closed

pablomartin4btc reviewed Feb 8, 2023

View reviewed changes

pablomartin4btc mentioned this pull request Feb 8, 2023

Persist "mask values" to prevent shoulder surfing #652

Closed

hebasto merged commit 1313b90 into bitcoin-core:master Feb 9, 2023

bitcoin-core locked and limited conversation to collaborators Mar 3, 2024

	m_mask_values_action->setChecked(_clientModel->getOptionsModel()->getOption(OptionsModel::OptionID::MaskValues).toBool());
	m_mask_values_action->setChecked(optionsModel->getMaskValues());

Persist Mask Values option #701

Persist Mask Values option #701

Uh oh!

Conversation

achow101 commented Jan 24, 2023

Uh oh!

DrahtBot commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

Uh oh!

instagibbs commented Jan 24, 2023

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan commented Jan 24, 2023

Uh oh!

hernanmarino left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Feb 2, 2023

Uh oh!

jarolrod commented Feb 3, 2023

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

kristapsk commented Feb 6, 2023

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

john-moffett left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc Feb 8, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 commented Feb 9, 2023

Uh oh!

Uh oh!

DrahtBot commented Jan 24, 2023 •

edited

Loading

hebasto commented Feb 2, 2023 •

edited

Loading

hebasto left a comment •

edited

Loading