Shows the HD enabled state in the status bar.
Example <img width="220" alt="bildschirmfoto 2016-08-17 um 14 10 36" src="https://cloud.githubusercontent.com/assets/178464/17735762/632e7686-6484-11e6-83cf-da3fd49e514a.png">
<img width="114" alt="bildschirmfoto 2016-08-17 um 14 11 09" src="https://cloud.githubusercontent.com/assets/178464/17735778/76bb4634-6484-11e6-9a87-e5ad97326aae.png">
Addresses #8508
The typo in the screenshots is fixed in the code.
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);
nit: From the name this sounds like a setter, but it returns a WalletModel. Maybe add documentation why, or change the function name?
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.
Concept ACK ad6145b
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$
446 | @@ -433,12 +447,12 @@ 447 | <height>24</height> 448 | </size> 449 | </property> 450 | - <property name="text">
Why this hunk in the diff?
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?
Fixed the walletLabel compile issue when compiling without the wallet.
@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.
Travis failure in libsecp256k1 now unrelated
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:
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.
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.
Changed the PR, removed changes that affected the debug window, added a HD status icon in the statusbar.
<img width="220" alt="bildschirmfoto 2016-08-17 um 14 10 36" src="https://cloud.githubusercontent.com/assets/178464/17736383/cfccaa26-6487-11e6-9e08-e5bfac4afb43.png"> <img width="114" alt="bildschirmfoto 2016-08-17 um 14 11 09" src="https://cloud.githubusercontent.com/assets/178464/17736384/cfedbcca-6487-11e6-86dd-b0c790a8a823.png">
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));
@MarcoFalke thanks for the report. Fixed the compile issue.
nit: please fix the typo (endabled -> enabled) in the commit message
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>"));
Does this tr work as you expect?
I don't think so. I am pretty sure you can only <strike>translate plain strings</strike> extract plain strings for translation.
Ah. Right. It will probably translate fine... but the extraction is broken in this case. I'll fix it.
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
hd -> HD
Looks brilliant, thanks! Concept ACK Will test later this week.
Fixed the nits (typo in commit, tr() issue, s/hd/HD).
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
Tested ACK 914154f
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)));
Nit: Any reason to cast the bool to int here?
1232 | @@ -1233,6 +1233,11 @@ bool CWallet::SetHDChain(const CHDChain& chain, bool memonly) 1233 | return true; 1234 | } 1235 | 1236 | +bool CWallet::IsHDEnabled()
Nit: can be const?
Post merge tested ACK 914154f0ccf2a18a273c7fdb3e80016732149303
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 :)
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.
@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.
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)