[Qt] show wallet HD state in statusbar

jonasschnelli commented at 2:32 pm on August 15, 2016: contributor

Shows the HD enabled state in the status bar.

Example

jonasschnelli added the label GUI on Aug 15, 2016

jonasschnelli commented at 2:33 pm on August 15, 2016: contributor

Addresses #8508

jonasschnelli commented at 2:35 pm on August 15, 2016: contributor

The typo in the screenshots is fixed in the code.

in src/qt/walletframe.h: in ad6145bdf7 outdated

29@@ -30,7 +30,7 @@ class WalletFrame : public QFrame
30     void setClientModel(ClientModel *clientModel);
31 
32     bool addWallet(const QString& name, WalletModel *walletModel);
33-    bool setCurrentWallet(const QString& name);
34+    WalletModel* setCurrentWallet(const QString& name);

MarcoFalke commented at 2:49 pm on August 15, 2016:

nit: From the name this sounds like a setter, but it returns a WalletModel. Maybe add documentation why, or change the function name?

jonasschnelli commented at 3:12 pm on August 15, 2016:

Agree that this can be confusing. The setter will just return the referenced WalletModel pointer or NULL in case if the wallet was not found.

Added a comment.

MarcoFalke commented at 2:52 pm on August 15, 2016: member

Concept ACK ad6145b

jonasschnelli force-pushed on Aug 15, 2016

paveljanik commented at 6:08 pm on August 15, 2016: contributor

Travis:

0../../src/qt/rpcconsole.cpp: In constructor ‘RPCConsole::RPCConsole(const PlatformStyle*, QWidget*)’:
1../../src/qt/rpcconsole.cpp:286:9: error: ‘class Ui::RPCConsole’ has no member named ‘walletLabel’
2     ui->walletLabel->hide();
3         ^
4make[2]: *** [qt/qt_libbitcoinqt_a-rpcconsole.o] Error 1
5make[2]: Leaving directory `/home/travis/build/bitcoin/bitcoin/build/src'
6make[1]: *** [install-recursive] Error 1
7make[1]: Leaving directory `/home/travis/build/bitcoin/bitcoin/build/src'
8make: *** [install-recursive] Error 1

Interesting, compiles cleanly here 8) But why if:

0src$ git grep walletLabel
1qt/rpcconsole.cpp:    ui->walletLabel->hide();
2src$

in src/qt/forms/debugwindow.ui: in d95db392df outdated

446@@ -433,12 +447,12 @@
447              <height>24</height>
448             </size>
449            </property>
450-           <property name="text">

paveljanik commented at 6:11 pm on August 15, 2016:

Why this hunk in the diff?

paveljanik commented at 6:29 pm on August 15, 2016: contributor

I like the idea to show HD to the user somewhere.

But isn’t it worth new headline

Wallet Version 130000 HD enabled

or even green or red sign instead of “enabled”/“disabled” there, prominent one. Is the wallet version so important for the user, BTW?

jonasschnelli force-pushed on Aug 16, 2016

jonasschnelli commented at 8:26 am on August 16, 2016: contributor

Fixed the walletLabel compile issue when compiling without the wallet.

jonasschnelli commented at 8:30 am on August 16, 2016: contributor

@paveljanik: I though about adding a new section. But the console window is already large and my goal was to keep it below 480px height (to prevent overflow if you run on a small screen which happens regularly when bootstrapping servers).

Maybe another option would be to create a new window for the wallet informations. That window could also allow verify the used HD seed (maybe exporting), kepool size, encryption state, maybe derive keys at a given keypath, etc.

I’m not sure if the debug window is the right place for wallet informations regarding a possible upcoming support for multi wallet.

MarcoFalke commented at 8:49 am on August 16, 2016: member

Travis failure in libsecp256k1 now unrelated

laanwj commented at 8:55 am on August 16, 2016: member

I’m not sure if the debug window is the right place for wallet informations regarding a possible upcoming support for multi wallet.

I tend to agree with this. This has always been the reason to push back on changes that would add wallet information to the debug window.

I’d suggest this:

The wallet version is not terribly interesting for end-users, and this could remain limited to an RPC call.
HD enabled/disabled is interesting, and IMO does deserve a special icon / mention on the overview page, or in the status bar.

Maybe another option would be to create a new window for the wallet informations. That window could also allow verify the used HD seed (maybe exporting), kepool size, encryption state, maybe derive keys at a given keypath, etc.

Further ahead something like that might be nice.

MarcoFalke commented at 8:56 am on August 16, 2016: member

HD enabled/disabled is interesting, and IMO does deserve a special icon / mention on the overview page, or in the status bar.

Was about to suggest just that.

jonasschnelli force-pushed on Aug 17, 2016

jonasschnelli renamed this:
~~[Qt] show wallet version and HD state in debugwindow~~
[Qt] show wallet HD state in statusbar
on Aug 17, 2016

jonasschnelli commented at 12:35 pm on August 17, 2016: contributor

Changed the PR, removed changes that affected the debug window, added a HD status icon in the statusbar.

jonasschnelli force-pushed on Aug 17, 2016

MarcoFalke commented at 1:06 pm on August 17, 2016: member

Looks good! Hope we can get 4K soon. :)

Issue with build:

0../../src/qt/bitcoingui.cpp:996:84: error: invalid operands to binary expression ('const char *' and 'const char *')
1
2    labelWalletHDStatusIcon->setPixmap(platformStyle->SingleColorIcon(":/icons/hd_"+(hdEnabled ? "en" : "dis")+"abled").pixmap(STATUSBAR_ICONSIZE,STATUSBAR_ICONSIZE));

jonasschnelli force-pushed on Aug 17, 2016

jonasschnelli commented at 1:15 pm on August 17, 2016: contributor

@MarcoFalke thanks for the report. Fixed the compile issue.

paveljanik commented at 8:13 pm on August 18, 2016: contributor

nit: please fix the typo (endabled -> enabled) in the commit message

in src/qt/bitcoingui.cpp: in c022b1d0d3 outdated

990@@ -988,28 +991,37 @@ bool BitcoinGUI::handlePaymentRequest(const SendCoinsRecipient& recipient)
991     return false;
992 }
993 
994+void BitcoinGUI::setHDStatus(int hdEnabled)
995+{
996+    labelWalletHDStatusIcon->setPixmap(platformStyle->SingleColorIcon(hdEnabled ? ":/icons/hd_enabled" : ":/icons/hd_disabled").pixmap(STATUSBAR_ICONSIZE,STATUSBAR_ICONSIZE));
997+    labelWalletHDStatusIcon->setToolTip(tr(hdEnabled ? "HD key generation is <b>enabled</b>" : "HD key generation is <b>disabled</b>"));

paveljanik commented at 8:15 pm on August 18, 2016:

Does this tr work as you expect?

MarcoFalke commented at 9:31 pm on August 18, 2016:

I don’t think so. I am pretty sure you can only translate plain strings extract plain strings for translation.

jonasschnelli commented at 6:31 am on August 19, 2016:

Ah. Right. It will probably translate fine… but the extraction is broken in this case. I’ll fix it.

in src/qt/walletview.cpp: in c022b1d0d3 outdated

 97@@ -98,6 +98,9 @@ void WalletView::setBitcoinGUI(BitcoinGUI *gui)
 98 
 99         // Pass through transaction notifications
100         connect(this, SIGNAL(incomingTransaction(QString,int,CAmount,QString,QString,QString)), gui, SLOT(incomingTransaction(QString,int,CAmount,QString,QString,QString)));
101+
102+        // Connect hd-enabled state signal

paveljanik commented at 8:16 pm on August 18, 2016:

hd -> HD

paveljanik commented at 8:19 pm on August 18, 2016: contributor

Looks brilliant, thanks! Concept ACK Will test later this week.

[Qt] add HD enabled/disabled icon to the status bar 914154f0cc

jonasschnelli force-pushed on Aug 19, 2016

jonasschnelli commented at 7:21 am on August 19, 2016: contributor

Fixed the nits (typo in commit, tr() issue, s/hd/HD).

laanwj commented at 10:00 am on August 19, 2016: member

Works for me: tested with my old testnet wallet, I get the HD disabled icon, tested with a new regtest wallet, get the HD enabled icon. Awesome. Tested ACK

btcdrak commented at 4:46 pm on August 19, 2016: contributor

Tested ACK 914154f

jonasschnelli merged this on Aug 19, 2016

jonasschnelli closed this on Aug 19, 2016

jonasschnelli referenced this in commit 2468292a03 on Aug 19, 2016

in src/qt/walletview.cpp: in 914154f0cc

 97@@ -98,6 +98,9 @@ void WalletView::setBitcoinGUI(BitcoinGUI *gui)
 98 
 99         // Pass through transaction notifications
100         connect(this, SIGNAL(incomingTransaction(QString,int,CAmount,QString,QString,QString)), gui, SLOT(incomingTransaction(QString,int,CAmount,QString,QString,QString)));
101+
102+        // Connect HD enabled state signal 
103+        connect(this, SIGNAL(hdEnabledStatusChanged(int)), gui, SLOT(setHDStatus(int)));

MarcoFalke commented at 5:12 pm on August 19, 2016:

Nit: Any reason to cast the bool to int here?

in src/wallet/wallet.cpp: in 914154f0cc

1232@@ -1233,6 +1233,11 @@ bool CWallet::SetHDChain(const CHDChain& chain, bool memonly)
1233     return true;
1234 }
1235 
1236+bool CWallet::IsHDEnabled()

MarcoFalke commented at 5:12 pm on August 19, 2016:

Nit: can be const?

MarcoFalke commented at 5:13 pm on August 19, 2016: member

Post merge tested ACK 914154f0ccf2a18a273c7fdb3e80016732149303

rebroad commented at 0:48 am on August 21, 2016: contributor

Heh, this pull req is funny as it reminds me when all television manufacturers were keen to put HD on their products, and before then it was HiFi…. now it seems bitcoin-qt has caught the marketing bug :)

luke-jr referenced this in commit b7f42a10e4 on Oct 20, 2016

ghost commented at 3:34 pm on October 23, 2016: none

Hi, why has this request been pulled to master on August 19. but is not in 0.13.1rc2, which has been released 2 months later? Missing the HD icon :(

I am not very familiar with modern developing process with branches, tags etc. I am really curious, for the case that this is on purpose, how You select the peaces from master to a release, or to prevent, as in this example, a PR to come to a release, but stay just in master, to make it probably in a future release? Are all PRs so strictly modular, that You can know, that there are no dependencies missing?? Or how does this work? I would be thankful for a little hint on the release tagging process, or what it’s name is.

MarcoFalke commented at 3:41 pm on October 23, 2016: member

@wodry We only backport bug fixes as per our policy: https://bitcoincore.org/en/lifecycle/#maintenance-releases

However, I would not object to backport this GUI-only feature, if people consider it important and when there is an rc3.

ghost commented at 3:55 pm on October 23, 2016: none

Thanks for Your answer, Marco. Because of the big change with new segwit in 0.13.1, I was not aware, that this is still a “minor” bugfix release regarding non-consensus things. (which really makes sense)

hebasto referenced this in commit 23a7d56df2 on Oct 19, 2021

sidhujag referenced this in commit 48d2d79dd8 on Oct 20, 2021

DrahtBot locked this on Feb 15, 2022

[Qt] show wallet HD state in statusbar #8517