Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix issue when disabling the auto-enabled blank wallet checkbox #243

Merged
merged 1 commit into from
Mar 26, 2021

Conversation

jarolrod
Copy link
Member

As detailed by #151, On master a user can create the confusing scenario where you have a disabled Encrypt Wallet checkbox and a selected Disable Private Keys checkbox after unselecting the auto-enabled Blank Wallet checkbox.

This commit makes it so that when the Blank Wallet checkbox is auto-selected after the user selects Disable Private keys, unselecting it will also unselect the Disable Private Keys checkbox, which in turn re-enables the Encrypt Wallet checkbox.

Below are screenshots comparing the behavior of selecting Disable Private Keys then unselecting the Blank Wallet between master and this PR:

Master:

Select Disable Private Keys Unselect Blank Wallet
Screen Shot 2021-03-09 at 7 57 14 PM Screen Shot 2021-03-09 at 7 57 31 PM

PR:

Select Disable Private Keys Unselect Blank Wallet
Screen Shot 2021-03-09 at 7 34 12 PM Screen Shot 2021-03-09 at 7 34 20 PM

@hebasto hebasto added the Design label Mar 10, 2021
@@ -53,6 +53,13 @@ CreateWalletDialog::CreateWalletDialog(QWidget* parent) :
}
});

connect(ui->blank_wallet_checkbox, &QCheckBox::toggled, [this](bool checked) {
// Disable the disable_privkeys_checkbox when blank_wallet_checkbox is false
Copy link

@Talkless Talkless Mar 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if these kind of comments are helpful. You can see that stated fact by just looking at rather self-explanatory code, with descriptive variable names.

Maybe this message would be more informative:

//Having "Disable Private Keys" checked after unchecking "Make Blank Wallet" makes no sense. Disabling keys means wallet has to be blank.

Or something similar.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the comment is wrong as the disable_privkeys_checkbox is to be unchecked, not disabled :)

I think just removing it is fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the comment in 915e341

Copy link

@leonardojobim leonardojobim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK ca6bb0d on Ubuntu 20.04

@maflcko maflcko requested a review from Sjors March 12, 2021 07:39
@Rspigler
Copy link
Contributor

Tested ACK commit ca6bb0d

Looks good!

Doesn't fix all the issues I described in #151, since the encryption tooltip is still wrong. But this annoying inconsistency is fixed

@hebasto
Copy link
Member

hebasto commented Mar 20, 2021

Concept ACK. The logic of checkboxes interaction is correct.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach ACK ca6bb0d, please remove comment.

This commit makes it so that when the `Blank Wallet` checkbox is auto-selected after the user selects 'Disable Private' keys, unselecting it will also unselect the 'Disable Private Keys' checkbox, which in turn re-enables the 'Encrypt Wallet' checkbox.
@jarolrod
Copy link
Member Author

Updated from ca6bb0d -> 915e341, addressed @hebasto's comment

Changes: remove the redundant comment. Code is self-explanatory.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 915e341

Copy link

@Talkless Talkless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 915e341

@maflcko maflcko changed the title qt: fix issue when disabling the auto-enabled blank wallet checkbox fix issue when disabling the auto-enabled blank wallet checkbox Mar 26, 2021
@maflcko maflcko merged commit 19e3e65 into bitcoin-core:master Mar 26, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 26, 2021
… blank wallet checkbox

915e341 qt: fix issue when disabling the auto-enabled blank wallet checkbox (Jarol Rodriguez)

Pull request description:

  As detailed by #151, On `master` a user can create the confusing scenario where you have a disabled `Encrypt Wallet` checkbox and a selected `Disable Private Keys` checkbox after unselecting the auto-enabled `Blank Wallet` checkbox.

  This commit makes it so that when the `Blank Wallet` checkbox is auto-selected after the user selects `Disable Private keys`, unselecting it will also unselect the `Disable Private Keys` checkbox, which in turn re-enables the `Encrypt Wallet` checkbox.

  Below are screenshots comparing the behavior of selecting `Disable Private Keys` then unselecting the `Blank Wallet` between `master` and this `PR`:

  **Master:**
  | Select `Disable Private Keys` | Unselect `Blank Wallet` |
  | ----------------------------- | ------------------------ |
  | ![Screen Shot 2021-03-09 at 7 57 14 PM](https://user-images.githubusercontent.com/23396902/110560141-77405a80-8113-11eb-9285-5acba6241dcf.png) |   ![Screen Shot 2021-03-09 at 7 57 31 PM](https://user-images.githubusercontent.com/23396902/110560159-81faef80-8113-11eb-9b37-086aa39ecb9f.png)    |

  **PR:**
  | Select `Disable Private Keys` | Unselect `Blank Wallet` |
  | ----------------------------- | ------------------------ |
  | ![Screen Shot 2021-03-09 at 7 34 12 PM](https://user-images.githubusercontent.com/23396902/110560379-e3bb5980-8113-11eb-899a-3a4c6a1bc115.png) | ![Screen Shot 2021-03-09 at 7 34 20 PM](https://user-images.githubusercontent.com/23396902/110560412-f170df00-8113-11eb-8bd0-f7fe6fc0d739.png) |

ACKs for top commit:
  hebasto:
    ACK 915e341
  Talkless:
    ACK 915e341

Tree-SHA512: ce6ecbc35b94a08cabf0b8a24dbdfc874d82cc8918cc8623dce8172c7fc9c75d63a13b036bae5f7ab2c090f8d020574a542285d1651600813faf5d91e2506a8d
@jarolrod jarolrod deleted the create-wallet branch March 26, 2021 19:36
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants