This PR is a subset of the work done in the context of #13100. This change consists in extracting from the application class the code that manages the wallet models.
The role of the WalletController instance is to coordinate wallet operations and the window.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
701@@ -702,7 +702,7 @@ void RPCConsole::addWallet(WalletModel * const walletModel)
702 // First wallet added, set to default so long as the window isn't presently visible (and potentially in use)
703 ui->WalletSelector->setCurrentIndex(1);
704 }
705- if (ui->WalletSelector->count() > 2) {
2 in the past?
unloadwallet with or without a wallet.
49+private:
50+ interfaces::Node& m_node;
51+ const PlatformStyle* const m_platform_style;
52+ OptionsModel* const m_options_model;
53+ std::vector<WalletModel*> m_wallets;
54+ std::list<WalletModel*> m_history;
m_history?
Sorry, should have some comment there. I’m also willing to remove this now and add it later.
This is like the stack of wallets, the smaller the index the most recent viewed wallet. With this when the user closes a the current wallet it goes to the previous used wallet.
tACK 23aae3d on macOS 10.14.2 with QT 5.11.2, also with --disable-wallet, with and without BIP70.
I’m not sure how to test unloading of wallets; it tends to return “unable to”.
Without wallet there’s a warning:
0qt/rpcconsole.cpp:908:22: warning: unused variable 'wallet_model' [-Wunused-variable]
1 WalletModel* wallet_model{nullptr};
#14784 is merged now, so you can edit the top comment
Maybe move qRegisterMetaType into the controller so you can remove the walletmodel.h dependency from qt/bitcoin.cpp?
re-tACK 5e4cb35, it seems to fix the crashes we discussed on IRC.
I noticed you removed the qRegisterMetaType<WalletModel> line completely. What was its purpose? And why can this one be removed but not the other ones?
573+void BitcoinGUI::addWallet(WalletModel* walletModel)
574 {
575- if(!walletFrame)
576- return false;
577- const QString display_name = walletModel->getDisplayName();
578+ assert(walletFrame);
walletFrame is set and where addWallet() is called), but why not just return early to minimize code change?
483@@ -483,7 +484,10 @@ void BitcoinGUI::createToolBars()
484 toolbar->addWidget(spacer);
485
486 m_wallet_selector = new QComboBox();
487- connect(m_wallet_selector, static_cast<void (QComboBox::*)(int)>(&QComboBox::currentIndexChanged), this, &BitcoinGUI::setCurrentWalletBySelectorIndex);
488+ connect(m_wallet_selector, static_cast<void (QComboBox::*)(int)>(&QComboBox::currentIndexChanged), [this](int index) {
50+ interfaces::Node& m_node;
51+ const PlatformStyle* const m_platform_style;
52+ OptionsModel* const m_options_model;
53+ std::vector<WalletModel*> m_wallets;
54+ std::list<WalletModel*> m_history;
55+ BitcoinGUI* m_window{nullptr};
45+ void currentWalletChanged(WalletModel* wallet_model);
46+
47+ void coinsSent(WalletModel* wallet_model, SendCoinsRecipient recipient, QByteArray transaction);
48+
49+private:
50+ interfaces::Node& m_node;
m_node, m_platform_style, m_options_model and m_handler_load_wallet need to be member variables?
WalletModel instances. And m_handler_load_wallet is needed to keep the signal/slot connection.
Looks good. A few comments and questions inline.
I agree with Sjors that m_history is unnecessary and should be dropped from this PR.
37+ wallet_models.push_back(it.second);
38+ }
39+ return wallet_models;
40+}
41+
42+std::vector<std::string> WalletController::getWalletsAvailableToLoad() const
Lightly tested ACK 89fa546e8211aa47c96e20ba2a80507acd125539.
One question inline.
614@@ -599,13 +615,19 @@ void BitcoinGUI::setCurrentWallet(WalletModel* wallet_model)
615 {
616 if (!walletFrame) return;
617 walletFrame->setCurrentWallet(wallet_model);
618+ for (int index = 0; index < m_wallet_selector->count(); ++index) {
Nit: ->itemData() can give you an iterator, which you can use for the loop, to prevent out of bounds accidents: http://doc.qt.io/qt-5/qvariant.html
Maybe it also has a helper to just find the index you need directly.
21+class Handler;
22+class Node;
23+} // namespace interfaces
24+
25+/**
26+ * WalletController
23+} // namespace interfaces
24+
25+/**
26+ * WalletController
27+ */
28+class WalletController : public QObject
tACK 1316f57
By the way, I’m seeing the following warning when compiling with --disable-wallet, probably from a previous commit:
0qt/rpcconsole.cpp:909:22: warning: unused variable 'wallet_model' [-Wunused-variable]
1 WalletModel* wallet_model{nullptr};
2 ^
@Sjors in #15150 https://github.com/bitcoin/bitcoin/pull/15150/commits/b669ba6bd86a7808e4692bb0cf692b1c95d8d32d that warning is removed.
Thanks for reviewing!
Tested ACK 1316f5706891bb94428078e874e761931ff0e087
Only difference is rebasing on master and removing the unused WalletController::getWalletsAvailableToLoad()
EDIT: pasted incorrect commit id. Now fixed
54+
55+ connect(wallet_model, &WalletModel::unload, [this, it, wallet_model] {
56+ // Unregister wallet model and update current if necessary.
57+ {
58+ QMutexLocker locker(&m_mutex);
59+ m_wallet_models.erase(it);
m_wallet_models did had additions or deletions during time of iteration creation and inner-usage?
375@@ -376,7 +376,7 @@ bool WalletModel::changePassphrase(const SecureString &oldPass, const SecureStri
376 static void NotifyUnload(WalletModel* walletModel)
377 {
378 qDebug() << "NotifyUnload";
379- QMetaObject::invokeMethod(walletModel, "unload", Qt::QueuedConnection);
380+ QMetaObject::invokeMethod(walletModel, "unload");
Can you elaborate on why this is safe? I’m still wrapping my head around how QT deals with threads: http://doc.qt.io/qt-5/qobject.html#thread-affinity
Is it similar to iOs where the UI can only be touched from the main thread, but notifications tend to arrive on different threads, leading to nasty crashes if you don’t pay attention? How do you know which thread NotifyUnload is called on?
I think https://woboq.com/blog/how-qt-signals-slots-work.html is a good read.
Each QObject belongs to one thread which must have one event loop spinning. With the default AutoConnection Qt ensures that the slot is executed in the receiver thread, so it either calls the slot directly (if the emit happens in the same thread) or enqueues an event (with all signal arguments) in the receiver event loop.