gui: Create wallet menu option

achow101 commented at 5:45 pm on February 20, 2019: member

This PR adds a menu option to create a new wallet. When clicked, a CreateWalletDialog will be created and prompt the user to name the wallet and choose whether to disable private keys, make a blank wallet, and encrypt the wallet. If the wallet is encrypted, the wallet will be born encrypted with the wallet first created blank, then encrypted, and then a new HD seed generated and set.

To allow the newly created wallets to be encrypted, some changes to how encrypting a wallet works. Instead of encrypting and locking the wallet, the wallet will be encrypted and then unlocked. This is also an extra belt-and-suspenders check to make sure that encryption worked.

achow101 force-pushed on Feb 20, 2019

in src/qt/createwalletdialog.cpp:63 in 63f1309fe6 outdated

65+    dialog->setCancelButton(nullptr);
66+    dialog->setWindowModality(Qt::ApplicationModal);
67+    dialog->show();
68+
69+    // Create the wallet
70+    std::unique_ptr<interfaces::Wallet> wallet = m_wallet_controller->createWallet(wallet_name, flags);

ryanofsky commented at 6:39 pm on February 20, 2019:

Maybe it is too difficult to do in this PR, but if it’s possible to avoid creating the wallet in the GUI event loop thread and calling createWallet, setNewHdSeed, topUpKeyPool synchronously that would be a nice of allowing the GUI to avoid hangs and be more responsive. I believe @promag has some changes which use worker threads, so he might be able to give specific suggestions.

There may also be reason to avoid the new dlg.exec() call as in #15313.

achow101 commented at 7:46 pm on February 20, 2019:

I’m not very familiar with the GUI code so I think it would best to let someone else do that.

promag commented at 10:14 pm on May 30, 2019:

The suggestion I have is to do similar to #15478. I think it’s fine as a follow up.

instagibbs commented at 7:36 pm on February 20, 2019: member

Maybe:

Move Encrypt wallet to the top since it’s likely the most common thing people would do
The menu as-is lacks any explanation, which makes it hard to evaluate unless you’re an expert and know all the internals. Explain what they do/interactions between different modes

achow101 force-pushed on Feb 20, 2019

achow101 commented at 7:46 pm on February 20, 2019: member

Move Encrypt wallet to the top since it’s likely the most common thing people would do

Done

The menu as-is lacks any explanation, which makes it hard to evaluate unless you’re an expert and know all the internals. Explain what they do/interactions between different modes

There are tooltips that explain this. Hover over the selections.

DrahtBot commented at 8:02 pm on February 20, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

Sjors commented at 9:11 pm on February 20, 2019: member

Concept ACK 🎉

promag commented at 9:53 pm on February 20, 2019: member

Concept ACK, will pick this shortly and push a branch with some changes for your consideration.

fanquake added the label GUI on Feb 20, 2019

in src/qt/bitcoingui.cpp:437 in 668aa51d2b outdated

433@@ -425,6 +434,7 @@ void BitcoinGUI::createMenuBar()
434     {
435         file->addAction(m_open_wallet_action);
436         file->addAction(m_close_wallet_action);
437+        file->addAction(m_create_wallet_action);

promag commented at 0:12 am on February 21, 2019:

Usually this comes first.

achow101 commented at 6:36 pm on May 23, 2019:

Done

fanquake commented at 4:51 am on February 21, 2019: member

Concept ACK.

Screen:

Travis warning: A new circular dependency in the form of "qt/bitcoingui -> qt/createwalletdialog -> qt/bitcoingui" appears to have been introduced.

jonasschnelli commented at 5:45 am on February 21, 2019: contributor

General Concept ACK I’m not sure if we – yet - should allow creating a blank wallet or a wallet with disabled private keys since it can’t be used/extended with the GUI (outside of the console). It would make very much sense if there would be a way to import descriptors/addresses and or set/change a hd seed via the GUI.

Sjors commented at 8:50 am on February 21, 2019: member

Works!

Couple of things, nothing that can’t wait for a followup, but since this is still very early in the 0.19 cycle might as well get it right:

I suspect adding a grey border, like in the send / receive screen, is more visually pleasing. It also allows separating the screen into sections.
Let’s add a section Advanced, where the Disable Private Keys and Make Blank Wallet goes.
Left side of check boxes should be aligned with the “Wallet” label.
“Create Wallet” menu items needs “…” (to indicate there’s still some form of confirmation)
Rename “OK” to “Create”
Disable OK and Cancel buttons as soon as the users clicks OK (might tie into @ryanofsky’s point about event loops, because double clicking doesn’t cause a duplicate wallet afaik)
When you click Encrypt Wallet, unhide an “Encryption” section with a Password & Confirmation field (just like we unhide coin selection and fees section)
Disable the OK (Create) button until form validation passes (in this case: name must be set)
Set initial focus on name field, and pressing tab should jump to the check boxes, the text next to them (this makes the tooltip accessible), and the OK / Cancel field
Canceling the password dialog should result in canceling wallet creation (easy to prevent with the previous suggestion, because the form would simply be invalid if “Encrypt” is ticked and the password is empty or not matching)

I agree with @jonasschnelli that being able to enter descriptors here would be useful, but I suggest holding off on that until we have a descriptor based wallet. Otherwise we have to explain that once the keypool runs out they somehow need to top it up. For the most part I expect descriptors would be part of a recovery workflow, which could be another menu item like Recover wallet. But they could also be an advanced setting during wallet creation, if someone wants to override our default derivation paths.

fanquake added the label Wallet on Feb 23, 2019

fanquake added this to the milestone 0.19.0 on Feb 23, 2019

fanquake added this to the "In progress" column in a project

hebasto commented at 8:54 pm on February 24, 2019: member

Concept ACK.

in src/qt/walletcontroller.cpp:61 in 668aa51d2b outdated

55@@ -55,6 +56,21 @@ std::vector<std::string> WalletController::getWalletsAvailableToOpen() const
56     return wallets;
57 }
58 
59+bool WalletController::checkWalletExists(std::string name) const

practicalswift commented at 11:29 pm on February 25, 2019:

name could be const ref?

achow101 commented at 3:07 pm on May 28, 2019:

Done

jnewbery commented at 5:52 pm on May 22, 2019: member

This fails a linter on Travis (“A new circular dependency in the form of “qt/bitcoingui -> qt/createwalletdialog -> qt/bitcoingui” appears to have been introduced.”), and also fails to build when rebased on master, since WalletImpl.m_wallet is now a shared pointer instead of a CWallet object. @achow101 - do you intend to keep working on this?

achow101 commented at 6:13 pm on May 22, 2019: member

Yes, I’ll rebase and have a look at it again soon.

achow101 force-pushed on May 23, 2019

achow101 commented at 6:36 pm on May 23, 2019: member

Rebased.

I’m not entirely sure how to get rid of the circular dependency.

in src/wallet/wallet.cpp:160 in 767a5f85d1 outdated

156@@ -157,6 +157,22 @@ std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const std::string&
157     return LoadWallet(chain, WalletLocation(name), error, warning);
158 }
159 
160+std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, const WalletLocation& location, uint64_t wallet_creation_flags)

jnewbery commented at 9:19 pm on May 23, 2019:

I don’t see any reason to split this out into two functions. This CreateWallet() function is only called from the CreateWallet() function below. Why not just place the logic in that function?

achow101 commented at 4:45 pm on May 24, 2019:

Done

in src/wallet/wallet.h:51 in 767a5f85d1 outdated

47@@ -48,6 +48,7 @@ bool HasWallets();
48 std::vector<std::shared_ptr<CWallet>> GetWallets();
49 std::shared_ptr<CWallet> GetWallet(const std::string& name);
50 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::string& warning);
51+std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, const WalletLocation& location, uint64_t wallet_creation_flags);

jnewbery commented at 9:20 pm on May 23, 2019:

CreateWallet(interfaces::Chain& chain, const std::string& name, uint64_t wallet_creation_flags) should be declared here (instead of the (interfaces::Chain& chain, const WalletLocation& location, uint64_t wallet_creation_flags) version, which can be removed)

achow101 commented at 4:45 pm on May 24, 2019:

Done

in src/wallet/rpcwallet.cpp:2144 in 767a5f85d1 outdated

2140@@ -2141,6 +2141,9 @@ static UniValue encryptwallet(const JSONRPCRequest& request)
2141     if (!pwallet->EncryptWallet(strWalletPass)) {
2142         throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: Failed to encrypt the wallet.");
2143     }
2144+    if (!pwallet->Unlock(strWalletPass)) {

jnewbery commented at 10:00 pm on May 23, 2019:

I don’t understand why this change (unlocking the wallet after it’s encrypted in RPC) is necessary or desirable. If you want to keep it, can you add some justification (perhaps to the commit log)?

achow101 commented at 4:45 pm on May 24, 2019:

Dropped that commit.

in src/qt/createwalletdialog.cpp:65 in 767a5f85d1 outdated

67+    dialog->show();
68+
69+    // Create the wallet
70+    std::unique_ptr<interfaces::Wallet> wallet = m_wallet_controller->createWallet(wallet_name, flags);
71+
72+    if (wallet) {

jnewbery commented at 10:08 pm on May 23, 2019:

This isn’t great. If I create a wallet with ‘Encrypt Wallet’ set, but then ‘Cancel’ from the ‘Encrypt Wallet’ dialog, the wallet will be created but not encrypted. That shouldn’t happen.

achow101 commented at 4:42 pm on May 24, 2019:

Hmm. I think fixing that requires a significant change to how AskPassphraseDialog works due to that dialog being the one that also encrypts.

jnewbery commented at 5:14 pm on May 24, 2019:

I think it needs to be done. Creating an unencrypted wallet by accident when the user backs out of the change password dialog is bad user experience.

achow101 commented at 6:03 pm on May 24, 2019:

I’ll look into it

achow101 commented at 10:18 pm on May 24, 2019:

Done

in src/qt/createwalletdialog.cpp:102 in 767a5f85d1 outdated

84+        }
85+        m_parent->setCurrentWallet(model);
86+    } else {
87+        QMessageBox::critical(this, tr("Wallet creation failed"), tr("Wallet creation failed due to an internal error. The wallet was not created."));
88+    }
89+    dialog->hide();

jnewbery commented at 10:10 pm on May 23, 2019:

I don’t think the dialog should hide if wallet creation fails. For example, if I try to create a wallet with the name of an existing wallet and get the ‘A wallet with the name already exists’ dialog, I shouldn’t be dumped back into the main GUI when I click ‘OK’. I should have the chance to try a different name.

achow101 commented at 4:45 pm on May 24, 2019:

Done

in src/qt/createwalletdialog.cpp:36 in 767a5f85d1 outdated

20+    QDialog(parent),
21+    ui(new Ui::CreateWalletDialog),
22+    m_parent(parent)
23+{
24+    ui->setupUi(this);
25+}

jnewbery commented at 10:12 pm on May 23, 2019:

You should use setWindowTitle() here so the dialog isn’t just titled ‘Dialog’.

achow101 commented at 4:45 pm on May 24, 2019:

Done

in src/qt/forms/createwalletdialog.ui:87 in 767a5f85d1 outdated

82+   </property>
83+   <property name="text">
84+    <string>Make Blank Wallet</string>
85+   </property>
86+  </widget>
87+  <widget class="QCheckBox" name="encrypt_wallet_checkbox">

jnewbery commented at 10:13 pm on May 23, 2019:

Should this default to checked?

achow101 commented at 6:12 pm on May 24, 2019:

Done

in src/qt/forms/createwalletdialog.ui:55 in 767a5f85d1 outdated

50+   </property>
51+   <property name="text">
52+    <string>Wallet Name</string>
53+   </property>
54+  </widget>
55+  <widget class="QCheckBox" name="disable_privkeys_checkbox">

jnewbery commented at 10:13 pm on May 23, 2019:

I agree with @Sjors that these two boxes should be hidden in an ‘Advanced options’ section.

achow101 commented at 6:13 pm on May 24, 2019:

I couldn’t figure out how to do this.

in src/qt/createwalletdialog.cpp:68 in 767a5f85d1 outdated

70+    std::unique_ptr<interfaces::Wallet> wallet = m_wallet_controller->createWallet(wallet_name, flags);
71+
72+    if (wallet) {
73+        WalletModel* model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
74+        // Encrypt the wallet
75+        if (encrypt) {

jnewbery commented at 10:15 pm on May 23, 2019:

There seems to be duplicated logic between here and the createwallet RPC. Can some of that be moved into CreateWallet() in wallet.cpp?

achow101 commented at 4:44 pm on May 24, 2019:

I don’t think so.

jnewbery commented at 5:18 pm on May 24, 2019:

I don’t understand why not. Both the RPC and QT are calling a createwallet function, then having control returned to them, and then they encrypt the wallet, set the HD seed, and top up the keypool. It seems better to me that the CreateWallet() function take care of this and not expose the low-level setNewHDSeed and topUpKeyPool methods on the wallet interface.

achow101 commented at 6:13 pm on May 24, 2019:

I see… I think this would require the AskPassphraseDialog change as well in order to do this.

achow101 commented at 10:18 pm on May 24, 2019:

Done

jnewbery commented at 10:16 pm on May 23, 2019: member

Thanks for rebasing @achow101!

I’m not entirely sure how to get rid of the circular dependency.

I think for now it’s ok to remove the call to m_parent->setCurrentWallet(model); in CreateWalletDialog::accept(). That would allow you to not have an m_wallet member variable, and change the CreateWalletDialog::CreateWalletDialog() signature to take a QWidget* instead of a BitcoinGUI*.

This matches the existing ‘Open Wallet’ behaviour, which does not select the new wallet when it’s opened.

achow101 force-pushed on May 24, 2019

in src/qt/createwalletdialog.h:31 in b2d0546a02 outdated

26+    explicit CreateWalletDialog(QWidget *parent);
27+    ~CreateWalletDialog();
28+
29+    void accept();
30+
31+    void SetWalletController(WalletController *wallet_controller);

jnewbery commented at 5:12 pm on May 24, 2019:

Why is this needed as a separate function? Can the WalletController just be passed as an argument to the constructor?

achow101 commented at 6:13 pm on May 24, 2019:

Done

in src/qt/createwalletdialog.cpp:10 in b2d0546a02 outdated

 5+#if defined(HAVE_CONFIG_H)
 6+#include <config/bitcoin-config.h>
 7+#endif
 8+
 9+#include <qt/askpassphrasedialog.h>
10+#include <qt/bitcoingui.h>

jnewbery commented at 5:41 pm on May 24, 2019:

This include needs to be removed

achow101 commented at 6:13 pm on May 24, 2019:

Done

jnewbery commented at 5:42 pm on May 24, 2019: member

This now segfaults for me when I try to create an encryted wallet.

I think that https://github.com/bitcoin/bitcoin/pull/15450/files#r287152838 needs to be addressed. If not, perhaps this PR could drop support for creating encrypted wallets to begin with, and we add that later.

achow101 force-pushed on May 24, 2019

achow101 commented at 10:18 pm on May 24, 2019: member

I’ve made two major changes to this: AskPassphraseDialog now has an output parameter for it to return the passphrase for encryption mode and I refactored the createwallet RPC so that all of the other wallet creation stuff it does (checking flags, encryption, etc.) is in its own function that this GUI option can call too. These two changes fix the issues that @jnewbery mentioned earlier. And it no longer segfaults.

achow101 force-pushed on May 24, 2019

achow101 force-pushed on May 28, 2019

instagibbs commented at 3:23 pm on May 28, 2019: member

non-blocking suggestion: I’d suggest having the password confirmation itself on an entirely different window. You really want to have the user go through the paces of unlocking it without being able to stare at the “answer” they just put in.

in src/qt/askpassphrasedialog.cpp:126 in 759c837f93 outdated

119@@ -119,7 +120,12 @@ void AskPassphraseDialog::accept()
120         {
121             if(newpass1 == newpass2)
122             {
123-                if(model->setWalletEncrypted(true, newpass1))
124+                bool success = (model && model->setWalletEncrypted(true, newpass1)) || m_passphrase_out;
125+                if (m_passphrase_out)
126+                {
127+                    m_passphrase_out->assign(newpass1);

jnewbery commented at 3:37 pm on May 28, 2019:

I suggest you add a QMessageBox::warning here (the message in the existing success warning about “Any previous backups” is not relevant for new wallets) and then return.

achow101 commented at 5:13 pm on May 28, 2019:

Done

jnewbery commented at 3:37 pm on May 28, 2019: member

This segfaults for me on startup:

0→ ./src/qt/bitcoin-qt 
1Segmentation fault (core dumped)

One other comment inline

achow101 commented at 5:02 pm on May 28, 2019: member

This segfaults for me on startup:

Can’t replicate this. Can you provide a backtrace?

achow101 force-pushed on May 28, 2019

jonatack commented at 4:02 pm on May 29, 2019: member

This segfaults for me on startup:

Can’t replicate this. Can you provide a backtrace?

FWIW baf58866f7fad57e2c4caf6ca198059cb333247a is running for me on Debian Linux. Will review.

jnewbery commented at 4:12 pm on May 29, 2019: member

Sorry, will retest today. It passes travis so it’s probably me doing something stupid.

fanquake added this to the "Blockers" column in a project

in src/qt/bitcoingui.h:152 in baf58866f7 outdated

148@@ -149,6 +149,7 @@ class BitcoinGUI : public QMainWindow
149     QAction* showHelpMessageAction = nullptr;
150     QAction* m_open_wallet_action{nullptr};
151     QAction* m_close_wallet_action{nullptr};
152+    QAction* m_create_wallet_action{nullptr};

promag commented at 10:15 pm on May 30, 2019:

nitnit, move up.

achow101 commented at 1:42 pm on June 13, 2019:

Done

in src/qt/createwalletdialog.h:26 in baf58866f7 outdated

21+{
22+    Q_OBJECT
23+
24+public:
25+    explicit CreateWalletDialog(QWidget *parent, WalletController *wallet_controller);
26+    ~CreateWalletDialog();

promag commented at 10:16 pm on May 30, 2019:

virtual

achow101 commented at 1:42 pm on June 13, 2019:

Done

in src/qt/walletcontroller.cpp:61 in baf58866f7 outdated

57@@ -57,13 +58,33 @@ std::vector<std::string> WalletController::getWalletsAvailableToOpen() const
58     return wallets;
59 }
60 
61+bool WalletController::checkWalletExists(const std::string name) const

promag commented at 10:17 pm on May 30, 2019:

string&

promag commented at 10:19 pm on May 30, 2019:

Not sure if this check (as it is pays) off. It doesn’t check if name as a path exists - since name can be a path. Maybe add m_node.canCreateWallet(name)?

jonatack commented at 7:43 pm on June 7, 2019:

checkWalletExists does not detect dupes if the user inadvertently enters leading or trailing spaces in the wallet name field. This is seems more problematic for trailing spaces since any such spaces are currently invisible in the “Open Wallet” list so the wallet names appear identical. Perhaps strip (leading and) trailing spaces before the Create Wallet validation stage?

achow101 commented at 1:42 pm on June 13, 2019:

I’ve removed the check entirely since the same thing is done in the CreateWallet() function already.

achow101 commented at 2:16 pm on June 13, 2019:

This function has been removed. The check is handled by WalletLocation now.

promag commented at 10:22 pm on May 30, 2019: member

Some comments, will review thoroughly tomorrow.

in src/qt/askpassphrasedialog.cpp:124 in baf58866f7 outdated

119@@ -119,24 +120,36 @@ void AskPassphraseDialog::accept()
120         {
121             if(newpass1 == newpass2)
122             {
123-                if(model->setWalletEncrypted(true, newpass1))
124+                if (m_passphrase_out)
125                 {

PastaPastaPasta commented at 2:41 am on June 6, 2019:

fix formatting, brackets should be on same line

achow101 commented at 1:43 pm on June 13, 2019:

Done

in src/qt/askpassphrasedialog.cpp:94 in baf58866f7 outdated

90@@ -90,7 +91,7 @@ void AskPassphraseDialog::setModel(WalletModel *_model)
91 void AskPassphraseDialog::accept()
92 {
93     SecureString oldpass, newpass1, newpass2;
94-    if(!model)
95+    if(!model && mode != Encrypt)

PastaPastaPasta commented at 2:42 am on June 6, 2019:

0    if (!model && mode != Encrypt)

also, move the body (return;) onto the same line or enclose in brackets

achow101 commented at 1:43 pm on June 13, 2019:

Changed the spacing, leaving return as is because it was not touched.

in src/qt/askpassphrasedialog.cpp:134 in baf58866f7 outdated

143-                    QMessageBox::critical(this, tr("Wallet encryption failed"),
144-                                         tr("Wallet encryption failed due to an internal error. Your wallet was not encrypted."));
145+                } else {
146+                    assert(model != nullptr);
147+                    if(model->setWalletEncrypted(true, newpass1))
148+                    {

PastaPastaPasta commented at 2:42 am on June 6, 2019:

brackets should be on same line

achow101 commented at 1:40 pm on June 13, 2019:

This is moved code, not changing formatting

in src/qt/askpassphrasedialog.cpp:146 in baf58866f7 outdated

157+                                             "For security reasons, previous backups of the unencrypted wallet file "
158+                                             "will become useless as soon as you start using the new, encrypted wallet.") +
159+                                             "</b></qt>");
160+                    }
161+                    else
162+                    {

PastaPastaPasta commented at 2:43 am on June 6, 2019:

147 - 149 should be on the same line

achow101 commented at 1:40 pm on June 13, 2019:

This is moved code, not changing formatting

in src/qt/bitcoingui.cpp:427 in baf58866f7 outdated

445@@ -438,6 +446,7 @@ void BitcoinGUI::createMenuBar()
446     QMenu *file = appMenuBar->addMenu(tr("&File"));
447     if(walletFrame)
448     {

PastaPastaPasta commented at 2:43 am on June 6, 2019:

fix? add space and move {

achow101 commented at 1:41 pm on June 13, 2019:

No, unrelated.

PastaPastaPasta changes_requested

PastaPastaPasta commented at 2:44 am on June 6, 2019: contributor

couple of formatting nits, concept ACK

in src/qt/forms/createwalletdialog.ui:65 in baf58866f7 outdated

60+     <width>171</width>
61+     <height>22</height>
62+    </rect>
63+   </property>
64+   <property name="toolTip">
65+    <string>Disable private keys for this wallet. Wallets with private keys disabled will not have any private keys and cannot have a HD seed set or private keys imported. These wallets are ideal for watch only wallets.</string>

jonatack commented at 7:23 pm on June 7, 2019:

s/a HD/an HD/ (“h” is pronounced as a vowel here similar to how one writes “an hour” and not “a hour”).

“cannot have a HD seed set or private keys imported” is unclear because “set” can be either a verb or a noun, which changes the possible meaning substantially.

Suggestion, depending on the intended meaning:

Wallets with private keys disabled will have no private keys and cannot import them or have an HD seed. This is ideal for watch-only wallets.

Wallets with private keys disabled will have no private keys and cannot have an HD seed or imported private keys. This is ideal for watch-only wallets.

achow101 commented at 2:20 pm on June 13, 2019:

Changed to the second suggestion.

in src/qt/forms/createwalletdialog.ui:81 in baf58866f7 outdated

76+     <width>171</width>
77+     <height>22</height>
78+    </rect>
79+   </property>
80+   <property name="toolTip">
81+    <string>Make a blank wallet. Blank wallets do not initially have private keys or scripts. Private keys and addresses can be imported, or a HD seed set later.</string>

jonatack commented at 7:32 pm on June 7, 2019:

Suggestion to avoid confusion between “set” as a noun or as a verb:

Make a blank wallet. Blank wallets do not initially have private keys or scripts. Private keys and addresses can be imported, or an HD seed can be set, at a later time.

s/a HD/an HD/ as above.

achow101 commented at 2:20 pm on June 13, 2019:

Done

in src/qt/forms/createwalletdialog.ui:102 in baf58866f7 outdated

91+     <y>60</y>
92+     <width>171</width>
93+     <height>22</height>
94+    </rect>
95+   </property>
96+   <property name="toolTip">

jonatack commented at 7:38 pm on June 7, 2019:

Tab focus hits the “Encrypt Wallet” checkbox after the first two checkboxes instead of in its new position first. Re-order needed.

achow101 commented at 2:20 pm on June 13, 2019:

Fixed

jonatack commented at 8:13 pm on June 7, 2019: member

Concept ACK.

In this review of baf58866f7fad57e2c4caf6ca198059cb333247a I focused on the user experience.

Main issues seen and suggestions are in my code comments below.

Thoughts relative to Sjors’ review above:

* "Create Wallet" menu items needs "..."

Agree.

* Rename "OK" to "Create"

Agree.

* Disable the OK (Create) button until form validation passes (in this case: name must be set)

Agree.

* Set initial focus on name field, and pressing tab should jump to the check boxes, the text next to them (this makes the tooltip accessible), and the OK / Cancel field

Agree.

* Canceling the password dialog should result in canceling wallet creation (easy to prevent with the previous suggestion, because the form would simply be invalid if "Encrypt" is ticked and the password is empty or not matching)

Agree!

* Left side of check boxes should be aligned with the "Wallet" label.

The current alignment looks fine to me (on Linux). Shifting the checkboxes left under the wallet label would move them quite close to the left border.

Finally, it would be nice for the tool tips to last longer before disappearing, if that is settable. The messages are quite long to take in.

flack commented at 10:01 pm on June 7, 2019: contributor

If the screenshot above is still current, the window title should probably be change from “Dialog” to “Create Wallet”

jonatack commented at 10:32 pm on June 7, 2019: member

If the screenshot above is still current, the window title should probably be change from “Dialog” to “Create Wallet” @flack that was fixed. See #15450 (review).

achow101 force-pushed on Jun 13, 2019

achow101 commented at 2:20 pm on June 13, 2019: member

* I suspect adding a grey border, like in the send / receive screen, is more visually pleasing. It also allows separating the screen into sections.

Done

* Let's add a section Advanced, where the Disable Private Keys and Make Blank Wallet goes.

Can’t figure out how to do that.

* Left side of check boxes should be aligned with the "Wallet" label.

Aligned the new group box they’re in

* "Create Wallet" menu items needs "..." (to indicate there's still some form of confirmation)

Done

* Rename "OK" to "Create"

Done

* Disable OK and Cancel buttons as soon as the users clicks OK (might tie into [@ryanofsky](/bitcoin-bitcoin/contributor/ryanofsky/)'s point about event loops, because double clicking doesn't cause a duplicate wallet afaik)

Done

* When you click Encrypt Wallet, unhide an "Encryption" section with a Password & Confirmation field (just like we unhide coin selection and fees section)

I prefer to reuse the existing AskPassphraseDialog

* Disable the OK (Create) button until form validation passes (in this case: name must be set)

Done

* Set initial focus on name field, and pressing tab should jump to the check boxes, the text next to them (this makes the tooltip accessible), and the OK / Cancel field

Done

* Canceling the password dialog should result in canceling wallet creation (easy to prevent with the previous suggestion, because the form would simply be invalid if "Encrypt" is ticked and the password is empty or not matching)

Done

achow101 force-pushed on Jun 13, 2019

jonatack commented at 0:08 am on June 17, 2019: member

Thanks @achow101 for sticking with this PR.

Review of 2ae68a3a33606cc5391c86ed6a43c798e3dd3eea

The “Encrypt Wallet” option is now 2/3 outside of the new grey border for me on Linux Debian/Gnome.
The duplicate wallet name checking now appears to work (nice), a dupes popup appears, but as soon as I click on “OK” in that popup, a “Creating Wallet xxx…” popup happens, appears to attempt to create the wallet and hangs indefinitely in that state.
Now that we asked for the OK button to be labelled “Create”, it’s somewhat surprising that it jumps to the passphrase form popup rather than actually creating a wallet. Perhaps label it as “Next” at that point? * ducks *
The OK button is still labeled “OK” in the passphrase step; perhaps here is the place where it should be labeled “Create”…
Once inside the password form, the “OK” button turns green when the 2 passphrase fields are not empty, even if they do not match. The wallet does not perform the verification until after “OK” turns to “Yes”, the warning about losing all your Bitcoins popup appears, and “Yes” is clicked. The supplied passphrases do not match popup then finally appears. If possible with qt – agreed, it’s not a web app – passphrase verification would preferably be done while the user enters the passphrases, and the OK button not become active until they match.

jonatack commented at 0:12 am on June 17, 2019: member

Further to point 5 above, if an interactive passphrase matching check isn’t feasible, then perhaps perform it as soon as the user clicks OK, before the confirmation warning about losing bitcoins, if possible.

achow101 commented at 10:09 pm on June 17, 2019: member

The “Encrypt Wallet” option is now 2/3 outside of the new grey border for me on Linux Debian/Gnome.

Screenshot please? I don’t see this.

The duplicate wallet name checking now appears to work (nice), a dupes popup appears, but as soon as I click on “OK” in that popup, a “Creating Wallet xxx…” popup happens, appears to attempt to create the wallet and hangs indefinitely in that state.

Fixed

Now that we asked for the OK button to be labelled “Create”, it’s somewhat surprising that it jumps to the passphrase form popup rather than actually creating a wallet. Perhaps label it as “Next” at that point? * ducks *

The OK button is still labeled “OK” in the passphrase step; perhaps here is the place where it should be labeled “Create”…

I’m gonna leave it as is because it’s kind of annoying to make it change like that especially since we use the AskPassphraseDialog that is used elsewhere.

Once inside the password form, the “OK” button turns green when the 2 passphrase fields are not empty, even if they do not match. The wallet does not perform the verification until after “OK” turns to “Yes”, the warning about losing all your Bitcoins popup appears, and “Yes” is clicked. The supplied passphrases do not match popup then finally appears. If possible with qt – agreed, it’s not a web app – passphrase verification would preferably be done while the user enters the passphrases, and the OK button not become active until they match.

That’s an issue with AskPassphraseDialog and I think changing that would be out of scope for this PR.

achow101 force-pushed on Jun 17, 2019

fanquake commented at 9:24 am on June 18, 2019: member

re-Concept ACK. Reviewing this at the moment.

DrahtBot added the label Needs rebase on Jun 18, 2019

ryanofsky commented at 9:09 pm on June 19, 2019: member

Just a suggestion, but I think it’d be nice to move commit “Move wallet creation out of the createwallet rpc into its own function” into its own separate PR, because it’s a desirable change by itself, and also because it would separate the gui and non-gui changes here, and make this PR smaller and less prone to conflicts.

It would also be nice to pull in promag’s changes from #16215 (and add him as co-author) so reviewers just need to review the final asynchronous code, instead of reviewing a temporary synchronous version and then another PR moving everything around and making it asynchronous.

jonatack commented at 9:18 pm on June 19, 2019: member

The “Encrypt Wallet” option is now 2/3 outside of the new grey border for me on Linux Debian/Gnome.

Screenshot please? I don’t see this. @achow101 here is one from today. Edit: I thought it was fine in baf58866f7fad57e2c4caf6ca198059cb333247a without the group box.

Screenshot from 2019-06-19 12-23-35

achow101 force-pushed on Jun 19, 2019

achow101 commented at 10:00 pm on June 19, 2019: member

Just a suggestion, but I think it’d be nice to move commit “Move wallet creation out of the createwallet rpc into its own function” into its own separate PR, because it’s a desirable change by itself, and also because it would separate the gui and non-gui changes here, and make this PR smaller and less prone to conflicts.

Done. See #16244

It would also be nice to pull in promag’s changes from #16215 (and add him as co-author) so reviewers just need to review the final asynchronous code, instead of reviewing a temporary synchronous version and then another PR moving everything around and making it asynchronous.

I’ll look into it.

DrahtBot removed the label Needs rebase on Jun 19, 2019

achow101 force-pushed on Jun 20, 2019

achow101 commented at 3:48 pm on June 20, 2019: member

@jonatack I’ve made a couple adjustments to the groupbox there. Does it change anything for you?

jonatack commented at 7:44 pm on June 20, 2019: member

@achow101 fe0ea08584eec1cbe44b59810ed6fe56f6e4250e:

Screenshot from 2019-06-20 15-24-35

and baf58866f7fad57e2c4caf6ca198059cb333247a:

Screenshot from 2019-06-20 15-40-45

achow101 force-pushed on Jun 21, 2019

achow101 commented at 6:18 pm on June 21, 2019: member

I’ve removed the group box as it seems to be causing some issues.

achow101 force-pushed on Jun 21, 2019

achow101 commented at 7:14 pm on June 21, 2019: member

I’ve rebased this onto #16261 and squashed in the create wallet changes from #16215

fanquake commented at 12:55 pm on June 22, 2019: member

I know it’s bit late to be thinking about this at a concept level, but I’ve got a few thoughts (they could all be addressed in a followup anyways):

When people are reviewing this PR: What end-user to they have in mind? Somewhere on the spectrum of new non-technical user, just downloaded Bitcoin Core & one of the engineers that helps build Bitcoin Core? I’d like to think we are trying to optimize for the former, with advanced options (hidden), bitcoin-cli, RPCs etc available for the later.
With that in mind, in the ideal world, the create wallet screen just has the Wallet Name field, and the three check-boxes are hidden in an Advanced Options drop down.
We’d have Encrypt Wallet checked by default, with password setting part of the create wallet process.
In it’s current form I’d guess that Make Blank Wallet is going to get ticked quite a bit, even when users don’t mean it, because the logical non-technical thought will be, why would I want anything other than a blank wallet..?
There also wouldn’t be a way that you could tick any combination of check boxes and end up in a state you don’t expect. i.e Check Encrypt Wallet and Disable Private Keys and even after multiple dialog boxes telling you Warning!!!!! Your wallet is about to be encrypted!!!!!, your wallet isn’t actually encrypted. The non-technical user will be non the wiser.
There are some other things that would be nice to have, like properly auto-focussing on the Encrypt wallet dialog when it appears. When you hit cancel, not having to restart the flow from scratch etc.

I bought the same thoughts up with @meshcollider on IRC here (line 204).

flack commented at 11:09 am on June 23, 2019: contributor

regarding 5: Couldn’t you simply add some logic that disables the “encrypt wallet” checkbox when “disable private keys” is checked (and vice versa)?

achow101 commented at 2:43 pm on June 25, 2019: member

@fanquake

IMO the end user in mind is the completely non-technical user who just downloaded Bitcoin Core. The advanced options are for the more technical users.
I tried having the advanced options hidden with a drop down, but I don’t know enough about Qt to make that happen.
Already got that :+1:
I could see how that can happen. I think changing the name blank would fix that. Otherwise we have to hope that users read the tooltip which explains what blank means.
Right now I don’t think any of the options really conflict with each other. Sure Encrypt Wallet and Disable Private Keys doesn’t make sense, but it still does actually encrypt the private keys in the wallet, and any private keys that are added would be encrypted. Of course the wallet has private keys disabled so nothing is actually encrypted, but it didn’t do nothing, and the wallet is still technically encrypted (under our definition of encrypted wallet).
It’d be great if someone who knows more Qt magic could do that.

promag commented at 2:59 pm on June 25, 2019: member

Dialog improvements can come next IMO. Another approach is to add a new Expert option to enable the advanced options in the create dialog.

Screenshot 2019-06-25 at 15 53 49

Please help review base PR #16261.

in src/qt/walletcontroller.cpp:8 in 54e15502f7 outdated

1@@ -2,8 +2,15 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5+#include <qt/guiutil.h>
6+#include <qt/askpassphrasedialog.h>
7+#include <qt/createwalletdialog.h>
8+#include <qt/guiconstants.h>
9+#include <qt/guiutil.h>

fanquake commented at 5:24 am on June 27, 2019:

Travis is not running because of the duplicate <qt/guiutil.h> include.

achow101 commented at 2:39 pm on June 27, 2019:

Fixed

fanquake commented at 5:44 am on June 27, 2019: member

@achow101 Thanks for the response.

IMO the end user in mind is the completely non-technical user who just downloaded Bitcoin Core. The advanced options are for the more technical users.

We’d have Encrypt Wallet checked by default, with password setting part of the create wallet process.

👍

I tried having the advanced options hidden with a drop down, but I don’t know enough about Qt to make that happen.

It’d be great if someone who knows more Qt magic could do that.

I don’t want to block the PR on this. We can open a followup issue outlining some ways the UI can be improved.

Of course the wallet has private keys disabled so nothing is actually encrypted, but it didn’t do nothing, and the wallet is still technically encrypted (under our definition of encrypted wallet).

My concern was was more inconsistency in the GUI. When you finish the wallet creation process (after choosing encrypt and disable private), you don’t have the lock icon in the bottom right, and you can go to Settings -> Encrypt Wallet.. and encrypt your wallet “again”. Does the user now think their wallet is “double” encrypted?

Regardless of the above, here are some screenshots of how this PR (54e15502f764dc131bc7d47fc98db54ac7234ddf) currently looks on macOS, on top of master (3077f11dadffbc8f2575449fc0177c91a9c3e046), using Qt 5.12.3:

create_encrypt

disable_priv_keys

blank_wallet

creating

achow101 force-pushed on Jun 27, 2019

achow101 commented at 2:39 pm on June 27, 2019: member

When you finish the wallet creation process (after choosing encrypt and disable private), you don’t have the lock icon in the bottom right,

I checked the code again and it actually doesn’t encrypt the wallet when WALLET_FLAG_DISABLE_PRIVATE_KEYS is set. I’ve changed this to now disable and uncheck the Disable Private Keys checkbox when the Encrypt Wallet checkbox is checked.

achow101 force-pushed on Jun 27, 2019

laanwj removed this from the "Blockers" column in a project

Sjors commented at 0:18 am on June 28, 2019: member

Lightly tested e0da87275b3febf338c017ef2d0c67fdc17d4c6b on macOS, but didn’t review the code. Imo the UI is good enough now for a first version. Looking forward to adding hardware support on top of this :-)

in src/qt/walletcontroller.cpp:184 in e0da87275b outdated

194-    : m_wallet_controller(wallet_controller)
195-    , m_name(name)
196-{}
197+void CreateWalletActivity::askPasshprase()
198+{
199+    m_passphrase_dialog = new AskPassphraseDialog(AskPassphraseDialog::Encrypt, m_parent_widget, &m_passphrase);

fanquake commented at 8:51 am on June 29, 2019:

A call to m_passphrase_dialog->setWindowModality(Qt::ApplicationModal); before ->show(); seems to fix the autofocussing.

achow101 commented at 8:37 pm on July 9, 2019:

Done

fanquake commented at 2:21 am on July 10, 2019:

Great, thanks.

fanquake commented at 8:54 am on June 29, 2019: member

I’ve changed this to now disable and uncheck the Disable Private Keys checkbox when the Encrypt Wallet checkbox is checked.

Nice, that seems better.

It’d be great if someone who knows more Qt magic could do that.

I’ve added a comment inline about autofocussing the passphrase dialog.

Also did some testing using depends Qt.

DrahtBot added the label Needs rebase on Jul 8, 2019

achow101 force-pushed on Jul 9, 2019

fanquake removed the label Needs rebase on Jul 10, 2019

fanquake added the label Needs rebase on Jul 10, 2019

fanquake commented at 2:22 am on July 10, 2019: member

@achow101 Looks like something might have gone wrong during the rebase. The whitespace linter is also having a sad:

0This diff appears to have added new lines with trailing whitespace.
1The following changes were suspected:
2diff --git a/src/qt/walletcontroller.h b/src/qt/walletcontroller.h
3@@ -48,0 +57,2 @@ public:
4+
5^---- failure generated from test/lint/lint-whitespace.sh

achow101 force-pushed on Jul 10, 2019

achow101 commented at 6:55 pm on July 10, 2019: member

@achow101 Looks like something might have gone wrong during the rebase. The whitespace linter is also having a sad:

Huh. I’ve reset to e0da872 and re-rebased.

DrahtBot removed the label Needs rebase on Jul 10, 2019

in src/qt/createwalletdialog.cpp:19 in c5d0860c86 outdated

14+CreateWalletDialog::CreateWalletDialog(QWidget* parent) :
15+    QDialog(parent),
16+    ui(new Ui::CreateWalletDialog)
17+{
18+    ui->setupUi(this);
19+    ui->buttonBox->button(QDialogButtonBox::Ok)->setText("Create");

Sjors commented at 9:41 am on July 11, 2019:

This won’t get translated?

achow101 commented at 8:14 pm on July 12, 2019:

Fixed.

in src/qt/createwalletdialog.cpp:24 in c5d0860c86 outdated

19+    ui->buttonBox->button(QDialogButtonBox::Ok)->setText("Create");
20+    ui->buttonBox->button(QDialogButtonBox::Ok)->setEnabled(false);
21+    ui->wallet_name_line_edit->setFocus(Qt::ActiveWindowFocusReason);
22+
23+    connect(ui->wallet_name_line_edit, &QLineEdit::textEdited, [this](const QString& text) {
24+        ui->buttonBox->button(QDialogButtonBox::Ok)->setEnabled(!text.isEmpty());

Sjors commented at 9:45 am on July 11, 2019:

In the future this should be moved to a validate() function, because there may be more factors that influence if OK should be enabled.

achow101 commented at 8:04 pm on July 12, 2019:

Something for a followup.

in src/qt/askpassphrasedialog.cpp:139 in 985620988a outdated

148+                    {
149+                        QMessageBox::warning(this, tr("Wallet encrypted"),
150+                                             "<qt>" +
151+                                             tr("Your wallet is now encrypted. "
152+                                             "Remember that encrypting your wallet cannot fully protect "
153+                                             "your bitcoins from being stolen by malware infecting your computer.") +

Sjors commented at 9:59 am on July 11, 2019:

Nit: avoid duplicating the above text, if only to make life easier on translators.

achow101 commented at 8:14 pm on July 12, 2019:

Done

Sjors approved

Sjors commented at 10:10 am on July 11, 2019: member

ACK d5d2728d43cb761dcf5da9a3ab8a37a0bd54cdb0, but I’d like more QT expert eyes on d65d0ec111a077568629669d31843360cf6eaef8 (gui: Refactor OpenWalletActivity) and b1a74a1d97d2d507af3ba1250197636d2604bbb1 (Expose wallet creation to the GUI via WalletController).

achow101 force-pushed on Jul 12, 2019

Sjors commented at 3:40 pm on July 13, 2019: member

re-ACK cb92402c12f184c3d9cefd7ab5ea3a8a7155293e

Sjors commented at 11:36 am on August 5, 2019: member

When I rebase on the latest master I can no longer compile (as a result of #16399):

0  CXXLD    qt/bitcoin-qt
1Undefined symbols for architecture x86_64:
2  "CreateWallet(interfaces::Chain&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&, WalletCreationStatus&, std::__1::basic_string<char, std::__1::char_traits<char>, secure_allocator<char> > const&, unsigned long long)", referenced from:
3      interfaces::(anonymous namespace)::NodeImpl::createWallet(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >&, std::__1::basic_string<char, std::__1::char_traits<char>, secure_allocator<char> > const&, unsigned long long) in libbitcoin_server.a(libbitcoin_server_a-node.o)
4ld: symbol(s) not found for architecture x86_64
5clang: error: linker command failed with exit code 1 (use -v to see invocation)
6make[1]: *** [qt/bitcoin-qt] Error 1
7make: *** [src/qt/bitcoin-qt] Error 2

This makes the error go away: https://github.com/Sjors/bitcoin/commit/f0b8811e4488e3b4a666213ecb572bb91e833a02

achow101 force-pushed on Aug 5, 2019

achow101 commented at 10:25 pm on August 5, 2019: member

Rebased and fixed.

in src/interfaces/node.cpp:266 in 6da865555e outdated

259@@ -257,6 +260,12 @@ class NodeImpl : public Node
260     {
261         return MakeWallet(LoadWallet(*m_interfaces.chain, name, error, warning));
262     }
263+    std::unique_ptr<Wallet> createWallet(const std::string& name, std::string& error, std::string& warning, const SecureString& passphrase, uint64_t wallet_creation_flags) override
264+    {
265+        std::shared_ptr<CWallet> wallet;
266+        WalletCreationStatus status = CreateWallet(*m_interfaces.chain, passphrase, wallet_creation_flags, name, error, warning, wallet);

Sjors commented at 9:45 am on August 6, 2019:

Unused variable status. Also it would be more consistent to make the interface method match the new pattern.

achow101 commented at 6:04 pm on August 6, 2019:

Changed the interface to match the new pattern.

Sjors commented at 9:50 am on August 6, 2019: member

I think that missed a few spots. Also --disable-wallet still has the same error.

achow101 force-pushed on Aug 6, 2019

achow101 commented at 6:04 pm on August 6, 2019: member

fixed --disable-wallet.

Sjors commented at 7:02 pm on August 6, 2019: member

re-ACK c41d2f68d

DrahtBot commented at 7:40 am on August 16, 2019: member

DrahtBot added the label Needs rebase on Aug 16, 2019

achow101 commented at 0:39 am on August 17, 2019: member

Rebased

achow101 force-pushed on Aug 17, 2019

fanquake removed the label Needs rebase on Aug 17, 2019

Sjors commented at 11:44 am on August 17, 2019: member

Still compiles on macOS and seems to work as before. Changes between da8a86c and my previous ACK at c41d2f6 are mostly in the “Refactor OpenWalletActivity” commit, so will defer to @promag.

laanwj added this to the "Blockers" column in a project

fanquake renamed this:
~~[GUI] Create wallet menu option~~
gui: Create wallet menu option
on Aug 30, 2019

fanquake approved

fanquake commented at 8:12 am on August 31, 2019: member

ACK da8a86ce7d1d685c1149fa294b8cc26b9c1d4b74 - Haven’t reviewed all of the Qt changes in depth. However I would like to see this in for v0.19.0. If this is merged soon, then we’ve still got time to shake out any bugs and get follow up changes in.

Tested creating the different types of wallets.

Checked that when Encrypt Wallet is selected Disable Private Keys is disabled. Also checked that if Disable Private Keys was checked before Encrypt Wallet is selected it gets unchecked.

Looks like my auto-focusing suggestion might have been lost in a rebase? However at this point it’s definitely not a blocker for merging:

0m_passphrase_dialog->setWindowModality(Qt::ApplicationModal); 
1m_passphrase_dialog->show();

This is at least one inconsistency where if you create a wallet with a HTML escaped character in the name, i.e Alice & Bob, then the & will not be shown in some menus / dialogs.

meshcollider requested review from promag on Sep 5, 2019

meshcollider commented at 1:37 pm on September 5, 2019: contributor

I agree with fanquake, I’ll review+test tomorrow but I think I will merge it after that to give proper time for testing as mentioned. Please review before then everyone :D

in src/qt/forms/createwalletdialog.ui:65 in da8a86ce7d outdated

60+     <width>171</width>
61+     <height>22</height>
62+    </rect>
63+   </property>
64+   <property name="toolTip">
65+    <string>Encrypt the wallet. The wallet will be encrypted with a password of your choice.</string>

jonatack commented at 6:20 pm on September 5, 2019:

s/password/passphrase/ given that the dialog in question asks the user to input passphrases, not passwords

in src/qt/createwalletdialog.cpp:48 in da8a86ce7d outdated

43+QString CreateWalletDialog::walletName() const
44+{
45+    return ui->wallet_name_line_edit->text();
46+}
47+
48+bool CreateWalletDialog::encrypt() const

jonatack commented at 6:25 pm on September 5, 2019:

Nit: it would be clearer to name this as encryptWallet here and makeBlankWallet on line 58, the way disablePrivateKeys is named.

in src/qt/createwalletdialog.h:29 in da8a86ce7d outdated

24+    virtual ~CreateWalletDialog();
25+
26+    QString walletName() const;
27+    bool encrypt() const;
28+    bool disablePrivateKeys() const;
29+    bool blank() const;

jonatack commented at 6:28 pm on September 5, 2019:

Nit: following my comment above, naming these encryptWallet and makeBlankWallet (like disablePrivateKeys) seems more precise/coherent/would improve git grepping.

in src/qt/guiconstants.h:8 in da8a86ce7d outdated

4@@ -5,6 +5,8 @@
5 #ifndef BITCOIN_QT_GUICONSTANTS_H
6 #define BITCOIN_QT_GUICONSTANTS_H
7 
8+#include <cstdint>

jonatack commented at 6:32 pm on September 5, 2019:

For my understanding, why is this now needed?

in src/qt/walletcontroller.cpp:211 in da8a86ce7d outdated

223-        Q_EMIT message(QMessageBox::Critical, QString::fromStdString(error));
224+    if (m_create_wallet_dialog->blank()) {
225+        flags |= WALLET_FLAG_BLANK_WALLET;
226     }
227+
228+    QTimer::singleShot(500, worker(), [this, name, flags] {

jonatack commented at 6:34 pm on September 5, 2019:

Nit: would be nice to hoist the 4 occurrences of this magic value (500) to a helpfully named constant.

jonatack commented at 6:53 pm on September 5, 2019: member

ACK da8a86ce7d1d685c1149fa294b8cc26b9c1d4b74. Built, ran all tests, manually tested the gui wallet with Debian, reviewed the code.

Verified:

when “Encrypt Wallet” is selected, “Disable Private Keys” is disabled
when “Disable Private Keys” is checked and then “Encrypt Wallet” is selected, “Disable” is unchecked

I’m not sure what auto-focus issue @fanquake is seeing in #15450#pullrequestreview-282297760. The focus behaves well in my testing. It begins automatically in the name field, and tabbing moves it successively through all the fields in the correct order.

0if you create a wallet with a HTML escaped character in the name, i.e Alice & Bob,
1then the & will not be shown in some menus / dialogs.

Saw the same issue on Debian. Among the special characters I entered, only the & seemed to be affected.

A few comments follow. Feel free to ignore or add them to the to-do list for the future.

gui: Refactor OpenWalletActivity bc6d8a3662

promag commented at 11:06 pm on September 5, 2019: member

@achow101 please rebase, here’s the trivial diff from my PR:

 0diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp
 1index d0ffd6ba3..88ccc8c2d 100644
 2--- a/src/qt/walletcontroller.cpp
 3+++ b/src/qt/walletcontroller.cpp
 4@@ -160,10 +160,6 @@ void WalletControllerActivity::showProgressDialog(const QString& label_text)
 5     m_progress_dialog->setCancelButton(nullptr);
 6     m_progress_dialog->setWindowModality(Qt::ApplicationModal);
 7     GUIUtil::PolishProgressDialog(m_progress_dialog);
 8-
 9-    connect(m_progress_dialog, &QObject::destroyed, [this] {
10-        m_progress_dialog = nullptr;
11-    });
12 }
13
14 OpenWalletActivity::OpenWalletActivity(WalletController* wallet_controller, QWidget* parent_widget)
15@@ -192,11 +188,11 @@ void OpenWalletActivity::open(const std::string& path)
16
17     showProgressDialog(tr("Opening Wallet <b>%1</b>...").arg(name.toHtmlEscaped()));
18
19-    QTimer::singleShot(500, worker(), [this, path] {
20+    QTimer::singleShot(0, worker(), [this, path] {
21         std::unique_ptr<interfaces::Wallet> wallet = node().loadWallet(path, m_error_message, m_warning_message);
22
23         if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
24
25-        QTimer::singleShot(500, this, &OpenWalletActivity::finish);
26+        QTimer::singleShot(0, this, &OpenWalletActivity::finish);
27     });
28 }
29diff --git a/src/qt/walletcontroller.h b/src/qt/walletcontroller.h
30index da8fde9f5..dada9cfa6 100644
31--- a/src/qt/walletcontroller.h
32+++ b/src/qt/walletcontroller.h
33@@ -46,10 +46,11 @@ public:
34     //! Returns wallet models currently open.
35     std::vector<WalletModel*> getOpenWallets() const;
36
37+    WalletModel* getOrCreateWallet(std::unique_ptr<interfaces::Wallet> wallet);
38+
39     //! Returns all wallet names in the wallet dir mapped to whether the wallet
40     //! is loaded.
41     std::map<std::string, bool> listWalletDir() const;
42-    WalletModel* getOrCreateWallet(std::unique_ptr<interfaces::Wallet> wallet);
43
44     void closeWallet(WalletModel* wallet_model, QWidget* parent = nullptr);

promag commented at 11:35 pm on September 5, 2019: member

Tested ACK on macOS 10.14.3 (rebased with my branch), played around, looks good to me.

This is at least one inconsistency where if you create a wallet with a HTML escaped character in the name, i.e Alice & Bob, then the & will not be shown in some menus / dialogs. @fanquake something to fix next.

Optionally allow AskPassphraseDialog to output the passphrase 60adb21c7a

Add CreateWalletDialog to create wallets from the GUI

Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>

78863e2900

Expose wallet creation to the GUI via WalletController

Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>

9b41cbb28f

Add Create Wallet menu action

Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>

613de61a04

achow101 force-pushed on Sep 6, 2019

achow101 commented at 0:38 am on September 6, 2019: member

Rebased onto #16261

sipa commented at 6:42 pm on September 6, 2019: member

Concept and approach ACK

fanquake commented at 6:29 am on September 7, 2019: member

ACK 613de61a04c210a51af9997e69f66439a17a632a - re-reviewed on macOS. I’m going to merge this now. It’s had a stack of review, and as mentioned multiple times above, lets get this into master so it can get more testing pre v0.19.0.

Checked that the gui: Refactor OpenWalletActivity commit in this PR matches the one in #16261. So will close #16261 post merge here.

Re-rested creating the different wallet types.

Checked that when Encrypt Wallet is selected Disable Private Keys is disabled. Also checked that if Disable Private Keys was checked before Encrypt Wallet is selected it gets unchecked.

Checked that there still aren’t any issues when compiling with --disable-wallet.

I’m not sure what auto-focus issue @fanquake is seeing

At least on macOS, if you’re creating an encrypted wallet, after clicking Create the Encrypt wallet dialog isn’t focused, so you can’t type until you click on it.

Screenshots:

Follow up for this PR include:

Encrypt wallet focussing (maybe macOS only).
HTML escaping issue
A couple nits from jonatack review

—–BEGIN PGP SIGNED MESSAGE—– Hash: SHA256

ACK 613de61a04c210a51af9997e69f66439a17a632a - re-reviewed on macOS. I’m going to merge this now. It’s had a stack of review, and as mentioned multiple times above, lets get this into master so it can get more testing pre v0.19.0. —–BEGIN PGP SIGNATURE—–

iQIzBAEBCAAdFiEEz7FuIclQ9n+pXlWPLuufXMCVJsEFAl1zUgAACgkQLuufXMCV JsGSEg//QrKZvNXQ6Jwom8qsfw3Zlcd7rIoJoLI9jfhXEGzIDi9yrTryrYWTzaqq PVp8aeFyQtXPg9a06mWaQJG0Gb2sQRv6rUAba8fLfDiEg+ycFzMRsd53o3fY0O+a FhTmE2hdkVQnwZX9yDmyRKnbmL2l0FP9zGqfas78yHVkq47zRe7dbcTQcLyLjhck sFAnZGJVgyko3ynRsk4YqIk3e5ALMcVXzJIXknbpiHlmwb346on1O08oiPMjArmo 3lDrEF1QuOs7DXrmW5y/aaCgd06EtqlZ1ufL1ISSHM8lah/sRrDkF+TMymJ6yLie pIrOQu8UUkLLh8hUsmHMlMtFksQHUSBRI2XYuYkfHJI6TB/M0f4UfCz7nq1VlLyZ YUKl4r6hgy+/GPrjQkf7b80zwpLdC0xaiNGI9S2zfpIk0rYJxkrB+2AKpmRQZsCX B004jEICoe6elyLtepAwm77UCDXz6cA3XDhlYT1415nWm3MM4mRgGPKMHe1K19A8 YyhrUfPI5ZCS2hEim5g/6VCYeY4HzOi9OlsZnzFhQbL2ImgNlyMQ3OC2KiSxJrg/ 4yEahL+zIGYe/esBu7rrRV76j6Q0c5ngkjSVMR9+ZhHV49T5AWUYQKwdUENc3TM9 gtk2q9Zi5cnLlSDPOtPW7NxWT0oKzHNJ5VmS8EdWGZ1aMZwF+UE= =AV8M —–END PGP SIGNATURE—–

fanquake referenced this in commit b5a8d0cff1 on Sep 7, 2019

fanquake merged this on Sep 7, 2019

fanquake closed this on Sep 7, 2019

fanquake removed this from the "Blockers" column in a project

fanquake referenced this in commit a953429a0e on Sep 16, 2019

sidhujag referenced this in commit 2b7ff1f581 on Sep 16, 2019

fanquake moved this from the "In progress" to the "Done" column in a project

deadalnix referenced this in commit e677cd7437 on Aug 3, 2020

deadalnix referenced this in commit 1d0e49b855 on Aug 3, 2020

jasonbcox referenced this in commit b7a0314489 on Aug 3, 2020

jasonbcox referenced this in commit d41a0be9c0 on Aug 3, 2020

jasonbcox referenced this in commit a87a2f00ae on Aug 3, 2020

MarcoFalke locked this on Dec 16, 2021

gui: Create wallet menu option #15450

Conflicts