Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gui: Refactor OpenWalletActivity #16261

Open
wants to merge 2 commits into
base: master
from

Conversation

@promag
Copy link
Member

commented Jun 21, 2019

This PR consists in 3 refactors:

  1. Split from OpenWalletActivity a base class WalletControllerActivity to simplify new activities, like the upcoming CreateWalletActivity;
  2. Move from BitcoinGUI the details of the wallet opening;
  3. Change threading model - 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.
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16661 (wallet: Fix segfault in CreateWalletFromFile, Pass error to rpc caller by MarcoFalke)
  • #15450 ([GUI] Create wallet menu option by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.


showProgressDialog(tr("Opening Wallet <b>%1</b>...").arg(name.toHtmlEscaped()));

QTimer::singleShot(500, worker(), [this, path] {

This comment has been minimized.

Copy link
@promag

promag Jun 21, 2019

Author Member

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.

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 7, 2019

Member

What is the point to pause?
If there is no clear and obvious rationale, QTimer::singleShot() should be avoided, IMO.

This comment has been minimized.

Copy link
@promag

promag Jul 13, 2019

Author Member

No point, but not harm, can make it 0. The point is to call the lambda in the worker() thread.

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 13, 2019

Member

The point is to call the lambda in the worker() thread.

Did you consider some other approaches?

This comment has been minimized.

Copy link
@promag

promag Jul 13, 2019

Author Member

Yes, those require more QObject subclass, signals and slots. IMO this approach is more clear.

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 13, 2019

Member

May I suggest to remove (make 0 ms) or comment 500 ms timeout?

This comment has been minimized.

Copy link
@promag

promag Jul 13, 2019

Author Member

Sure, I can make 0ms and leave a comment.

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jul 16, 2019

Member

Is there a benefit of using 500ms? Seems racy...

@fanquake

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

@promag Can you rebase this on master; as it contains some bug fixes that would be nice to have when testing this PR, such as #16231.

@promag promag force-pushed the promag:2019-06-refactor-open-wallet branch from a962b68 to 5604d30 Jul 5, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

@fanquake done.

@hebasto

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Concept ACK.

@fanquake

This comment has been minimized.

Copy link
Member

commented Jul 6, 2019

I had a quick test, and this PR on top of master (4f378ac) doesn't fix #15453.


if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));

QTimer::singleShot(500, this, &OpenWalletActivity::finish);

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 7, 2019

Member

Same here.

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 13, 2019

Member

OpenWalletActivity::finish() could be inlined here.
OpenWalletActivity::open() will remain small enough and readability will be improved, no?

This comment has been minimized.

Copy link
@promag

promag Jul 13, 2019

Author Member

OpenWalletActivity::finish() could be inlined here.

At the time I had 2 reasons to not do that:

  1. finish could be called from multiple places, also initially it was a virtual method in the super class;
  2. wanted to avoid nesting lambdas.

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 14, 2019

Member

Could it be just

        finish();

?

This comment has been minimized.

Copy link
@promag

promag Jul 14, 2019

Author Member

No, this finish() is not thread safe - it would be called from the worker thread.

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 14, 2019

Member

... this finish() is not thread safe - it would be called from the worker thread.

Right. But thread safety is not required for finish(), IMO.


showProgressDialog(tr("Opening Wallet <b>%1</b>...").arg(name.toHtmlEscaped()));

QTimer::singleShot(500, worker(), [this, path] {

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 7, 2019

Member

What is the point to pause?
If there is no clear and obvious rationale, QTimer::singleShot() should be avoided, IMO.

m_activity_thread.wait();
m_activity_thread->quit();
m_activity_thread->wait();
delete m_activity_worker;

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 7, 2019

Member

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.

This comment has been minimized.

Copy link
@promag

promag Jul 13, 2019

Author Member

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.

This comment has been minimized.

Copy link
@hebasto
{}
WalletControllerActivity::~WalletControllerActivity()
{
delete m_progress_dialog;

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 7, 2019

Member

Could naked delete be avoided in the new code?

This comment has been minimized.

Copy link
@promag

promag Jul 13, 2019

Author Member

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.

This comment has been minimized.

Copy link
@hebasto

@promag promag force-pushed the promag:2019-06-refactor-open-wallet branch from 5604d30 to b71017a Jul 8, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jul 9, 2019

@@ -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);

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 14, 2019

Member

Could add

    connect(m_activity_thread, &QThread::finished, m_activity_worker, &QObject::deleteLater);

and remove following

   delete m_activity_worker;

?

This comment has been minimized.

Copy link
@promag

promag Jul 16, 2019

Author Member

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.

This comment has been minimized.

Copy link
@hebasto

hebasto Jul 18, 2019

Member

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.

src/qt/walletcontroller.cpp Show resolved Hide resolved
src/qt/walletcontroller.cpp Outdated Show resolved Hide resolved
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

Looks like the wallet encryption test fails...

@promag

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

@jonasschnelli it was failing because of build timeouts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.