Fix issues with new create wallet dialogue#934
Conversation
In the "Create Wallet" UI, there is an option and tooltip for "Encrypt Wallet", which is misleading because only the private key is encrypted. This updates the tooltip text to indicate that only the private key is encrypted. refs bitcoin-core#151
In the "Create Wallet" UI, checking the encrypt wallet option disables the "Disable Private Keys" option. However, checking the encryption option and "Make Blank Wallet" re-enables "Disable Private Keys" incorrectly. This fixes that so it does not re-enable the option. refs bitcoin-core#151
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
| // Disable the disable_privkeys_checkbox when blank_wallet_checkbox is checked | ||
| // as blank-ness only pertains to wallets with private keys. | ||
| ui->disable_privkeys_checkbox->setEnabled(!checked); | ||
| ui->disable_privkeys_checkbox->setEnabled(!checked && !ui->encrypt_wallet_checkbox->isChecked()); |
There was a problem hiding this comment.
Seems like the opposite fix is also needed?
I feel like this whole logic needs to be rewritten...
|
Heey i was actually going through issue #151. Concept ACK on both fixes. The wording approach is sensible, keeping the familiar "Encrypt Wallet" label while clarifying in the tooltip that it encrypts the private keys specifically (not all wallet metadata) is a good balance between accuracy and not disrupting the 27 other instances across the app. On the checkbox fix: the blank_wallet_checkbox handler correctly gets !checked && !ui->encrypt_wallet_checkbox->isChecked(), but as luke-jr noted, the symmetric case is missing. The encrypt_wallet_checkbox toggled handler at line 31 has the same pattern: ui->disable_privkeys_checkbox->setEnabled(!checked); This causes an identical bug in the other direction, if blank_wallet_checkbox is checked, then encrypt_wallet_checkbox is toggled on and back off, disable_privkeys_checkbox incorrectly becomes re-enabled. The fix is symmetric: ui->disable_privkeys_checkbox->setEnabled(!checked && !ui->blank_wallet_checkbox->isChecked()); Adding that one line alongside your existing fix would fully cover vasild's noted inconsistency in both directions. |
With this PR, I aim to fix #151.
As @Rspigler noted in the issue:
Point number one is still an issue. However, as @vasild mentioned here, point number 2 is no longer happening as of 17072f7. They point out another inconsistency where:
This PR fixes @Rspigler's point number one by changing the "Encrypt Wallet" tooltip to clarify that it encrypts the private key rather than the wallet. It also fixes @vasild's noted inconsistency where the checkbox becomes re-enabled.
Rationale
I considered for a moment that it might be more technically accurately to say "encrypt private key" instead of "encrypt wallet". However, I counted 27 instances of user-facing strings that describe encrypting the wallet. Users might be accustomed to the feature being called "encrypt wallet", so changing the text all across the app felt drastic, IMHO. Therefore, I thought we could simply provide some clarification in the tooltip that encrypting the wallet specifically means encrypting the private keys, but not other wallet metadata.
Testing
I successfully built the application with each commit of this PR, and I ran
ctest --test-dir buildto verify all the tests passed.First PR
This is my first time opening a PR to Bitcoin Core. I've done my research on best practices, but I probably made errors. Happy to correct anything I may have missed.