This PR consists in 3 refactors:
OpenWalletActivity a base class WalletControllerActivity to simplify new activities, like the upcoming CreateWalletActivity;BitcoinGUI the details of the wallet opening;WalletControllerActivity instances belong to the GUI thread and some code (the blocking code) is now executed in the worker thread asynchronously, which allows for a responsive GUI.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
211+{
212+ QString name = path.empty() ? QString("["+tr("default wallet")+"]") : QString::fromStdString(path);
213+
214+ showProgressDialog(tr("Opening Wallet <b>%1</b>...").arg(name.toHtmlEscaped()));
215+
216+ QTimer::singleShot(500, worker(), [this, path] {
We can ditch these 500ms but I think it is ok.
If we decide to have 0ms then we can only replace QTimer::singleShot(0, with QMetaObject::invokeMethod after bumping Qt to 5.10 - see https://doc.qt.io/qt-5/qmetaobject.html#invokeMethod-5.
QTimer::singleShot() should be avoided, IMO.
worker() thread.
The point is to call the lambda in the
worker()thread.
Did you consider some other approaches?
216+ QTimer::singleShot(500, worker(), [this, path] {
217+ std::unique_ptr<interfaces::Wallet> wallet = node().loadWallet(path, m_error_message, m_warning_message);
218+
219+ if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
220+
221+ QTimer::singleShot(500, this, &OpenWalletActivity::finish);
OpenWalletActivity::finish() could be inlined here.
OpenWalletActivity::open() will remain small enough and readability will be improved, no?
OpenWalletActivity::finish() could be inlined here.
At the time I had 2 reasons to not do that:
finish could be called from multiple places, also initially it was a virtual method in the super class;
Could it be just
0 finish();
?
finish() is not thread safe - it would be called from the worker thread.
… this
finish()is not thread safe - it would be called from the worker thread.
Right. But thread safety is not required for finish(), IMO.
OpenWalletActivity::finish(), it must be called on the GUI thread.
212+ QString name = path.empty() ? QString("["+tr("default wallet")+"]") : QString::fromStdString(path);
213+
214+ showProgressDialog(tr("Opening Wallet <b>%1</b>...").arg(name.toHtmlEscaped()));
215+
216+ QTimer::singleShot(500, worker(), [this, path] {
217+ std::unique_ptr<interfaces::Wallet> wallet = node().loadWallet(path, m_error_message, m_warning_message);
0auto wallet = node().loadWallet(path, m_error_message, m_warning_message);
44 {
45- m_activity_thread.quit();
46- m_activity_thread.wait();
47+ m_activity_thread->quit();
48+ m_activity_thread->wait();
49+ delete m_activity_worker;
delete be avoided in the new code? There are some techniques to achieve this.
E.g., m_activity_worker could have a parent QObject, like m_activity_thread has.
m_activity_worker parent because it’s the only object on its thread. Could connect WalletController::destroyed to its deleteLater or could use some smart pointer but honestly I think this is better.
QThread::finished signal. See: https://github.com/bitcoin/bitcoin/pull/16261/files#r303227866
157- : m_wallet_controller(wallet_controller)
158- , m_name(name)
159-{}
160+WalletControllerActivity::~WalletControllerActivity()
161+{
162+ delete m_progress_dialog;
delete be avoided in the new code?
m_parent_widget so that it shows up properly. If the activity finishes first then we delete it - note lines L174-L176 below.
32@@ -29,15 +33,17 @@ WalletController::WalletController(interfaces::Node& node, const PlatformStyle*
33 getOrCreateWallet(std::move(wallet));
34 }
35
36- m_activity_thread.start();
37+ m_activity_worker->moveToThread(m_activity_thread);
Could add
0 connect(m_activity_thread, &QThread::finished, m_activity_worker, &QObject::deleteLater);
and remove following
0 delete m_activity_worker;
?
~WalletController() so what better place to also delete the worker? I think explicit teardown is preferable.
Why?
Your approach binds the life of m_activity_worker to the WalletController instance lifetime.
IMO, as m_activity_worker lives in m_activity_thread, connecting to the QThread::finished signal seems more appropriate.
176- if (wallet) {
177- Q_EMIT opened(m_wallet_controller->getOrCreateWallet(std::move(wallet)));
178- } else {
179- Q_EMIT message(QMessageBox::Critical, QString::fromStdString(error));
180+ m_progress_dialog = new QProgressDialog(m_parent_widget);
181+
Could add
0 connect(this, &OpenWalletActivity::finished, m_progress_dialog, &QObject::deleteLater);
and remove above:
0 delete m_progress_dialog;
?
184+ m_progress_dialog->setCancelButton(nullptr);
185+ m_progress_dialog->setWindowModality(Qt::ApplicationModal);
186+ GUIUtil::PolishProgressDialog(m_progress_dialog);
187+
188+ connect(m_progress_dialog, &QObject::destroyed, [this] {
189+ m_progress_dialog = nullptr;
setAttribute(Qt::WA_DeleteOnClose). Removed.