This is an overhaul of #11383 (plus some additions).
It avoids unnecessary coupling of httpserver/jsonrpc and the wallet as well as it avoids pointer pure passing (and pointer deletion) of CWallet (plus other minor design changes).
Additionally it adds the wallet name to the sendconfirmation and request dialog (in case multiwallet is active)
546+ const QString name = walletModel->getWalletName();
547 setWalletActionsEnabled(true);
548- return walletFrame->addWallet(name, walletModel);
549+ m_wallet_selector->addItem(name);
550+ if (m_wallet_selector->count() == 2) {
551+ m_wallet_selector_label = new QLabel();
755+ return walletName;
756+}
757+
758+bool WalletModel::isMultiwallet()
759+{
760+ return gArgs.GetArgs("-wallet").size() > 1;
m_wallet_models->count(). Maybe we need a WalletsModel class to hold on to a series of wallets?
WalletModel, long term, we may want to have a WalletsManager (see #12587).
1008 QString msg = tr("Date: %1\n").arg(date) +
1009- tr("Amount: %1\n").arg(BitcoinUnits::formatWithUnit(unit, amount, true)) +
1010- tr("Type: %1\n").arg(type);
1011+ tr("Amount: %1\n").arg(BitcoinUnits::formatWithUnit(unit, amount, true));
1012+ if (WalletModel::isMultiwallet() && !walletName.isEmpty()) {
1013+ msg += tr("Wallet: %1\n").arg(walletName);
\n in the Amount: above.
I still think it’s better to use the menu rather than add a dropbox, but as long as it’s easy to change later it doesn’t matter. I left some comments to better separate UI from model to make such a change easier. That already improved in this PR compared to #11383.
Nit: “Loading wallet” should be “Loading wallets” if there’s more than 1.
276@@ -277,8 +277,11 @@ void SendCoinsDialog::on_sendButton_clicked()
277 QStringList formatted;
278 for (const SendCoinsRecipient &rcp : currentTransaction.getRecipients())
279 {
280- // generate bold amount string
281+ // generate bold amount string with wallet name in case of multiwallet
282 QString amount = "<b>" + BitcoinUnits::formatHtmlWithUnit(model->getOptionsModel()->getDisplayUnit(), rcp.amount);
283+ if (model->isMultiwallet()) {
284+ amount.append(" <u>"+tr("from wallet %1").arg(GUIUtil::HtmlEscape(model->getWalletName()))+"</u> ");
1006 // On new transaction, make an info balloon
1007 QString msg = tr("Date: %1\n").arg(date) +
1008- tr("Amount: %1\n").arg(BitcoinUnits::formatWithUnit(unit, amount, true)) +
1009- tr("Type: %1\n").arg(type);
1010+ tr("Amount: %1\n").arg(BitcoinUnits::formatWithUnit(unit, amount, true));
1011+ if (WalletModel::isMultiwallet() && !walletName.isEmpty()) {
{.
Tested ACK.
Receiving addresses and Sending addresses dialogs could have the wallet name in the dialog title.
-disablewallet should not show the wallet selector? Currently looks like:
-disablewallet case (also won’t show the RPCConsole wallet selector if only one wallet is present).
Testing https://github.com/bitcoin/bitcoin/pull/12610/commits/3b156cf01fda530cca127079d630c295d839efc1 on top of master (https://github.com/bitcoin/bitcoin/commit/4ad3b3c72c73d61e0a0cab541dca20acf651320d). macOS 10.13.3, Qt 5.10.1
Wallet Selector:
In the debug console if I try and use the help command with either wallet selected I get an error. Selecting (none) and using help works fine:
743@@ -743,3 +744,18 @@ int WalletModel::getDefaultConfirmTarget() const
744 {
745 return nTxConfirmTarget;
746 }
747+
748+QString WalletModel::getWalletName() const
749+{
750+ LOCK(wallet->cs_wallet);
751+ QString walletName = QString::fromStdString(wallet->GetName());
752+ if (walletName.endsWith(".dat")) {
I’m not sure that we should strip .dat from the wallet name now that #11687 is merged. Perhaps @ryanofsky has an opinion on this?
Also note that new default wallets will have the name "" (the empty string). Perhaps QT should display something different in this case?
EDIT: for now, I think the only way we can have the default wallet with its name set to "" is if we start bitcoin with no wallet arguments, in which case multiwallet isn’t active and the dropdown won’t appear. However, in future we’ll have dynamic wallet loading/unloading, in which case we could have the default "" wallet plus named wallets.
303@@ -303,10 +304,9 @@ bool RPCConsole::RPCParseCommandLine(std::string &strResult, const std::string &
304 req.params = RPCConvertValues(stack.back()[0], std::vector<std::string>(stack.back().begin() + 1, stack.back().end()));
305 req.strMethod = stack.back()[0];
306 #ifdef ENABLE_WALLET
307- // TODO: Move this logic to WalletModel
308- if (!vpwallets.empty()) {
309+ if (walletID && !walletID->empty()) {
310 // in Qt, use always the wallet with index 0 when running with multiple wallets
Tested ACK 3b156cf01fda530cca127079d630c295d839efc1, although I’m not very familiar with the qt code.
To aid review, the commits could be tidied up a little (some code is added and removed in a later commit) and squashed a bit.
A couple of comments inline.
argh, sorry for the late review
this logic here means that anytime we do a wallet match on rpc debug console, it will not find the wallet unless it has no .dat extension: https://github.com/bitcoin/bitcoin/pull/12610/commits/3dba3c3ac1679cf0086ee7734eee12268004ace7#diff-8c9d79ba40bf702f01685008550ac100R491
This also means you can load two wallets(eg wallet.dat and wallet), and they will look the same in the drop down selector.
imo if we’re supporting arbitrary filename extensions we should just display the entire name
imo if we’re supporting arbitrary filename extensions we should just display the entire name
Agree, it would be good to drop the special .dat extension handling (which is now in WalletModel::getWalletName().
.dat extension handling