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.
Split in multiple commits for easier review.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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) {
Why was this 2 in the past?
Before the wallet selector would be visible with only 2 wallets. However I think it makes sense to show the selector even with 1 wallet only because otherwise you can't decide to execute 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;
I'm confused, what is this 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.
I have a light preference for dropping that functionality, unless someone is willing to write tests for it. I don't expect users to be loading and unloading so often that a lack of history gets annoying.
Agree that this is unnecessary.
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:
qt/rpcconsole.cpp:908:22: warning: unused variable 'wallet_model' [-Wunused-variable]
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);
I think this and the other assertions are ok (I've gone through and checked where walletFrame is set and where addWallet() is called), but why not just return early to minimize code change?
I'll leave these changes for later with a good rationale.
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) {
I'm not sure if this lambda makes this any clearer. What's the motivation for this change?
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};
is this used?
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;
do m_node, m_platform_style, m_options_model and m_handler_load_wallet need to be member variables?
Yes, to create 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
Is this unused?
I'd recommend moving it there. I think it's best not to have unused code in PRs.
Lightly tested ACK 89fa546e8211aa47c96e20ba2a80507acd125539.
One question inline.
Rebased.
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
Explain in more detail (in a followup PR) what this controller is for.
Fair enough, I'll bring more code here so I can also improve this.
23 | +} // namespace interfaces 24 | + 25 | +/** 26 | + * WalletController 27 | + */ 28 | +class WalletController : public QObject
Nit: WalletsController?
heh 😛avoid plural?
Plural makes it clear that this controls multiple wallets, rather than one controller per wallet.
I prefer to interpret it as the "gui controller of the wallet related things". Let's see what others say and I'll happily rename.
I think either is fine.
Tech language prefers a singular form; e.g., "window manager", "file manager" etc.
tACK 1316f57
By the way, I'm seeing the following warning when compiling with --disable-wallet, probably from a previous commit:
qt/rpcconsole.cpp:909:22: warning: unused variable 'wallet_model' [-Wunused-variable]
WalletModel* wallet_model{nullptr};
^
@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);
Is it safe to transport the iterator from outside the lambda to this point? Assume m_wallet_models did had additions or deletions during time of iteration creation and inner-usage?
From https://stackoverflow.com/a/6442829 I think it is safe, map iterator aren't invalidated.
I'm going to change this to simplify other PR (close wallet action) I'm about to submit.
Refactored a bit to simplify follow ups.
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.
re-tACK 0dd9bde
utACK 0dd9bdefa1427746ae94b4dd3da1a5257c1ec68e
Tested ACK 0dd9bdefa1427746ae94b4dd3da1a5257c1ec68e