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: Add WalletController #15101

Merged
merged 3 commits into from Jan 18, 2019

Conversation

Projects
None yet
7 participants
@promag
Copy link
Member

promag commented Jan 4, 2019

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.

@fanquake fanquake added the GUI label Jan 4, 2019

@promag promag force-pushed the promag:2019-01-walletcontroller branch 3 times, most recently Jan 4, 2019

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Jan 4, 2019

Split in multiple commits for easier review.

@promag promag force-pushed the promag:2019-01-walletcontroller branch 4 times, most recently Jan 4, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jan 5, 2019

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

Conflicts

No conflicts as of last run.

@Sjors

Sjors approved these changes Jan 7, 2019

Copy link
Member

Sjors left a comment

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?

Show resolved Hide resolved src/qt/rpcconsole.cpp Outdated
Show resolved Hide resolved src/qt/walletcontroller.h Outdated

@promag promag force-pushed the promag:2019-01-walletcontroller branch Jan 7, 2019

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Jan 9, 2019

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?

@jnewbery
Copy link
Member

jnewbery left a comment

Looks good. A few comments and questions inline.

I agree with Sjors that m_history is unnecessary and should be dropped from this PR.

Show resolved Hide resolved src/qt/bitcoingui.cpp Outdated
Show resolved Hide resolved src/qt/bitcoingui.cpp Outdated
Show resolved Hide resolved src/qt/walletcontroller.h Outdated
Show resolved Hide resolved src/qt/walletcontroller.h Outdated
Show resolved Hide resolved src/qt/walletcontroller.h

@promag promag force-pushed the promag:2019-01-walletcontroller branch Jan 12, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 12, 2019

@promag promag force-pushed the promag:2019-01-walletcontroller branch Jan 12, 2019

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Jan 12, 2019

Rebased with #15149 to avoid conflicts.

@promag promag force-pushed the promag:2019-01-walletcontroller branch Jan 12, 2019

@promag promag referenced this pull request Jan 12, 2019

Merged

gui: Add Open Wallet menu #15153

@promag promag force-pushed the promag:2019-01-walletcontroller branch 4 times, most recently Jan 12, 2019

@jnewbery
Copy link
Member

jnewbery left a comment

Lightly tested ACK 89fa546e8211aa47c96e20ba2a80507acd125539.

One question inline.

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

@promag promag force-pushed the promag:2019-01-walletcontroller branch Jan 14, 2019

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Jan 14, 2019

@jnewbery done.

@DrahtBot DrahtBot removed the Needs rebase label Jan 14, 2019

@promag promag force-pushed the promag:2019-01-walletcontroller branch Jan 14, 2019

@promag promag force-pushed the promag:2019-01-walletcontroller branch Jan 15, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 15, 2019

@promag promag force-pushed the promag:2019-01-walletcontroller branch Jan 15, 2019

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Jan 15, 2019

Will re-test after #15149 rebase, but looking good.

@promag promag force-pushed the promag:2019-01-walletcontroller branch Jan 16, 2019

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Jan 16, 2019

Rebased.

@Sjors

Sjors approved these changes Jan 16, 2019

Copy link
Member

Sjors left a comment

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};
                     ^
@@ -599,13 +615,19 @@ void BitcoinGUI::setCurrentWallet(WalletModel* wallet_model)
{
if (!walletFrame) return;
walletFrame->setCurrentWallet(wallet_model);
for (int index = 0; index < m_wallet_selector->count(); ++index) {

This comment has been minimized.

@Sjors

Sjors Jan 16, 2019

Member

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.

Show resolved Hide resolved src/qt/walletcontroller.h Outdated
/**
* WalletController
*/
class WalletController : public QObject

This comment has been minimized.

@Sjors

Sjors Jan 16, 2019

Member

Nit: WalletsController?

This comment has been minimized.

@promag

promag Jan 16, 2019

Author Member

heh 😛avoid plural?

This comment has been minimized.

@Sjors

Sjors Jan 16, 2019

Member

Plural makes it clear that this controls multiple wallets, rather than one controller per wallet.

This comment has been minimized.

@promag

promag Jan 16, 2019

Author Member

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.

This comment has been minimized.

@jnewbery

jnewbery Jan 17, 2019

Member

I think either is fine.

This comment has been minimized.

@hebasto

hebasto Jan 18, 2019

Member

Tech language prefers a singular form; e.g., "window manager", "file manager" etc.

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Jan 16, 2019

@Sjors in #15150 b669ba6 that warning is removed.

Thanks for reviewing!

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Jan 17, 2019

Tested ACK 1316f5706891bb94428078e874e761931ff0e087

Only difference is rebasing on master and removing the unused WalletController::getWalletsAvailableToLoad()

EDIT: pasted incorrect commit id. Now fixed

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

@promag promag force-pushed the promag:2019-01-walletcontroller branch Jan 17, 2019

@promag promag force-pushed the promag:2019-01-walletcontroller branch to 0dd9bde Jan 18, 2019

@promag

This comment has been minimized.

Copy link
Member Author

promag commented Jan 18, 2019

Refactored a bit to simplify follow ups.

@Sjors

Sjors approved these changes Jan 18, 2019

Copy link
Member

Sjors left a comment

re-tACK 0dd9bde

@@ -376,7 +376,7 @@ bool WalletModel::changePassphrase(const SecureString &oldPass, const SecureStri
static void NotifyUnload(WalletModel* walletModel)
{
qDebug() << "NotifyUnload";
QMetaObject::invokeMethod(walletModel, "unload", Qt::QueuedConnection);
QMetaObject::invokeMethod(walletModel, "unload");

This comment has been minimized.

@Sjors

Sjors Jan 18, 2019

Member

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?

This comment has been minimized.

@promag

promag Jan 18, 2019

Author Member

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.

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Jan 18, 2019

utACK 0dd9bde

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jan 18, 2019

Tested ACK 0dd9bde

@jonasschnelli jonasschnelli merged commit 0dd9bde into bitcoin:master Jan 18, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Jan 18, 2019

Merge #15101: gui: Add WalletController
0dd9bde gui: Refactor to use WalletController (João Barbosa)
8fa271f gui: Add WalletController (João Barbosa)
cefb399 gui: Use AutoConnection for WalletModel::unload signal (João Barbosa)

Pull request description:

  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.

Tree-SHA512: 6a824054376730eb7d16c643dd2003f5f60778e8ad3af707b82bc12c48438db179ca4446316b28fb17b206f4b9aba8998419aab8c5dd1f7c32467015732b5094

@promag promag deleted the promag:2019-01-walletcontroller branch Jan 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment