gui: Add Close All Wallets action #15202

pull promag wants to merge 2 commits into bitcoin:master from promag:2019-01-closeallwallets changing 4 files +27 −1

promag commented at 3:32 PM on January 18, 2019: member

This PR adds the action to close all wallets.

<img width="405" alt="Screenshot 2020-06-01 at 01 06 12" src="https://user-images.githubusercontent.com/3534524/83365986-25a8b980-a3a4-11ea-9613-24dcd8eaa55c.png">
DrahtBot commented at 10:40 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.
fanquake added the label GUI on Jan 18, 2019
Sjors commented at 3:10 PM on January 19, 2019: member

I'm Concept Meh on this.
promag force-pushed on Jan 19, 2019
hebasto changes_requested
hebasto commented at 7:42 PM on January 20, 2019: member

Concept ACK.
meshcollider added the label Wallet on Feb 10, 2019
DrahtBot added the label Needs rebase on Feb 12, 2019
promag force-pushed on Mar 17, 2019
DrahtBot removed the label Needs rebase on Mar 17, 2019
in src/qt/walletcontroller.cpp:97 in 29686af11a outdated
```
  77 | @@ -78,6 +78,23 @@ void WalletController::closeWallet(WalletModel* wallet_model, QWidget* parent)
  78 |      removeAndDeleteWallet(wallet_model);
  79 |  }
  80 |  
  81 | +void WalletController::closeAllWallets(QWidget* parent)
  82 | +{
  83 | +    QMessageBox::StandardButton button = QMessageBox::question(parent, tr("Close all wallets"),
```
promag commented at 10:24 AM on March 24, 2019:

We want to avoid these sync calls (these create a temporary event loop) so here there's 2 options:

merge this and later remove all usages in the code

fix this one now

Not sure what is preferable.
jonasschnelli commented at 7:52 AM on April 16, 2019:

Just use exec and a lambda?
promag commented at 10:31 AM on March 24, 2019: member

Rebased. Currently it creates a wallet if the folder is not a wallet, and I think this shouldn't be the case - it should show an error message "Not a wallet" or something. Will fix.
jonasschnelli commented at 7:52 AM on April 16, 2019: contributor

Concept ACK

in src/qt/bitcoingui.cpp:353 in 29686af11a outdated

 340 | @@ -341,6 +341,9 @@ void BitcoinGUI::createActions()
 341 |      m_close_wallet_action = new QAction(tr("Close Wallet..."), this);
 342 |      m_close_wallet_action->setStatusTip(tr("Close wallet"));
 343 |  
 344 | +    m_close_all_wallets_action = new QAction(tr("Close All Wallets..."), this);

luke-jr commented at 6:27 AM on April 17, 2019:

I'm not sure if ... is appropriate here, just for a confirmation dialog?

promag commented at 6:09 PM on May 27, 2019:

I know the feeling, I'm just following the "rule".

DrahtBot added the label Needs rebase on Aug 16, 2019
jonasschnelli commented at 9:52 AM on October 9, 2019: contributor

Needs rebase
promag commented at 3:33 AM on November 3, 2019: member

@luke-jr do you concept ACK?
luke-jr commented at 4:18 AM on November 3, 2019: member

I think so? Seems harmless worst-case scenario. Can always remove later if nobody uses it.
MarkLTZ referenced this in commit 5a3c3ea2c2 on Nov 17, 2019
fanquake added this to the "In progress" column in a project

brakmic commented at 8:10 AM on May 2, 2020: contributor

@promag

I have proposed a similar solution yesterday but didn't know that your PR exists (since Jan 2019 btw.). Anyway. :)

I'd recommend to implement an additional MsgBox about closing all wallets automatically. This way users would be able to start the closing procedure while still having an option to keep some of their wallets open.

This is how I implemented it in my demo code.

Regards,
jonasschnelli commented at 7:22 AM on May 29, 2020: contributor

Needs rebase.
gui: Add closeAllWallets to WalletController f30960adc0
gui: Add Close All Wallets action c4b574899a
promag commented at 12:07 AM on June 1, 2020: member

@jonasschnelli done.
promag force-pushed on Jun 1, 2020
DrahtBot removed the label Needs rebase on Jun 1, 2020
in src/qt/walletcontroller.cpp:103 in c4b574899a
```
  98 | +        tr("Are you sure you wish to close all wallets?"),
  99 | +        QMessageBox::Yes|QMessageBox::Cancel,
 100 | +        QMessageBox::Yes);
 101 | +    if (button != QMessageBox::Yes) return;
 102 | +
 103 | +    QMutexLocker locker(&m_mutex);
```
hebasto commented at 3:46 PM on June 4, 2020:

Why do you prefer QMutexLocker to our annotated variants?

promag commented at 11:33 PM on June 7, 2020:

No preference, m_mutex is a QMutex so we have to change that too.

hebasto commented at 6:12 AM on June 8, 2020:

It seems deserve its own pull :)
hebasto commented at 3:50 PM on June 4, 2020: member

Tested c4b574899abfa27f83c6948593838ed418c78f9c on Linux Mint 19.3 (x86_64) -- works as expected.
jonasschnelli commented at 7:17 AM on June 5, 2020: contributor

Tested ACK c4b574899abfa27f83c6948593838ed418c78f9c
jonasschnelli merged this on Jun 5, 2020
jonasschnelli closed this on Jun 5, 2020
promag deleted the branch on Jun 7, 2020
jnewbery moved this from the "In progress" to the "Done" column in a project

luke-jr referenced this in commit c4f078c444 on Aug 15, 2020
luke-jr referenced this in commit da26c96492 on Aug 15, 2020
Fabcien referenced this in commit 2762001a8f on Aug 25, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

gui: Add Close All Wallets action #15202

Conflicts