This PR adds support to close the current wallet in the GUI.
<img width="543" alt="screenshot 2019-01-18 at 00 44 26" src="https://user-images.githubusercontent.com/3534524/51358241-424b9680-1aba-11e9-88f2-b85869507737.png"> <img width="532" alt="screenshot 2019-01-18 at 00 44 38" src="https://user-images.githubusercontent.com/3534524/51358242-424b9680-1aba-11e9-83e2-fa275a9017b3.png">
Ack https://github.com/bitcoin/bitcoin/pull/15195/commits/2fef1852cfacd5a435d78cee51c0059061941dc0 Tested open and close wallet behaviour with multiple wallets loaded
Concept ACK.
Tested works for me on Debian stable. Code not reviewed. I did get some compiler warnings:
<details>
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);
^~~~~~~~~~</details>
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
Are you sure you wish to close wallet ??!??
This would look slightly less confusing:
Are you sure you wish to close wallet "??!?"?
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
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?
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);
Isn't this declaration redundant?
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()) {
path should be const reference?
48 | + return wallets; 49 | +} 50 | + 51 | +WalletModel* WalletController::openWallet(const std::string& name, QWidget* parent) 52 | +{ 53 | + std::string error, warning;
Nit: Shadows function error from system.h.
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 | +
It could be written without button variable:
if (QMessageBox::Yes != QMessageBox::question(parent, tr("Close wallet"),
tr("Are you sure you wish to close wallet %1?").arg(wallet_model->getDisplayName()),
QMessageBox::Yes|QMessageBox::Cancel,
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
textparameter.
Done.
Needs rebase now that open wallet is merged.
@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.
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.
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
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<CWallet>, 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.)
@jonasschnelli @ryanofsky WDYT about #15195 (comment)?
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.
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)?
@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&.
void WalletImpl::unload() override
{
RemoveWallet(m_shared_wallet);
UnloadWallet(std::move(m_shred_wallet));
// now m_wallet is invalid, and this interface is also "null"
// also, this interface would be deleted by the unload notification (?)
}
@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.
Tested ACK 94086fb59d1a05eb9a2960c2aae242e49c0a9103 This is a good first step.
@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.
Milestone
0.18.0