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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
What is the point to pause?
If there is no clear and obvious rationale, QTimer::singleShot() should be avoided, IMO.
No point, but not harm, can make it 0. The point is to call the lambda in the worker() thread.
The point is to call the lambda in the
worker()thread.
Did you consider some other approaches?
Yes, those require more QObject subclass, signals and slots. IMO this approach is more clear.
May I suggest to remove (make 0 ms) or comment 500 ms timeout?
Sure, I can make 0ms and leave a comment.
Is there a benefit of using 500ms? Seems racy...
Not really, I'll make 0 then.
Concept ACK.
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);
Same here.
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
finish();
?
No, this 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.
But it is, see OpenWalletActivity::finish(), it must be called on the GUI thread.
Correct.
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);
auto wallet = node().loadWallet(path, m_error_message, m_warning_message);
I prefer to have to have the type.
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;
Could naked 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.
No object could be 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.
You could use 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;
Could naked delete be avoided in the new code?
Ownership is given to m_parent_widget so that it shows up properly. If the activity finishes first then we delete it - note lines L174-L176 below.
See: #16261 (review)
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
connect(m_activity_thread, &QThread::finished, m_activity_worker, &QObject::deleteLater);
and remove following
delete m_activity_worker;
?
Why? The thread is explicitly stopped (actually the event loop is stopped and the thread finishes) in ~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
connect(this, &OpenWalletActivity::finished, m_progress_dialog, &QObject::deleteLater);
and remove above:
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;
Which cases require this line?
Actually I think it's not required now. Initially I had setAttribute(Qt::WA_DeleteOnClose). Removed.
Looks like the wallet encryption test fails...
@jonasschnelli it was failing because of build timeouts.
ACK 7afbd0cb69dae4af4b6a78c442c25832cd7fad42, I have tested the code: no change in behavior is observed (compared to the master 6519be605480fec95dcd814771038efcb1ad2abe).