Skip to content

Persist "mask values" in gui #655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Persist "mask values" in gui #655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/qt/bitcoingui.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,8 @@ BitcoinGUI::~BitcoinGUI()

void BitcoinGUI::createActions()
{
QSettings settings;

QActionGroup *tabGroup = new QActionGroup(this);
connect(modalOverlay, &ModalOverlay::triggered, tabGroup, &QActionGroup::setEnabled);

Expand Down Expand Up @@ -359,6 +361,7 @@ void BitcoinGUI::createActions()
m_mask_values_action->setShortcut(QKeySequence(Qt::CTRL | Qt::SHIFT | Qt::Key_M));
m_mask_values_action->setStatusTip(tr("Mask the values in the Overview tab"));
m_mask_values_action->setCheckable(true);
m_mask_values_action->setChecked(settings.value("privacy").toBool());

connect(quitAction, &QAction::triggered, this, &BitcoinGUI::quitRequested);
connect(aboutAction, &QAction::triggered, this, &BitcoinGUI::aboutClicked);
Expand Down
5 changes: 5 additions & 0 deletions src/qt/optionsmodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Contributor

@ryanofsky ryanofsky Sep 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

default: throw std::logic_error(strprintf("GUI option %i has no corresponding node setting.", option));
}
}
Expand Down Expand Up @@ -189,6 +190,10 @@ bool OptionsModel::Init(bilingual_str& error)
}
m_enable_psbt_controls = settings.value("enable_psbt_controls", false).toBool();

if (!settings.contains("privacy")) {
settings.setValue("privacy", false);
}

// These are shared with the core or have a command-line parameter
// and we want command-line parameters to overwrite the GUI settings.
for (OptionID option : {DatabaseCache, ThreadsScriptVerif, SpendZeroConfChange, ExternalSignerPath, MapPortUPnP,
Expand Down
1 change: 1 addition & 0 deletions src/qt/optionsmodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ class OptionsModel : public QAbstractListModel
MapPortUPnP, // bool
MapPortNatpmp, // bool
MinimizeOnClose, // bool
Privacy, // bool
ProxyUse, // bool
ProxyIP, // QString
ProxyPort, // int
Expand Down
5 changes: 5 additions & 0 deletions src/qt/overviewpage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
#include <qt/transactiontablemodel.h>
#include <qt/walletmodel.h>

#include <QSettings>

#include <QAbstractItemDelegate>
#include <QApplication>
#include <QDateTime>
Expand Down Expand Up @@ -176,6 +178,9 @@ void OverviewPage::handleTransactionClicked(const QModelIndex &index)

void OverviewPage::setPrivacy(bool privacy)
{
QSettings settings;
settings.setValue("privacy", privacy);
Copy link
Contributor

@ryanofsky ryanofsky Sep 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


m_privacy = privacy;
if (m_balances.balance != -1) {
setBalance(m_balances);
Expand Down