gui: Add WalletController #15101

promag · 2019-01-04T12:25:30Z

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.

promag · 2019-01-04T21:23:36Z

Split in multiple commits for easier review.

DrahtBot · 2019-01-05T16:42:45Z

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

Conflicts

No conflicts as of last run.

Sjors

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?

src/qt/rpcconsole.cpp

src/qt/walletcontroller.h

Sjors · 2019-01-09T13:33:23Z

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

Looks good. A few comments and questions inline.

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

src/qt/bitcoingui.cpp

src/qt/walletcontroller.h

promag · 2019-01-12T11:02:42Z

Rebased with #15149 to avoid conflicts.

jnewbery

Lightly tested ACK 89fa546e8211aa47c96e20ba2a80507acd125539.

One question inline.

src/qt/walletcontroller.cpp

promag · 2019-01-14T22:27:56Z

@jnewbery done.

promag · 2019-01-16T00:58:42Z

Rebased.

Sjors

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};
                     ^

Sjors · 2019-01-16T15:55:48Z

src/qt/bitcoingui.cpp

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


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.

src/qt/walletcontroller.h

Sjors · 2019-01-16T16:01:26Z

src/qt/walletcontroller.h

+/**
+ * WalletController
+ */
+class WalletController : public QObject


Nit: WalletsController?

heh 😛avoid plural?

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

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.

I think either is fine.

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

promag · 2019-01-16T16:16:26Z

@Sjors in #15150 b669ba6 that warning is removed.

Thanks for reviewing!

jnewbery · 2019-01-17T16:18:48Z

Tested ACK 1316f5706891bb94428078e874e761931ff0e087

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

EDIT: pasted incorrect commit id. Now fixed

src/qt/walletcontroller.cpp

promag · 2019-01-18T00:51:18Z

Refactored a bit to simplify follow ups.

Sjors

re-tACK 0dd9bde

Sjors · 2019-01-18T10:14:18Z

src/qt/walletmodel.cpp

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


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?

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 · 2019-01-18T18:18:15Z

utACK 0dd9bde

jonasschnelli · 2019-01-18T20:20:45Z

Tested ACK 0dd9bde

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

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 bitcoin#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

Backport bitcoin#11625, bitcoin#15101

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 bitcoin#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

fanquake added the GUI label Jan 4, 2019

promag force-pushed the 2019-01-walletcontroller branch 3 times, most recently from d7d57d7 to f9b606e Compare January 4, 2019 21:21

promag force-pushed the 2019-01-walletcontroller branch 4 times, most recently from 4bb0233 to 23aae3d Compare January 5, 2019 15:30

Sjors approved these changes Jan 7, 2019

View reviewed changes

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

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

promag force-pushed the 2019-01-walletcontroller branch from 45ecc25 to 5e4cb35 Compare January 7, 2019 22:42

jnewbery reviewed Jan 9, 2019

View reviewed changes

DrahtBot added the Needs rebase label Jan 9, 2019

promag force-pushed the 2019-01-walletcontroller branch from 5e4cb35 to 744f0e3 Compare January 12, 2019 02:49

DrahtBot removed the Needs rebase label Jan 12, 2019

promag force-pushed the 2019-01-walletcontroller branch from 744f0e3 to 20f11da Compare January 12, 2019 11:01

promag force-pushed the 2019-01-walletcontroller branch from 20f11da to 3c50d90 Compare January 12, 2019 11:12

promag mentioned this pull request Jan 12, 2019

gui: Add Open Wallet menu #15153

Merged

promag force-pushed the 2019-01-walletcontroller branch 4 times, most recently from e79abc7 to 89fa546 Compare January 14, 2019 14:11

DrahtBot added the Needs rebase label Jan 14, 2019

jnewbery reviewed Jan 14, 2019

View reviewed changes

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

promag force-pushed the 2019-01-walletcontroller branch from 89fa546 to a9a3069 Compare January 14, 2019 22:27

DrahtBot removed the Needs rebase label Jan 14, 2019

Sjors approved these changes Jan 16, 2019

View reviewed changes

promag mentioned this pull request Jan 17, 2019

gui: Add dynamic wallets support #13100

Closed

jonasschnelli reviewed Jan 17, 2019

View reviewed changes

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

promag force-pushed the 2019-01-walletcontroller branch from 1316f57 to 1b82938 Compare January 17, 2019 23:31

promag added 3 commits January 18, 2019 00:27

gui: Use AutoConnection for WalletModel::unload signal

cefb399

gui: Add WalletController

8fa271f

gui: Refactor to use WalletController

0dd9bde

promag force-pushed the 2019-01-walletcontroller branch from 1b82938 to 0dd9bde Compare January 18, 2019 00:40

Sjors approved these changes Jan 18, 2019

View reviewed changes

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

promag deleted the 2019-01-walletcontroller branch January 18, 2019 20:38

ryanofsky mentioned this pull request Jan 23, 2019

Multiprocess bitcoin #10102

Draft

hebasto mentioned this pull request Jan 25, 2019

qt: Add warning messages to the debug window #14879

Merged

ryanofsky mentioned this pull request Mar 17, 2020

gui: segfault unloading and immediately reloading wallet with gui #18362

Closed

UdjinM6 added a commit to dashpay/dash that referenced this pull request Sep 18, 2021

Merge pull request #4410 from PastaPastaPasta/backport-new-wallet-pr2

6751412

Backport bitcoin#11625, bitcoin#15101

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

gui: Add WalletController #15101

gui: Add WalletController #15101

Uh oh!

Conversation

promag commented Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jan 4, 2019

Uh oh!

DrahtBot commented Jan 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sjors commented Jan 9, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

promag commented Jan 12, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag commented Jan 14, 2019

Uh oh!

promag commented Jan 16, 2019

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

promag Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

Sjors Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

promag Jan 16, 2019

Choose a reason for hiding this comment

Uh oh!

jnewbery Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 18, 2019

Choose a reason for hiding this comment

Uh oh!

promag commented Jan 16, 2019

Uh oh!

jnewbery commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

promag commented Jan 18, 2019

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

promag commented Jan 4, 2019 •

edited

Loading

DrahtBot commented Jan 5, 2019 •

edited

Loading

jnewbery commented Jan 17, 2019 •

edited

Loading

Sjors Jan 18, 2019 •

edited

Loading

promag Jan 18, 2019 •

edited

Loading