[Qt] show wallet HD state in statusbar #8517

Merged
merged 1 commit into from Aug 19, 2016

Projects

None yet

7 participants

@jonasschnelli
Member
jonasschnelli commented Aug 15, 2016 edited

Shows the HD enabled state in the status bar.

Example
bildschirmfoto 2016-08-17 um 14 10 36

bildschirmfoto 2016-08-17 um 14 11 09

@jonasschnelli jonasschnelli added the GUI label Aug 15, 2016
@jonasschnelli
Member

Addresses #8508

@jonasschnelli
Member

The typo in the screenshots is fixed in the code.

@MarcoFalke MarcoFalke and 1 other commented on an outdated diff Aug 15, 2016
src/qt/walletframe.h
@@ -30,7 +30,7 @@ class WalletFrame : public QFrame
void setClientModel(ClientModel *clientModel);
bool addWallet(const QString& name, WalletModel *walletModel);
- bool setCurrentWallet(const QString& name);
+ WalletModel* setCurrentWallet(const QString& name);
@MarcoFalke
MarcoFalke Aug 15, 2016 Member

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

@jonasschnelli
jonasschnelli Aug 15, 2016 Member

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
Member

Concept ACK ad6145b

@paveljanik
Contributor
paveljanik commented Aug 15, 2016 edited

Travis:

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

Interesting, compiles cleanly here 8) But why if:

src$ git grep walletLabel
qt/rpcconsole.cpp:    ui->walletLabel->hide();
src$
@paveljanik paveljanik commented on an outdated diff Aug 15, 2016
src/qt/forms/debugwindow.ui
@@ -433,12 +447,12 @@
<height>24</height>
</size>
</property>
- <property name="text">
@paveljanik
paveljanik Aug 15, 2016 Contributor

Why this hunk in the diff?

@paveljanik
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
Member

Fixed the walletLabel compile issue when compiling without the wallet.

@jonasschnelli
Member

@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
Member

Travis failure in libsecp256k1 now unrelated

@laanwj
Member
laanwj commented Aug 16, 2016 edited

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
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 jonasschnelli changed the title from [Qt] show wallet version and HD state in debugwindow to [Qt] show wallet HD state in statusbar Aug 17, 2016
@jonasschnelli
Member

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

bildschirmfoto 2016-08-17 um 14 10 36

bildschirmfoto 2016-08-17 um 14 11 09

@MarcoFalke
Member

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

Issue with build:

../../src/qt/bitcoingui.cpp:996:84: error: invalid operands to binary expression ('const char *' and 'const char *')

    labelWalletHDStatusIcon->setPixmap(platformStyle->SingleColorIcon(":/icons/hd_"+(hdEnabled ? "en" : "dis")+"abled").pixmap(STATUSBAR_ICONSIZE,STATUSBAR_ICONSIZE));


@jonasschnelli
Member

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

@paveljanik
Contributor

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

@paveljanik paveljanik and 2 others commented on an outdated diff Aug 18, 2016
src/qt/bitcoingui.cpp
@@ -988,28 +991,37 @@ bool BitcoinGUI::handlePaymentRequest(const SendCoinsRecipient& recipient)
return false;
}
+void BitcoinGUI::setHDStatus(int hdEnabled)
+{
+ labelWalletHDStatusIcon->setPixmap(platformStyle->SingleColorIcon(hdEnabled ? ":/icons/hd_enabled" : ":/icons/hd_disabled").pixmap(STATUSBAR_ICONSIZE,STATUSBAR_ICONSIZE));
+ labelWalletHDStatusIcon->setToolTip(tr(hdEnabled ? "HD key generation is <b>enabled</b>" : "HD key generation is <b>disabled</b>"));
@paveljanik
paveljanik Aug 18, 2016 Contributor

Does this tr work as you expect?

@MarcoFalke
MarcoFalke Aug 18, 2016 Member

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

@jonasschnelli
jonasschnelli Aug 19, 2016 Member

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

@paveljanik paveljanik commented on an outdated diff Aug 18, 2016
src/qt/walletview.cpp
@@ -98,6 +98,9 @@ void WalletView::setBitcoinGUI(BitcoinGUI *gui)
// Pass through transaction notifications
connect(this, SIGNAL(incomingTransaction(QString,int,CAmount,QString,QString,QString)), gui, SLOT(incomingTransaction(QString,int,CAmount,QString,QString,QString)));
+
+ // Connect hd-enabled state signal
@paveljanik
paveljanik Aug 18, 2016 Contributor

hd -> HD

@paveljanik
Contributor

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

@jonasschnelli jonasschnelli [Qt] add HD enabled/disabled icon to the status bar 914154f
@jonasschnelli
Member

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

@laanwj
Member
laanwj commented Aug 19, 2016 edited

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
Member
btcdrak commented Aug 19, 2016

Tested ACK 914154f

@jonasschnelli jonasschnelli merged commit 914154f into bitcoin:master Aug 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jonasschnelli jonasschnelli added a commit that referenced this pull request Aug 19, 2016
@jonasschnelli jonasschnelli Merge #8517: [Qt] show wallet HD state in statusbar
914154f [Qt] add HD enabled/disabled icon to the status bar (Jonas Schnelli)
2468292
@MarcoFalke MarcoFalke commented on the diff Aug 19, 2016
src/qt/walletview.cpp
@@ -98,6 +98,9 @@ void WalletView::setBitcoinGUI(BitcoinGUI *gui)
// Pass through transaction notifications
connect(this, SIGNAL(incomingTransaction(QString,int,CAmount,QString,QString,QString)), gui, SLOT(incomingTransaction(QString,int,CAmount,QString,QString,QString)));
+
+ // Connect HD enabled state signal
+ connect(this, SIGNAL(hdEnabledStatusChanged(int)), gui, SLOT(setHDStatus(int)));
@MarcoFalke
MarcoFalke Aug 19, 2016 Member

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

@MarcoFalke MarcoFalke commented on the diff Aug 19, 2016
src/wallet/wallet.cpp
@@ -1233,6 +1233,11 @@ bool CWallet::SetHDChain(const CHDChain& chain, bool memonly)
return true;
}
+bool CWallet::IsHDEnabled()
@MarcoFalke
MarcoFalke Aug 19, 2016 Member

Nit: can be const?

@MarcoFalke
Member

Post merge tested ACK 914154f

@rebroad
Contributor
rebroad commented Aug 21, 2016

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 luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [Qt] add HD enabled/disabled icon to the status bar
Github-Pull: #8517
Rebased-From: 914154f
b7f42a1
@wodry
Contributor
wodry commented Oct 23, 2016 edited

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

@wodry
Contributor
wodry commented Oct 23, 2016

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment