gui: Add Close Wallet action

promag commented at 0:45 am on January 18, 2019: member

This PR adds support to close the current wallet in the GUI.

fanquake added the label GUI on Jan 18, 2019

IlyasRidhuan commented at 5:39 am on January 18, 2019: none

Ack https://github.com/bitcoin/bitcoin/pull/15195/commits/2fef1852cfacd5a435d78cee51c0059061941dc0 Tested open and close wallet behaviour with multiple wallets loaded

promag renamed this:
~~gui: Add close wallet action~~
gui: Add Close Wallet action
on Jan 18, 2019

hebasto commented at 9:27 am on January 18, 2019: member

Concept ACK.

harding commented at 11:53 am on January 18, 2019: contributor

Tested works for me on Debian stable. Code not reviewed. I did get some compiler warnings:

  CXX      interfaces/libbitcoin_server_a-node.o
interfaces/node.cpp:42:10: warning: redundant redeclaration of ‘boost::filesystem::path GetWalletDir()’ in same scope [-Wredundant-decls]
 fs::path GetWalletDir();
          ^~~~~~~~~~~~
In file included from interfaces/node.cpp:31:0:
./wallet/walletutil.h:13:10: note: previous declaration of ‘boost::filesystem::path GetWalletDir()’
 fs::path GetWalletDir();
          ^~~~~~~~~~~~
interfaces/node.cpp:43:23: warning: redundant redeclaration of ‘std::vector<boost::filesystem::path> ListWalletDir()’ in same scope [-Wredundant-decls]
 std::vector<fs::path> ListWalletDir();
                       ^~~~~~~~~~~~~
In file included from interfaces/node.cpp:31:0:
./wallet/walletutil.h:16:23: note: previous declaration of ‘std::vector<boost::filesystem::path> ListWalletDir()’
 std::vector<fs::path> ListWalletDir();
                       ^~~~~~~~~~~~~



  CXX      wallet/libbitcoin_wallet_a-rpcwallet.o
wallet/rpcwallet.cpp:2494:26: warning: redundant redeclaration of ‘std::shared_ptr<CWallet> LoadWallet(interfaces::Chain&, const WalletLocation&, std::__cxx11::string&, std::__cxx11::string&)’ in same scope [-Wredundant-decls]
 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::string& warning);
                          ^~~~~~~~~~
In file included from ./wallet/coincontrol.h:11:0,
                 from wallet/rpcwallet.cpp:31:
./wallet/wallet.h:69:26: note: previous declaration of ‘std::shared_ptr<CWallet> LoadWallet(interfaces::Chain&, const WalletLocation&, std::__cxx11::string&, std::__cxx11::string&)’
 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::string& warning);
                          ^~~~~~~~~~

jnewbery added this to the "Issues" column in a project

jnewbery moved this from the "Issues" to the "In progress" column in a project

flack commented at 8:42 pm on January 18, 2019: contributor

Is it possible to visually separate the wallet name from the question text in the confirmation dialog? For example by putting quotes around it or adding a line break before.

Silly example: A wallet named ??!? would currently render

0Are you sure you wish to close wallet ??!??

This would look slightly less confusing:

0Are you sure you wish to close wallet "??!?"?

promag commented at 8:47 pm on January 18, 2019: member

@flack good point, I’ll see other examples too.

DrahtBot commented at 10:42 pm on January 18, 2019: member

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

Conflicts

No conflicts as of last run.

Sjors commented at 7:37 am on January 19, 2019: member

Concept ACK.

When the node is pruned, can you add a warning that closing the wallet for too long can result in having to resync the entire chain?

in src/wallet/rpcwallet.cpp:2494 in 2fef1852cf outdated

2490@@ -2491,6 +2491,8 @@ static UniValue listwallets(const JSONRPCRequest& request)
2491     return obj;
2492 }
2493 
2494+std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::string& warning);

practicalswift commented at 2:19 pm on January 19, 2019:

Isn’t this declaration redundant?

in src/qt/bitcoingui.cpp:373 in 2fef1852cf outdated

367@@ -360,6 +368,19 @@ void BitcoinGUI::createActions()
368         connect(usedSendingAddressesAction, &QAction::triggered, walletFrame, &WalletFrame::usedSendingAddresses);
369         connect(usedReceivingAddressesAction, &QAction::triggered, walletFrame, &WalletFrame::usedReceivingAddresses);
370         connect(openAction, &QAction::triggered, this, &BitcoinGUI::openClicked);
371+        connect(m_open_wallet_action->menu(), &QMenu::aboutToShow, [this] {
372+            m_open_wallet_action->menu()->clear();
373+            for (std::string path : m_wallet_controller->getWalletsAvailableToOpen()) {

practicalswift commented at 2:20 pm on January 19, 2019:

path should be const reference?

in src/qt/walletcontroller.cpp:54 in 2fef1852cf outdated

48+    return wallets;
49+}
50+
51+WalletModel* WalletController::openWallet(const std::string& name, QWidget* parent)
52+{
53+    std::string error, warning;

practicalswift commented at 2:27 pm on January 19, 2019:

Nit: Shadows function error from system.h.

promag force-pushed on Jan 19, 2019

in src/qt/walletcontroller.cpp:75 in b1aa998997 outdated

67+    QMessageBox::StandardButton button = QMessageBox::question(parent, tr("Close wallet"),
68+        tr("Are you sure you wish to close wallet %1?").arg(wallet_model->getDisplayName()),
69+        QMessageBox::Yes|QMessageBox::Cancel,
70+        QMessageBox::Yes);
71+    if (button != QMessageBox::Yes) return;
72+

hebasto commented at 7:57 pm on January 20, 2019:

It could be written without button variable:

0    if (QMessageBox::Yes != QMessageBox::question(parent, tr("Close wallet"),
1        tr("Are you sure you wish to close wallet %1?").arg(wallet_model->getDisplayName()),
2        QMessageBox::Yes|QMessageBox::Cancel,
3        QMessageBox::Yes)) return;

Also Qt docs do not recommend use the Static Functions API:

Using the property-based API is recommended.

The Microsoft Windows User Interface Guidelines recommend using the application name as the window’s title, which means that if you have an informative text in addition to your main text, you must concatenate it to the text parameter.

promag commented at 11:03 pm on February 12, 2019:

Done.

meshcollider added this to the milestone 0.18.0 on Feb 8, 2019

meshcollider added the label Wallet on Feb 10, 2019

Sjors commented at 6:26 pm on February 12, 2019: member

Needs rebase now that open wallet is merged.

DrahtBot added the label Needs rebase on Feb 12, 2019

interfaces: Add remove to Wallet f6122abe03

gui: Add closeWallet to WalletController f77ba34313

gui: Add close wallet action 94086fb59d

promag force-pushed on Feb 12, 2019

DrahtBot removed the label Needs rebase on Feb 12, 2019

promag commented at 11:03 pm on February 12, 2019: member

@flack currently the wallet name is in italic. @practicalswift you reviewed commits that belong #15153, but I’ll take them into account once I touch that code again. @hebasto changed to property based. @Sjors rebased and added your suggestion.

promag commented at 11:07 pm on February 12, 2019: member

Note to reviewers, closing the wallet doesn’t explicitly unloads it, just tries to drop all references to the wallet - in other words, it doesn’t wait for some RPC worker thread, if any, to finish its job.

Please give feedback if this should unload the wallet, like unloadwallet RPC.

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

Sjors commented at 9:39 am on February 13, 2019: member

Eventually it should unload the wallet, because an advanced user may want to be able to move files around. It would also make it consistent with the RPC if they both call UnloadWallet() (which would have to added to an interface).

It can be done later as long as it doesn’t break stuff. I was able to open and close the same wallet multiple times without a crash, so that’s good.

Much more useful, once #11082 is merged, is for this menu item to remove the wallet from rw_config, so that it remains closed on the next launch.

tACK 94086fb

gwillen commented at 9:45 am on February 13, 2019: contributor

Tested on OS X, seems to work great. tACK 94086fb, notwithstanding the kibbitz below about unloading.

As to unloading, am I correctly understanding the process: the unloadwallet RPC will block until the wallet is unloaded, which could take time if someone has taken a reference to the shared_ptr, because UnloadWallet will block until they drop it. The approach taken in this PR is not to block, but to remove the wallet from the UI’s list immediately; if someone still has a reference to it, it will not actually be unloaded until they drop the reference?

It seems like the worst case here is: a user closes a wallet on which some long-running operation is holding the CWallet open. (Is this possible? I don’t know how long a wallet can take to unload.) Then the wallet will appear in the Open Wallet… menu, since getWalletsAvailableToOpen will not find it already open (since it’s searching the WalletController’s WalletModel list, not the CWallet list.) But if it’s clicked, opening it will fail, since it’s still open. And if I’m following the logic right, a critical messagebox will pop up displaying the message: “Wallet file verification failed: Error loading wallet [blah].dat. Duplicate -wallet filename specified. (code -4)”

This is certainly a little weird but doesn’t seem terrible.

(But now I see Sjors’s comment, and it’s an interesting point that a power user may want to touch the wallet file, and may assume that when it disappears from the list in the GUI, that means it’s safe to move it, which would be disastrous if it’s not unloaded. If this really is an issue, I don’t see any way around this other than putting up a modal “unloading…” dialog, ultimately.)

promag commented at 10:11 am on February 13, 2019: member

@Sjors right, I thought the same. In order to not block UI with UnloadWallet I have to create a CloseWalletActivity, so IMO definitely something to improve later - I mean, can be improved even after feature freeze (I think?).

promag commented at 10:14 pm on February 13, 2019: member

@jonasschnelli @ryanofsky WDYT about #15195 (comment)?

ryanofsky commented at 10:35 pm on February 13, 2019: member

If the question is what do I think should happen when a wallet is slow to unload, it’s the same as what I wrote in #15153 (review) about what should I think should happen when a wallet is slow to load. Wallet display should show “Unloading wallet X…” while the wallet is unloading, and when it’s done unloading, the wallet should be removed from the dropdown list, and if it was still selected, the next item in the list should be selected instead.

While the wallet is unloading, the user should be able to switch to other wallets, load and unload other wallets, shut down the program, and do everything else normally except interact with the wallet that’s being unloaded. There shouldn’t be any popups or modal unloading dialogs or anything like that.

jonasschnelli commented at 1:45 am on February 14, 2019: contributor

I agree with @ryanofsky. As a long term goal. Unsure if this can be broken down to simpler steps. A wallet can – currently – consume a decent amount of memory (and eventually CPU) and closing a wallet should IMO unload the wallet (graceful).

I haven’t looked at the code yet,… is there a reason to not unload the wallet synchronously as a first step (blocking the GUI as an initial close seems acceptable)?

promag commented at 3:07 pm on February 14, 2019: member

@jonasschnelli IMO currently it’s not so easily done, it’s not the same as the RPC unloadwallet where there is only a std::shared_ptr<CWallet>. Here we have interfaces::Wallet which has a member CWallet&.

0void WalletImpl::unload() override
1{
2    RemoveWallet(m_shared_wallet);
3    UnloadWallet(std::move(m_shred_wallet));
4    // now m_wallet is invalid, and this interface is also "null"
5    // also, this interface would be deleted by the unload notification (?)
6}

@ryanofsky what do you suggest to fix the above? I thought not calling UnloadWallet and then wait for the unload notification..

Anyway, I think this is ready to merge and then I can follow up with improvements.

jonasschnelli commented at 8:46 pm on February 14, 2019: contributor

Tested ACK 94086fb59d1a05eb9a2960c2aae242e49c0a9103 This is a good first step.

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

jonasschnelli merged this on Feb 14, 2019

jonasschnelli closed this on Feb 14, 2019

jonasschnelli referenced this in commit b7456e6bf9 on Feb 14, 2019

ryanofsky commented at 8:54 pm on February 14, 2019: member

@ryanofsky what do you suggest to fix the above?

What you suggested looks good, but WalletImpl class should just have a single std::shared_ptr<CWallet> m_wallet member, not two wallet members.

promag deleted the branch on Feb 17, 2019

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

deadalnix referenced this in commit 30b314cb2e on May 20, 2020

deadalnix referenced this in commit f51a83856e on May 21, 2020

deadalnix referenced this in commit e44e705acc on May 21, 2020

PastaPastaPasta referenced this in commit b17f7b6707 on Sep 10, 2021

PastaPastaPasta referenced this in commit f5a754c8ee on Oct 23, 2021

PastaPastaPasta referenced this in commit 808f77bea0 on Oct 25, 2021

pravblockc referenced this in commit d984375e65 on Nov 18, 2021

DrahtBot locked this on Dec 16, 2021

gui: Add Close Wallet action #15195

Conflicts