qt: Add privacy to the Overview page

hebasto commented at 8:13 am on July 22, 2019: member

This PR allows to hide/reveal values on the Overviewpage by checking/unchecking Menu->Settings-> Mask Values

Closes #16407

Privacy mode is OFF (the default behavior): Screenshot from 2020-01-02 15-08-28

Privacy mode is ON: Screenshot from 2020-01-02 15-10-23 cropped

fanquake added the label GUI on Jul 22, 2019

DrahtBot commented at 10:41 am on July 22, 2019: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17966 (qt, refactor: Optimize signal-slot connections logic by hebasto)
#17877 (qt, refactor: Make enums in BitcoinUnits class scoped by hebasto)
#17659 (qt: Do not block GUI thread in RPCConsole by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

practicalswift commented at 11:08 am on July 22, 2019: contributor

@hebasto Would you mind posting before/after screenshots?

hebasto commented at 11:40 am on July 22, 2019: member

@practicalswift

Would you mind posting before/after screenshots?

Done.

in src/qt/bitcoinamountlabel.cpp:25 in 25bd2167af outdated

20+
21+void BitcoinAmountLabel::changePrivacyMode(bool privacy)
22+{
23+    if (privacy && cache.isEmpty()) {
24+        cache = text();
25+        QLabel::setText("h-i-d-d-e-n");

MarcoFalke commented at 12:01 pm on July 22, 2019:

I think a fence of # translates better.

Couldn’t this use BitcoinUnits::format(unit, 0).replace("0", "#") + unit.shortName() (or whatever is used right now), so that the UI doesn’t reflow the columns on every toggle?

MarcoFalke commented at 12:05 pm on July 22, 2019:

hebasto commented at 1:04 pm on July 22, 2019:

I think a fence of # translates better.

Done.

… so that the UI doesn’t reflow the columns on every toggle?

This requires a monospaced font.

hebasto commented at 2:53 pm on July 22, 2019:

This requires a monospaced font.

Done.

hebasto force-pushed on Jul 22, 2019

laanwj commented at 1:04 pm on July 22, 2019: member

Concept ACK, definitely prefer the # to h-i-d-d-e-n

hebasto commented at 1:07 pm on July 22, 2019: member

@MarcoFalke

I think a fence of # translates better. @laanwj definitely prefer the # to h-i-d-d-e-n

The screenshot on OP has been updated.

achow101 commented at 1:24 pm on July 22, 2019: member

Can it use a fixed number/string of #s instead of replacing each character with #? That way we don’t leak any information about the number of coins. For example, right now, in your screenshot, we can see that you have at least 1000 coins and at least 100 available to spend.

jonasschnelli commented at 2:17 pm on July 22, 2019: contributor

Thanks for working on this! Tested a bit. Works.

Not sure if clicking on the actual amount label is the right way to activate this (may confuse user who lacks understanding the “hidden”-concept).

Plus, should it not also cover the “Recent transactions”-list amounts?

I would say ideally, once enabled, the values in the transaction list should also be “hidden”.

promag commented at 2:24 pm on July 22, 2019: member

Concept ACK. Not so sure about the approach as it requires too much wiring. I’m testing a different approach and I’ll give feedback if it’s worth it.

promag commented at 2:26 pm on July 22, 2019: member

Not sure if clicking on the actual amount label is the right way to activate this

Agree with @jonasschnelli, I initially thought of having a toggle somewhere (in the status bar for instance) as well a menu option. Something for a follow up.

hebasto force-pushed on Jul 22, 2019

hebasto commented at 2:32 pm on July 22, 2019: member

@achow101

Can it use a fixed number/string of #s instead of replacing each character with #? That way we don’t leak any information about the number of coins. For example, right now, in your screenshot, we can see that you have at least 1000 coins and at least 100 available to spend.

Done. The screenshot on OP has been updated.

hebasto commented at 2:43 pm on July 22, 2019: member

@jonasschnelli Thank you for your review.

Not sure if clicking on the actual amount label is the right way to activate this (may confuse user who lacks understanding the “hidden”-concept).

It seems very intuitive ;) (also there is an example of usage in Wasabi) To further improvements (on the following PR), a toggle could be added (as @promag suggested).

Plus, should it not also cover the “Recent transactions”-list amounts? I would say ideally, once enabled, the values in the transaction list should also be “hidden”.

Agree. I’d leave this improvement for the following PRs.

kristapsk commented at 10:35 pm on July 22, 2019: contributor

Not sure if clicking on the actual amount label is the right way to activate this (may confuse user who lacks understanding the “hidden”-concept).

That’s the way “lurking wife mode” of Wasabi Wallet, which is basically the same, is activated. I think Bitcoin is already hard enough for newcomers, so unnecessary reinventing the wheel should be avoided, if possible, so that users get similar experience using different wallets. Unless there are clear benefits in doing so, of course. But probably some message about privacy / stealth mode being activated and how to turn it back off would not hurt.

in src/qt/bitcoinamountlabel.cpp:36 in a65a2be962 outdated

31+        assert(parts.size() == 2);
32+        QString integer_part = parts.first();
33+        integer_part = QString(integer_part.size() - 1, ' ') + mask;
34+        QString fractional_part = parts.last();
35+        fractional_part = fractional_part.replace(QRegularExpression("\\d"), mask);
36+        QLabel::setText(integer_part + '.' + fractional_part);

kristapsk commented at 11:18 pm on July 22, 2019:

Just looked at how Wasabi did this and they just replace any amounts, addresses, labels everywhere with a fixed number of #’s. IMO “#########” even looks better than “#.########” and it’s less lines of code, all this block could be just replaced with

0cache = text();
1QLabel::setText("#########");`

hebasto commented at 5:34 am on July 23, 2019:

Unfortunately, this will cause the UI to reflow the columns on every toggle (see @MarcoFalke’s comment).

fanquake commented at 5:43 am on July 23, 2019: member

Concept ACK

hebasto force-pushed on Jul 23, 2019

hebasto commented at 12:26 pm on July 23, 2019: member

Fixed two bugs:

sat units (no fractional part) handling
unit switching handling

fanquake commented at 5:32 am on July 24, 2019: member

Had a quick test. Functionality works as described. I might just be seeing things, but the monospace font looks a bit weird no macOS, don’t think that has to be any sort of blocker though. @promag Are you still working on a different approach, or happy with this PR now?

master: master

This PR: 16432-values 16432-hidden

darosior commented at 9:55 pm on July 24, 2019: member

ACK 22c16fd3e19437db187a514ba043af635135b719 I readed the code and I tested it. I think it’s a great addition.

promag commented at 1:01 am on July 25, 2019: member

I’ve tried alternatives - basically tried filtering paint events if the widget had amounts or even extend QStyle - but didn’t like at the end.

Currently I think this PR can be improved, I’ll review shortly.

jonasschnelli commented at 12:13 pm on August 12, 2019: contributor

Looks like the build is failing:

0qt/bitcoinamountlabel.cpp:26:47: error: 'QRegularExpression' is an incomplete type
1    fractional_part = fractional_part.replace(QRegularExpression("\\d"), mask);
2                                              ^
3/home/ubuntu/src/depends/x86_64-apple-darwin14/include/QtCore/qstring.h:80:7: note: forward declaration of 'QRegularExpression'
4class QRegularExpression;

https://bitcoinbuilds.org/index.php?build=669

hebasto force-pushed on Aug 12, 2019

hebasto commented at 6:23 pm on August 12, 2019: member

@jonasschnelli

Looks like the build is failing: …

Rebased and fixed by partially reverting the 248e22bbc0d7bc40ae3584d53a18507c46b0e553 commit (#16386).

jonasschnelli commented at 8:20 am on August 13, 2019: contributor

Works as intended. I like the concept but think it should be implemented more radical (maybe in follow up PRs?).

The privacy mode won’t hide recent transaction values
For users not aware of the privacy mode, it’s unclear why the numbers are now # and how to bring it back,… a quick disappearing info-element (statusbar?) would make things much more understandable for users new to this concept (or a toggle element in the icon section of the status bar?).
I think all amount values should be masqueraded, especially the ones in the transaction list.

in depends/packages/qt.mk:96 in 90c7f4f4ba outdated

86@@ -87,7 +87,6 @@ $(package)_config_opts += -no-feature-printdialog
87 $(package)_config_opts += -no-feature-printer
88 $(package)_config_opts += -no-feature-printpreviewdialog
89 $(package)_config_opts += -no-feature-printpreviewwidget
90-$(package)_config_opts += -no-feature-regularexpression

luke-jr commented at 0:28 am on August 20, 2019:

It seems a bit excessive to add regex support just for this…

laanwj commented at 10:56 am on October 26, 2019:

Would agree here. If you really need complex regexps then I don’t mind adding the dep, but it seems overkill for this.

laanwj commented at 11:17 am on October 27, 2019:

OTOH #17213 seems to need qt-regexp as well.

hebasto commented at 5:22 pm on October 27, 2019:

Reverted back.

luke-jr changes_requested

luke-jr commented at 0:30 am on August 20, 2019: member

Rather than cloaking the text we want to hide (which seems problematic so long as we base it on the real hidden value), how about simply setting the background colour to the text colour?

konez2k commented at 3:24 pm on August 20, 2019: none

This should work without regexp

0static QString cloak(QString& text)
1{
2    QStringList parts = text.split(' ');
3    QStringList amounts = parts[0].split('.');
4    for (int i = 0; i < amounts.size(); i++)
5        amounts[i].fill('#');
6    return amounts.join('.') + ' ' + parts[1];
7}

laanwj added the label Feature on Sep 30, 2019

JeremyCrookshank commented at 7:34 pm on October 25, 2019: contributor

Are you still working on this @hebasto. This PR is definitely of interest for me and would love to help/takeover it if you aren’t

hebasto force-pushed on Oct 27, 2019

hebasto commented at 5:33 pm on October 27, 2019: member

@kristapsk

But probably some message about privacy / stealth mode being activated and how to turn it back off would not hurt. @jonasschnelli For users not aware of the privacy mode, it’s unclear why the numbers are now # and how to bring it back,… a quick disappearing info-element (statusbar?) would make things much more understandable for users new to this concept

A relevant message has been added to the status bar.

@luke-jr

Rather than cloaking the text we want to hide (which seems problematic so long as we base it on the real hidden value), how about simply setting the background colour to the text colour?

Nice suggestion! Honestly, I don’t like the look of some white bars in a black window (in the dark desktop theme) ;)

@konez2k

This should work without regexp

0static QString cloak(QString& text)
1{
2    QStringList parts = text.split(' ');
3    QStringList amounts = parts[0].split('.');
4    for (int i = 0; i < amounts.size(); i++)
5        amounts[i].fill('#');
6    return amounts.join('.') + ' ' + parts[1];
7}

text.split(' ') could return more then two parts, e.g., 1 234.56 bits ;)

Nevertheless, the latest push works without QRegularExpression.

@JeremyCrookshank

This PR is definitely of interest for me and would love to help…

Thank you! Would you mind reviewing this PR?

hebasto force-pushed on Oct 27, 2019

hebasto commented at 7:18 pm on October 27, 2019: member

@jonasschnelli

Plus, should it not also cover the “Recent transactions”-list amounts?

Done.

hebasto commented at 7:19 pm on October 27, 2019: member

All comments have been addressed. PR is ready to be re-reviewed ;)

hebasto force-pushed on Oct 28, 2019

hebasto commented at 11:05 am on October 28, 2019: member

Rebased in order to incorporate already merged #15529 and fix MSVC build.

in src/qt/forms/overviewpage.ui:121 in 3960d8564e outdated

114@@ -115,9 +115,10 @@
115              <number>12</number>
116             </property>
117             <item row="2" column="2">
118-             <widget class="QLabel" name="labelWatchPending">
119+             <widget class="BitcoinAmountLabel" name="labelWatchPending">
120               <property name="font">
121                <font>
122+                <family>monospace</family>

luke-jr commented at 9:41 pm on November 13, 2019:

Why not just make this the default for BitcoinAmountLabel?

hebasto commented at 6:24 pm on November 17, 2019:

I’ve tried different approaches, including

0const QFont font{"monospace"};
1QApplication::setFont(font, "BitcoinAmountLabel");

~~Only the current code works as expected. But I’m still open for suggestions.~~

hebasto commented at 6:02 pm on November 28, 2019:

Done.

luke-jr commented at 10:22 pm on November 13, 2019: member

This kind of interferes with selecting to copy… :/

JeremyCrookshank commented at 1:35 pm on November 16, 2019: contributor

As Luke said it’s very hard to copy and paste values with this enabled also
It’s not obvious why they’re hidden if a user is still downloading blocks. I think the best solution is to disable this feature by default and only enable through GUI/config setting.

It might be better as well to have an icon indicator instead of using the entire status bar when in privacy mode.

I also think it would be a better idea to show the recent transactions but hide the BTC amount of each transaction so you can still see your recent transactions perhaps
Also do we need to stop this happening on windows if in privacy mode?(the popup shows value)

hebasto commented at 2:08 pm on November 16, 2019: member

@JeremyCrookshank

Thank you for your review.

As Luke said it’s very hard to copy and paste values with this enabled also

It seems a bit counter-intuitive to try to copy a hidden / masked value.

I think the best solution is to disable this feature by default and only enable through GUI/config setting.

I’d prefer to make GUI/config settings related changes in follow-up PRs.

It might be better as well to have an icon indicator instead of using the entire status bar when in privacy mode.

An icon indicator could be added in follow-up PR. But I’m not agree to replace a status bar message with an icon indicator completely: a text message is (mostly) always clear, an icon needs more cognitive efforts from a user.

I also think it would be a better idea to show the recent transactions but hide the BTC amount of each transaction so you can still see your recent transactions perhaps

Such implementation details could be improved in follow-up PRs.

Also do we need to stop this happening on windows if in privacy mode?(the popup shows value)

Good idea, and it could be implemented in follow-up PRs. A full-fledged privacy mode is not the goal of the current PR.

JeremyCrookshank commented at 2:11 pm on November 16, 2019: contributor

@JeremyCrookshank

Thank you for your review.

As Luke said it’s very hard to copy and paste values with this enabled also

It seems a bit counter-intuitive to try copy a hidden / masked value.

It’s hard to copy the non masked value. As you have to right click to copy which masks it.

luke-jr commented at 2:20 pm on November 16, 2019: member

I typically copy values by double-clicking. No right-click needed, but still broken by this feature.

Maybe a checkable menu item is best?

RandyMcMillan commented at 0:43 am on November 17, 2019: contributor

Using an asterisk as the replacement character may be easier on the eyes.

0*.******

or

0•.••••••

and conforms with what users may be used to as far as password obfuscation.

Each field could have an individual unhide icon next to it.

Screen Shot 2019-11-16 at 7 49 09 PM

So the user can selectively unhide a field without have to turn off privacy mode for the entire UI.

hebasto force-pushed on Nov 17, 2019

hebasto commented at 6:21 pm on November 17, 2019: member

@JeremyCrookshank

It’s hard to copy the non masked value. As you have to right click to copy which masks it. @luke-jr I typically copy values by double-clicking. No right-click needed, but still broken by this feature.

Click-to-hide/reveal buggy feature has been removed.

Maybe a checkable menu item is best?

Implemented in the latest push e9df42cc7066f089c2c83844893a677920ee5859.

MarkLTZ referenced this in commit 9589959c20 on Nov 18, 2019

hebasto force-pushed on Nov 28, 2019

hebasto commented at 6:15 pm on November 28, 2019: member

@luke-jr

Why not just make this the default for BitcoinAmountLabel?

Done in the latest push.

MarkLTZ referenced this in commit 14408dd001 on Dec 17, 2019

MarkLTZ commented at 2:39 pm on December 17, 2019: none

Nice PR. I think it just miss to hide the transactions page.

hebasto commented at 4:45 pm on December 17, 2019: member

@MarkLTZ

Nice PR.

Thank you.

I think it just miss to hide the transactions page.

I believe this improvement could follow this PR.

in src/qt/bitcoinamountlabel.cpp:22 in 7a6766bed6 outdated

15+namespace {
16+const QChar space{' '};
17+const QChar point{'.'};
18+const QChar mask{'#'};
19+
20+QString cloak(QString value_and_unit)

luke-jr commented at 6:12 pm on December 24, 2019:

Basing it on the real number is revealing too much info.

hebasto commented at 8:54 am on December 25, 2019:

Basing it on the real number is revealing too much info.

I’m not sure if I understand you correctly. Mind rewording your concerns?

luke-jr commented at 3:58 pm on December 25, 2019:

Your cloaking reveals at least how many whole number digits are in the real amount, by measuring the number of spaces you add.

https://github.com/bitcoin/bitcoin/commit/98ce0b830e505ddede5d56951e4ce7ee1bf5a0c5

hebasto commented at 11:28 am on January 1, 2020:

Your cloaking reveals at least how many whole number digits are in the real amount, by measuring the number of spaces you add.

Fixed in the latest push.

98ce0b8

Monospace font style prevents the UI to reflow the columns on every toggle (https://github.com/bitcoin/bitcoin/pull/16432#discussion_r305807357).

in src/qt/bitcoinamountlabel.h:31 in 7a6766bed6 outdated

26+
27+public Q_SLOTS:
28+    void setPrivacyMode(bool privacy);
29+
30+private:
31+    QString cache{};

luke-jr commented at 6:14 pm on December 24, 2019:

Should be called m_cache

hebasto commented at 11:24 am on January 1, 2020:

Fixed.

in src/qt/overviewpage.cpp:153 in 7a6766bed6 outdated

148+    connect(this, &OverviewPage::setPrivacyMode, ui->labelImmature, &BitcoinAmountLabel::setPrivacyMode);
149+    connect(this, &OverviewPage::setPrivacyMode, ui->labelTotal, &BitcoinAmountLabel::setPrivacyMode);
150+    connect(this, &OverviewPage::setPrivacyMode, ui->labelWatchAvailable, &BitcoinAmountLabel::setPrivacyMode);
151+    connect(this, &OverviewPage::setPrivacyMode, ui->labelWatchPending, &BitcoinAmountLabel::setPrivacyMode);
152+    connect(this, &OverviewPage::setPrivacyMode, ui->labelWatchImmature, &BitcoinAmountLabel::setPrivacyMode);
153+    connect(this, &OverviewPage::setPrivacyMode, ui->labelWatchTotal, &BitcoinAmountLabel::setPrivacyMode);

luke-jr commented at 6:16 pm on December 24, 2019:

I wonder if there’s a good way to auto-connect these

hebasto commented at 11:37 am on January 1, 2020:

~~I believe this could follow this PR.~~

hebasto commented at 1:29 pm on January 2, 2020:

This part of code has been removed in the latest push.

luke-jr referenced this in commit 4a52e29c79 on Dec 25, 2019

luke-jr referenced this in commit 3105687cef on Dec 25, 2019

luke-jr referenced this in commit 3342bf2e0c on Dec 25, 2019

hebasto force-pushed on Jan 1, 2020

hebasto force-pushed on Jan 2, 2020

hebasto commented at 1:27 pm on January 2, 2020: member

In the latest push:

code is much simplified
monospace font style continues to be used
a shortcut has been added to simplify testing and/or usage

Screenshot from 2020-01-02 15-08-28

Screenshot from 2020-01-02 15-10-23 cropped

~~Currently, the privacy mode is not inherited when a wallet is added/loaded.~~

hebasto force-pushed on Jan 2, 2020

hebasto commented at 3:33 pm on January 2, 2020: member

The OP has been updated. @achow101 @jonasschnelli @fanquake @laanwj @luke-jr @MarcoFalke @promag Would you mind re-reviewing this PR?

achow101 commented at 6:23 pm on January 2, 2020: member

ACK 3935bce9c28eefe38ea4e0152dbbddc630671d27

Tested change and reviewed.

Not sure if this is possible, but it would be nice if the spaces for the right justify weren’t selectable. Is there some other way to achieve the justify using spacers in the ui form?

hebasto commented at 1:03 am on January 3, 2020: member

@achow101

Not sure if this is possible, but it would be nice if the spaces for the right justify weren’t selectable. Is there some other way to achieve the justify using spacers in the ui form?

It is possible, but it requires to overhaul a form layout in a separate PR.

in src/qt/bitcoinamountlabel.cpp:20 in 3935bce9c2 outdated

15+{
16+}
17+
18+void BitcoinAmountLabel::setValue(const CAmount& amount, int unit, bool privacy)
19+{
20+    QString value = BitcoinUnits::format(unit, privacy ? 0 : amount, false, BitcoinUnits::separatorAlways, true);

luke-jr commented at 10:05 pm on January 3, 2020:

Please use formatWithUnit

hebasto commented at 7:36 pm on January 4, 2020:

A new signature of BitcoinUnits::format() is used here.

in src/qt/bitcoinamountlabel.cpp:18 in 3935bce9c2 outdated

13+BitcoinAmountLabel::BitcoinAmountLabel(QWidget* parent)
14+    : QLabel(parent)
15+{
16+}
17+
18+void BitcoinAmountLabel::setValue(const CAmount& amount, int unit, bool privacy)

luke-jr commented at 10:05 pm on January 3, 2020:

I’m not sure there’s a point to a dedicated widget anymore…

hebasto commented at 7:35 pm on January 4, 2020:

Agree. Fixed in the latest push.

in src/qt/bitcoinamountlabel.cpp:24 in 3935bce9c2 outdated

19+{
20+    QString value = BitcoinUnits::format(unit, privacy ? 0 : amount, false, BitcoinUnits::separatorAlways, true);
21+    if (privacy) {
22+        value.replace('0', '#');
23+    }
24+    QLabel::setText(value + QString(" ") + BitcoinUnits::shortName(unit));

luke-jr commented at 10:05 pm on January 3, 2020:

QLabel:: is unnecessary

luke-jr changes_requested

hebasto force-pushed on Jan 4, 2020

in src/qt/forms/overviewpage.ui:9 in ea1fb691c9 outdated

 5@@ -6,8 +6,8 @@
 6    <rect>
 7     <x>0</x>
 8     <y>0</y>
 9-    <width>596</width>
10-    <height>342</height>
11+    <width>798</width>

hebasto commented at 7:39 pm on January 4, 2020:

Note to reviewers: these changes in numbers are just Qt Designer artefacts.

hebasto commented at 7:43 pm on January 4, 2020: member

As @luke-jr rightly commented:

I’m not sure there’s a point to a dedicated widget anymore…

So the dedicated BitcoinAmountLabel widget has been removed.

luke-jr referenced this in commit a35821fd61 on Jan 5, 2020

luke-jr referenced this in commit bc77c967ef on Jan 5, 2020

DrahtBot added the label Needs rebase on Feb 1, 2020

hebasto force-pushed on Feb 2, 2020

hebasto commented at 6:23 pm on February 2, 2020: member

Rebased after #17937 has been merged.

DrahtBot removed the label Needs rebase on Feb 2, 2020

kristapsk commented at 11:28 pm on February 2, 2020: contributor

I think good place for icon when this mode is enabled would be near “Balances” and “Recent transactions” titles, at is is already done during sync, which informs that balances may be wrong and recent transactions might not appear. As this privacy mode also impacts the same things. But agree that this could be added as a follow-up PR. Also, more important IMHO would be to work on hiding values in other parts of interface too when this mode is enabled.

kristapsk approved

kristapsk commented at 11:29 pm on February 2, 2020: contributor

ACK 08a3048dd703d67f878d16be00a6288a5d27157f (reviewed code and tested).

luke-jr commented at 3:37 am on February 3, 2020: member

Hmm, I wonder if just “closing” the wallets in the GUI (but leaving them open for sync/updates) would work best.

MarkLTZ referenced this in commit 136a27c84d on Feb 3, 2020

hebasto commented at 6:59 pm on February 3, 2020: member

Hmm, I wonder if just “closing” the wallets in the GUI (but leaving them open for sync/updates) would work best.

TBH, I do not like how it would look like (from the point of a user).

DrahtBot added the label Needs rebase on Mar 23, 2020

hebasto force-pushed on Mar 24, 2020

hebasto commented at 4:54 pm on March 24, 2020: member

Rebased 08a3048dd703d67f878d16be00a6288a5d27157f -> dba83b9dab919ea9c72f4321612605eb3242d523 due to the conflict with #18278.

DrahtBot removed the label Needs rebase on Mar 24, 2020

DrahtBot added the label Needs rebase on Apr 27, 2020

hebasto force-pushed on Apr 29, 2020

hebasto commented at 5:23 pm on April 29, 2020: member

Rebased dba83b9dab919ea9c72f4321612605eb3242d523 -> 6920e1236b3f7a6b46c78f058aac4a32fcfede9f due to the conflict with #16528.

DrahtBot removed the label Needs rebase on Apr 29, 2020

hebasto commented at 8:08 am on May 6, 2020: member

@achow101 @jonasschnelli @luke-jr @promag and @instagibbs (as initiator of #16407) Mind re-reviewing this PR?

instagibbs commented at 1:26 pm on May 6, 2020: member

@hebasto huh didn’t know this PR existed. Will review.

instagibbs commented at 1:29 pm on May 6, 2020: member

just from looking at the screenshots, did you consider something like adding a GUI toggle above/near the balances themselves? It’s probably something I’d want to toggle on and off pretty fast, yeah?

Concept ACK on the ##### scheme at least

hebasto commented at 1:32 pm on May 6, 2020: member

just from looking at the screenshots, did you consider something like adding a GUI toggle above/near the balances themselves? It’s probably something I’d want to toggle on and off pretty fast, yeah?

https://github.com/bitcoin/bitcoin/blob/6920e1236b3f7a6b46c78f058aac4a32fcfede9f/src/qt/bitcoingui.cpp#L354

instagibbs commented at 2:07 pm on May 6, 2020: member

I see, it’s a top-level setting. Light testing shows it does what’s on the tin.

edit: Continued testing is :heavy_check_mark:

tACK https://github.com/bitcoin/bitcoin/pull/16432/commits/6920e1236b3f7a6b46c78f058aac4a32fcfede9f

in src/qt/bitcoinunits.h:80 in 6920e1236b outdated

76+    static QString format(int unit, const CAmount& amount, bool plussign = false, SeparatorStyle separators = separatorStandard, bool justify = false);
77     //! Format as string (with unit)
78     static QString formatWithUnit(int unit, const CAmount& amount, bool plussign=false, SeparatorStyle separators=separatorStandard);
79     //! Format as HTML string (with unit)
80     static QString formatHtmlWithUnit(int unit, const CAmount& amount, bool plussign=false, SeparatorStyle separators=separatorStandard);
81+    //! Format as string (with unit) of fixed length to preserve privacy if it set

jonatack commented at 2:34 pm on May 9, 2020:

pico nit: s/if it set/, if it is set/ or “, if set”

hebasto commented at 2:26 pm on May 23, 2020:

Updated.

in src/qt/bitcoingui.cpp:355 in 6920e1236b outdated

349@@ -350,6 +350,11 @@ void BitcoinGUI::createActions()
350     showHelpMessageAction->setMenuRole(QAction::NoRole);
351     showHelpMessageAction->setStatusTip(tr("Show the %1 help message to get a list with possible Bitcoin command-line options").arg(PACKAGE_NAME));
352 
353+    m_mask_values_action = new QAction(tr("&Mask values"), this);
354+    m_mask_values_action->setShortcut(QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_M));
355+    m_mask_values_action->setStatusTip(tr("Cloak values on Overview tab"));

jonatack commented at 3:02 pm on May 9, 2020:

Easy user-facing improvement:

s/values on/the values in the|all values in the/
I’d suggest keeping the verb consistent everywhere, e.g. s/Cloak/Mask/

hebasto commented at 2:27 pm on May 23, 2020:

Updated.

in src/qt/overviewpage.cpp:168 in 6920e1236b outdated

163+        }
164+
165+        ui->listTransactions->setVisible(!m_privacy);
166+    }
167+
168+    const QString status_tip = m_privacy ? tr("PRIVACY mode is activated. To reveal cloaked values uncheck Menu->Settings->Mask values") : "";

jonatack commented at 3:06 pm on May 9, 2020:

User-facing suggestion:

0- "PRIVACY mode is activated. To reveal cloaked values uncheck Menu->Settings->Mask values"
1+ "PRIVACY mode is activated. To unmask the values, uncheck Menu->Settings->Mask values."

adds a missing “the”, comma, and period
replaces “reveal cloaked” with the more consistent, shorter, and simpler “unmask”

(also, maybe consider not screaming :) e.g. s/PRIVACY/Privacy/)

hebasto commented at 2:27 pm on May 23, 2020:

Updated.

in src/qt/bitcoinunits.cpp:166 in 6920e1236b outdated

159+    QString value = format(unit, privacy ? 0 : amount, false, separators, true);
160+    if (privacy) {
161+        value.replace('0', '#');
162+    }
163+    return value + QString(" ") + shortName(unit);
164+}

jonatack commented at 3:11 pm on May 9, 2020:

micro-nit, can perform one less conditional (with slightly improved readability) by avoiding the embedded ternary:

0-    QString value = format(unit, privacy ? 0 : amount, false, separators, true);
1+    QString value;
2     if (privacy) {
3+        value = format(unit, 0, false, separators, true);
4         value.replace('0', '#');
5+    } else {
6+        value = format(unit, amount, false, separators, true);
7     }

hebasto commented at 2:27 pm on May 23, 2020:

Updated.

jonatack commented at 3:24 pm on May 9, 2020: contributor

Tested ACK 6920e1236b3f7 with some suggestions if you retouch or for a follow-up. Feel free to ignore the code nits; I’m more concerned with the user-facing ones.

I wonder if it’s stealthy enough with the screaming mode activation message at the bottom. For a follow-up, it might be good to add a user option in Settings -> Options -> Display (or Privacy) to allow not displaying it.

All in all, a cool addition :sunglasses:

qt: Add BitcoinUnits::formatWithPrivacy() function 73d8ef7274

hebasto force-pushed on May 23, 2020

hebasto commented at 2:22 pm on May 23, 2020: member

Updated 6920e1236b3f7a6b46c78f058aac4a32fcfede9f -> 206d17e4d7131179e7c5a5e74632806859d7d493 (pr16432.17 -> pr16432.18, diff):

addressed all @jonatack’s comments

instagibbs commented at 4:25 pm on May 23, 2020: member

reACK https://github.com/bitcoin/bitcoin/pull/16432/commits/206d17e4d7131179e7c5a5e74632806859d7d493

jonatack commented at 11:20 am on May 24, 2020: contributor

ACK 206d17e reviewed changes (thanks!) since my last review of 6920e12, rebased on current master, built and tested on Debian.

 0$ git diff 6920e12 206d17e
 1diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
 2index 9d695f1215..a2fd231c05 100644
 3--- a/src/qt/bitcoingui.cpp
 4+++ b/src/qt/bitcoingui.cpp
 5@@ -352,7 +352,7 @@ void BitcoinGUI::createActions()
 6 
 7     m_mask_values_action = new QAction(tr("&Mask values"), this);
 8     m_mask_values_action->setShortcut(QKeySequence(Qt::CTRL + Qt::SHIFT + Qt::Key_M));
 9-    m_mask_values_action->setStatusTip(tr("Cloak values on Overview tab"));
10+    m_mask_values_action->setStatusTip(tr("Mask the values in the Overview tab"));
11     m_mask_values_action->setCheckable(true);
12 
13     connect(quitAction, &QAction::triggered, qApp, QApplication::quit);
14diff --git a/src/qt/bitcoinunits.cpp b/src/qt/bitcoinunits.cpp
15index cf7051cda8..318a6dcbfd 100644
16--- a/src/qt/bitcoinunits.cpp
17+++ b/src/qt/bitcoinunits.cpp
18@@ -156,9 +156,11 @@ QString BitcoinUnits::formatHtmlWithUnit(int unit, const CAmount& amount, bool p
19 QString BitcoinUnits::formatWithPrivacy(int unit, const CAmount& amount, SeparatorStyle separators, bool privacy)
20 {
21     assert(amount >= 0);
22-    QString value = format(unit, privacy ? 0 : amount, false, separators, true);
23+    QString value;
24     if (privacy) {
25-        value.replace('0', '#');
26+        value = format(unit, 0, false, separators, true).replace('0', '#');
27+    } else {
28+        value = format(unit, amount, false, separators, true);
29     }
30     return value + QString(" ") + shortName(unit);
31 }
32diff --git a/src/qt/bitcoinunits.h b/src/qt/bitcoinunits.h
33index 2d0b85126e..5b8c351d73 100644
34--- a/src/qt/bitcoinunits.h
35+++ b/src/qt/bitcoinunits.h
36@@ -77,7 +77,7 @@ public:
37     static QString formatWithUnit(int unit, const CAmount& amount, bool plussign=false, SeparatorStyle separators=separatorStandard);
38     //! Format as HTML string (with unit)
39     static QString formatHtmlWithUnit(int unit, const CAmount& amount, bool plussign=false, SeparatorStyle separators=separatorStandard);
40-    //! Format as string (with unit) of fixed length to preserve privacy if it set
41+    //! Format as string (with unit) of fixed length to preserve privacy, if it is set.
42     static QString formatWithPrivacy(int unit, const CAmount& amount, SeparatorStyle separators, bool privacy);
43     //! Parse string to coin amount
44     static bool parse(int unit, const QString &value, CAmount *val_out);
45diff --git a/src/qt/overviewpage.cpp b/src/qt/overviewpage.cpp
46index b80fb635ad..48ad438eb2 100644
47--- a/src/qt/overviewpage.cpp
48+++ b/src/qt/overviewpage.cpp
49@@ -165,7 +165,7 @@ void OverviewPage::setPrivacy(bool privacy)
50         ui->listTransactions->setVisible(!m_privacy);
51     }
52 
53-    const QString status_tip = m_privacy ? tr("PRIVACY mode is activated. To reveal cloaked values uncheck Menu->Settings->Mask values") : "";
54+    const QString status_tip = m_privacy ? tr("PRIVACY mode is activated. To unmask the values, uncheck Menu->Settings->Mask values.") : "";
55     setStatusTip(status_tip);
56\```
57</p></details>

hebasto commented at 12:27 pm on May 24, 2020: member

@MarcoFalke Mind re-reviewing this PR?

MarcoFalke commented at 12:33 pm on May 24, 2020: member

Concept 🅰🅲🅺

in src/qt/overviewpage.cpp:160 in 206d17e4d7 outdated

153@@ -152,6 +154,23 @@ void OverviewPage::handleOutOfSyncWarningClicks()
154     Q_EMIT outOfSyncWarningClicked();
155 }
156 
157+void OverviewPage::setPrivacy(bool privacy)
158+{
159+    m_privacy = privacy;
160+    if (walletModel && walletModel->getOptionsModel()) {

promag commented at 0:22 am on May 25, 2020:

This condition looks unnecessary/unrelated, could add a comment maybe?

hebasto commented at 5:59 am on May 25, 2020:

Updated.

in src/qt/overviewpage.cpp:166 in 206d17e4d7 outdated

181@@ -163,25 +182,25 @@ void OverviewPage::setBalance(const interfaces::WalletBalances& balances)
182     m_balances = balances;
183     if (walletModel->wallet().isLegacy()) {
184         if (walletModel->wallet().privateKeysDisabled()) {
185-            ui->labelBalance->setText(BitcoinUnits::formatWithUnit(unit, balances.watch_only_balance, false, BitcoinUnits::separatorAlways));

promag commented at 0:29 am on May 25, 2020:

Could we simplify this? Maybe define a class AmountLabel : public QLabel and here you would just call setAmount(...). And in setWalletModel you could ui->labelBalance->setOptionsModel. This way AmountLabel would call setText when privacy or display unit changes (and amount ofc).

hebasto commented at 5:36 am on May 25, 2020:

Removing a tailored widget was considered as simplification already :)

As @luke-jr rightly commented:

I’m not sure there’s a point to a dedicated widget anymore…

So the dedicated BitcoinAmountLabel widget has been removed.

Anyway, let’s focus this PR on user-faced changes, and leave internal optimization for followup pulls, if you don’t mind :)

in src/qt/overviewpage.h:53 in 206d17e4d7 outdated

49@@ -49,6 +50,7 @@ public Q_SLOTS:
50     ClientModel *clientModel;
51     WalletModel *walletModel;
52     interfaces::WalletBalances m_balances;
53+    bool m_privacy{false};

promag commented at 0:33 am on May 25, 2020:

I think this should be in OptionsModel. No need to go through bitcoingui -> walletframe -> walletview -> overview.

hebasto commented at 5:28 am on May 25, 2020:

I think this should be in OptionsModel.

In this PR the privacy mode state is not intended to be preserved between consequent bitcoin-qt runs, that an OptionsModel instance does.

I’d leave the detailed discussion for a followup pull.

promag commented at 8:29 am on May 25, 2020:

It doesn’t have to be persisted to be in OptionsModel, it’s a runtime option only for now.

hebasto commented at 2:16 pm on May 25, 2020:

Sorry, going to leave it as is.

in src/qt/walletframe.cpp:78 in 206d17e4d7 outdated

73@@ -73,6 +74,10 @@ bool WalletFrame::addWallet(WalletModel *walletModel)
74     connect(walletView, &WalletView::encryptionStatusChanged, gui, &BitcoinGUI::updateWalletStatus);
75     connect(walletView, &WalletView::incomingTransaction, gui, &BitcoinGUI::incomingTransaction);
76     connect(walletView, &WalletView::hdEnabledStatusChanged, gui, &BitcoinGUI::updateWalletStatus);
77+    connect(gui, &BitcoinGUI::setPrivacy, walletView, &WalletView::setPrivacy);
78+    QTimer::singleShot(0, walletView, [this, walletView] {

promag commented at 0:37 am on May 25, 2020:

Why singleShot? Anyway, if you follow the OptionsModel then this is not needed.

hebasto commented at 5:04 am on May 25, 2020:

Why singleShot?

To apply the privacy mode to a just opened wallet.

promag commented at 8:33 am on May 25, 2020:

But this is the same as just calling walletView->setPrivacy(gui->isPrivacyModeActivated()).

hebasto commented at 1:24 pm on May 25, 2020:

But this is the same as just calling walletView->setPrivacy(gui->isPrivacyModeActivated()).

Indeed :) Thank you!

hebasto commented at 2:14 pm on May 25, 2020:

Updated.

hebasto force-pushed on May 25, 2020

hebasto commented at 5:59 am on May 25, 2020: member

Updated 206d17e4d7131179e7c5a5e74632806859d7d493 -> ad5103667d0dc0a8117a2dd99032ecba2107cdd5 (pr16432.18 -> pr16432.19, diff):

addressed @promag’s comment:

This condition looks unnecessary/unrelated, could add a comment maybe?

hebasto force-pushed on May 25, 2020

hebasto commented at 2:14 pm on May 25, 2020: member

Updated ad5103667d0dc0a8117a2dd99032ecba2107cdd5 -> 31be796e44aff80510fb561cc0e8dbd2aa302080 (pr16432.19 -> pr16432.20, diff):

addressed @promag’s comment:

Why singleShot? But this is the same as just calling walletView->setPrivacy(gui->isPrivacyModeActivated()).

in src/qt/bitcoinunits.cpp:158 in 73d8ef7274 outdated

152@@ -150,6 +153,17 @@ QString BitcoinUnits::formatHtmlWithUnit(int unit, const CAmount& amount, bool p
153     return QString("<span style='white-space: nowrap;'>%1</span>").arg(str);
154 }
155 
156+QString BitcoinUnits::formatWithPrivacy(int unit, const CAmount& amount, SeparatorStyle separators, bool privacy)
157+{
158+    assert(amount >= 0);

promag commented at 2:38 pm on May 25, 2020:

73d8ef72742ab9193e9e95158b26176bfaab3f66

nit, any reason for this assertion in this function? Looks like it should be in format()?

hebasto commented at 1:06 pm on May 26, 2020:

73d8ef7

nit, any reason for this assertion in this function? Looks like it should be in format()?

BitcoinUnits::format() handles a negative CAmount argument correctly. But the code of BitcoinUnits::formatWithPrivacy() is not designed to work with the minus sign. This is a deliberate simplification.

in src/qt/bitcoingui.cpp:1412 in 31be796e44 outdated

1406@@ -1398,6 +1407,11 @@ void BitcoinGUI::unsubscribeFromCoreSignals()
1407     m_handler_question->disconnect();
1408 }
1409 
1410+bool BitcoinGUI::isPrivacyModeActivated() const
1411+{
1412+    return m_mask_values_action && m_mask_values_action->isChecked();

promag commented at 2:48 pm on May 25, 2020:

31be796e44aff80510fb561cc0e8dbd2aa302080

ATM m_mask_values_action is always instanced but only added to the GUI when walletFrame is set. But I’d make the instancing optional and here I’d assert(m_mask_values_action) - it should only be called when wallets are enabled.

hebasto commented at 1:57 pm on May 26, 2020:

Updated.

promag commented at 3:15 pm on May 25, 2020: member

Tested ACK 31be796e44aff80510fb561cc0e8dbd2aa302080.

I still think this could have a different implementation, but this PR is already long. Improvements can be made separately so I’d say this could be merged considering we are in early 0.21.

in src/qt/overviewpage.cpp:166 in 31be796e44 outdated

161+        setBalance(m_balances);
162+    }
163+
164+    ui->listTransactions->setVisible(!m_privacy);
165+
166+    const QString status_tip = m_privacy ? tr("PRIVACY mode is activated. To unmask the values, uncheck Menu->Settings->Mask values.") : "";

jonatack commented at 9:31 am on May 26, 2020:

0    const QString status_tip = m_privacy ? tr("Privacy mode is activated for the Overview tab. To unmask the values, uncheck Menu->Settings->Mask values.") : "";

hebasto commented at 1:57 pm on May 26, 2020:

Updated.

jonatack commented at 9:35 am on May 26, 2020: contributor

Tested re-ACK 31be796 with suggestion (here or in a follow-up) to not display “PRIVACY mode” in all caps, and to add “for the Overview tab” to avoid confusion per the principle of least surprise until it is extended to the other tabs as well.

hebasto force-pushed on May 26, 2020

hebasto commented at 1:56 pm on May 26, 2020: member

Updated 31be796e44aff80510fb561cc0e8dbd2aa302080 -> 908b01457bc8454c78fb4d12b7b0d37030f2a6bd (pr16432.20 -> pr16432.21, diff):

addressed @promag’s comment:

ATM m_mask_values_action is always instanced but only added to the GUI when walletFrame is set. But I’d make the instancing optional and here I’d assert(m_mask_values_action) - it should only be called when wallets are enabled.

addressed @jonatack’s comment

Tested re-ACK 31be796 with suggestion (here or in a follow-up) to not display “PRIVACY mode” in all caps, and to add “for the Overview tab” to avoid confusion per the principle of least surprise until it is extended to the other tabs as well.

jonatack commented at 8:50 am on May 27, 2020: contributor

Tested re-ACK 908b01457bc8454c78fb4d12b7b0d37030f2a6bd only changes are addressing feedback (thanks!) as per git diff 31be796 908b014

 0((HEAD detached at origin/pr/16432))$ git diff 31be796 908b014
 1diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
 2index a2fd231c05..321812c1f0 100644
 3--- a/src/qt/bitcoingui.cpp
 4+++ b/src/qt/bitcoingui.cpp
 5@@ -1409,7 +1409,8 @@ void BitcoinGUI::unsubscribeFromCoreSignals()
 6 
 7 bool BitcoinGUI::isPrivacyModeActivated() const
 8 {
 9-    return m_mask_values_action && m_mask_values_action->isChecked();
10+    assert(m_mask_values_action);
11+    return m_mask_values_action->isChecked();
12 }
13 
14 UnitDisplayStatusBarControl::UnitDisplayStatusBarControl(const PlatformStyle *platformStyle) :
15diff --git a/src/qt/overviewpage.cpp b/src/qt/overviewpage.cpp
16index cc521c2acb..51682fe2fc 100644
17--- a/src/qt/overviewpage.cpp
18+++ b/src/qt/overviewpage.cpp
19@@ -163,7 +163,7 @@ void OverviewPage::setPrivacy(bool privacy)
20 
21     ui->listTransactions->setVisible(!m_privacy);
22 
23-    const QString status_tip = m_privacy ? tr("PRIVACY mode is activated. To unmask the values, uncheck Menu->Settings->Mask values.") : "";
24+    const QString status_tip = m_privacy ? tr("Privacy mode is activated for the Overview tab. To unmask the values, uncheck Menu->Settings->Mask values.") : "";
25     setStatusTip(status_tip);
26     QStatusTipEvent event(status_tip);
27     QApplication::sendEvent(this, &event);

Apologetic nit: I belatedly noticed that this change makes the message long enough to be potentially cut off at the end (on my display on Debian). Do we need to write Menu->? If you retouch, maybe consider something like the following. I tested that it fits in the smallest display size (on my display).

0-    const QString status_tip = m_privacy ? tr("Privacy mode is activated for the Overview tab. To unmask the values, uncheck Menu->Settings->Mask values.") : "";
1+    const QString status_tip = m_privacy ? tr("Privacy mode activated for the Overview tab. To unmask the values, uncheck Settings->Mask values.") : "";

qt: Add privacy feature to Overview page 8d75115844

hebasto force-pushed on May 27, 2020

hebasto commented at 8:58 am on May 27, 2020: member

Updated 908b01457bc8454c78fb4d12b7b0d37030f2a6bd -> 8d75115844baefe5cad4d82ec8dce001dd16eb9c (pr16432.21 -> pr16432.22, diff):

addressed @jonatack’s comment:

Apologetic nit: I belatedly noticed that this change makes the message long enough to be potentially cut off at the end (on my display on Debian). Do we need to write Menu->? If you retouch, maybe consider something like the following. I tested that it fits in the smallest display size (on my display).
0-    const QString status_tip = m_privacy ? tr("Privacy mode is activated for the Overview tab. To unmask the values, uncheck Menu->Settings->Mask values.") : "";
1+    const QString status_tip = m_privacy ? tr("Privacy mode activated for the Overview tab. To unmask the values, uncheck Settings->Mask values.") : "";

jonatack commented at 9:20 am on May 27, 2020: contributor

Tested ACK 8d75115

Changes since last review:

 0((HEAD detached at origin/pr/16432))$ git diff 31be796 908b014
 1$ git diff 908b014 8d75115
 2diff --git a/src/qt/overviewpage.cpp b/src/qt/overviewpage.cpp
 3index 51682fe2fc..1ab30fefc7 100644
 4--- a/src/qt/overviewpage.cpp
 5+++ b/src/qt/overviewpage.cpp
 6@@ -163,7 +163,7 @@ void OverviewPage::setPrivacy(bool privacy)
 7 
 8     ui->listTransactions->setVisible(!m_privacy);
 9 
10-    const QString status_tip = m_privacy ? tr("Privacy mode is activated for the Overview tab. To unmask the values, uncheck Menu->Settings->Mask values.") : "";
11+    const QString status_tip = m_privacy ? tr("Privacy mode activated for the Overview tab. To unmask the values, uncheck Settings->Mask values.") : "";

Thanks @hebasto :trophy:

luke-jr commented at 5:07 am on May 28, 2020: member

Apologetic nit: I belatedly noticed that this change makes the message long enough to be potentially cut off at the end (on my display on Debian).

We shouldn’t be assuming or breaking with any window or font size… Is this a bug?

jonatack commented at 7:26 am on May 28, 2020: contributor

@luke-jr Good question. My description was imprecise. When the GUI window was at its smallest size, the message wasn’t truncated per se but required navigating horizontally using the window scroll bar to see the last word.

jonasschnelli commented at 6:54 am on May 29, 2020: contributor

Tested ACK 8d75115844baefe5cad4d82ec8dce001dd16eb9c

jonasschnelli merged this on May 29, 2020

jonasschnelli closed this on May 29, 2020

hebasto deleted the branch on May 29, 2020

MarkLTZ referenced this in commit ff11611320 on May 29, 2020

sidhujag referenced this in commit e94d453411 on May 31, 2020

luke-jr referenced this in commit 62c81dcaa8 on Jun 9, 2020

luke-jr referenced this in commit 2c053892f9 on Jun 9, 2020

MarcoFalke referenced this in commit d0e76b5050 on Dec 17, 2020

laanwj referenced this in commit 7fca189a2a on Feb 22, 2021

Fabcien referenced this in commit 357df3aa24 on Aug 24, 2021

deadalnix referenced this in commit 15c401ce02 on Aug 24, 2021

Fabcien referenced this in commit 37eac8110a on Aug 24, 2021

BenWestgate commented at 0:40 am on August 12, 2022: none

Current Behavior: “Mask values” mode is off each launch and must be manually re-enabled. If a wallet loads on startup from settings.json this leaves user vulnerable to shoulder surfing.

Solution: This mode should persist across restarts of bitcoin-qt by being saved in Bitcoin-Qt.conf. Exactly how nDisplayUnit=0 will persist the unit to show amounts in.

Expected Behavior: If I shutdown Bitcoin core with it on, I’d expect it to be on when I restart and vice versa.

bitcoin locked this on Aug 12, 2023

qt: Add privacy to the Overview page #16432

Conflicts