gui: Refactor OpenWalletActivity #16261
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
promag
force-pushed
the
2019-06-refactor-open-wallet
branch
from
a962b68
to
5604d30
Compare
|
@fanquake done. |
|
Concept ACK. |
src/qt/walletcontroller.cpp
Outdated
|
|
||
| if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet)); | ||
|
|
||
| QTimer::singleShot(500, this, &OpenWalletActivity::finish); |
There was a problem hiding this comment.
OpenWalletActivity::finish() could be inlined here.
OpenWalletActivity::open() will remain small enough and readability will be improved, no?
There was a problem hiding this comment.
OpenWalletActivity::finish() could be inlined here.
At the time I had 2 reasons to not do that:
finishcould be called from multiple places, also initially it was a virtual method in the super class;- wanted to avoid nesting lambdas.
There was a problem hiding this comment.
No, this finish() is not thread safe - it would be called from the worker thread.
There was a problem hiding this comment.
... this
finish()is not thread safe - it would be called from the worker thread.
Right. But thread safety is not required for finish(), IMO.
There was a problem hiding this comment.
But it is, see OpenWalletActivity::finish(), it must be called on the GUI thread.
| m_activity_thread.wait(); | ||
| m_activity_thread->quit(); | ||
| m_activity_thread->wait(); | ||
| delete m_activity_worker; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You could use QThread::finished signal. See: https://github.com/bitcoin/bitcoin/pull/16261/files#r303227866
| {} | ||
| WalletControllerActivity::~WalletControllerActivity() | ||
| { | ||
| delete m_progress_dialog; |
There was a problem hiding this comment.
Could naked delete be avoided in the new code?
There was a problem hiding this comment.
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.
promag
force-pushed
the
2019-06-refactor-open-wallet
branch
from
5604d30
to
b71017a
Compare
| @@ -29,15 +33,17 @@ WalletController::WalletController(interfaces::Node& node, const PlatformStyle* | |||
| getOrCreateWallet(std::move(wallet)); | |||
| } | |||
|
|
|||
| m_activity_thread.start(); | |||
| m_activity_worker->moveToThread(m_activity_thread); | |||
There was a problem hiding this comment.
Could add
connect(m_activity_thread, &QThread::finished, m_activity_worker, &QObject::deleteLater);
and remove following
delete m_activity_worker;
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Looks like the wallet encryption test fails... |
|
@jonasschnelli it was failing because of build timeouts. |
promag
force-pushed
the
2019-06-refactor-open-wallet
branch
from
b83c3e1
to
7afbd0c
Compare
promag
force-pushed
the
2019-06-refactor-open-wallet
branch
from
7afbd0c
to
845d57f
Compare
promag
force-pushed
the
2019-06-refactor-open-wallet
branch
from
845d57f
to
bc6d8a3
Compare
|
@hebasto rebased and removed the unused declaration |
This PR consists in 3 refactors:
OpenWalletActivitya base classWalletControllerActivityto simplify new activities, like the upcomingCreateWalletActivity;BitcoinGUIthe details of the wallet opening;WalletControllerActivityinstances 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.