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: Ensure 'Transaction View' remains disabled if no wallet is selected #780

Conversation

pablomartin4btc
Copy link
Contributor

@pablomartin4btc pablomartin4btc commented Dec 7, 2023

This PR addresses an issue where, with no wallet selected, ticking on "Settings -> Mask values" checkbox twice enables the transaction tab when the checkbox is unticked.

Current behavior display on master

Peek 2023-12-06 19-18

Correction display from this branch

Peek 2023-12-07 13-07

Note for maintaners: this PR should be backported to both 25.x and 26.x.


Originally this PR was disabling the "Mask Values" checkbox when no wallet was selected but since a reviewer pointed out that a user might want to open a wallet already on "privacy mode" I rolled that change out.

Original correction display disabling "Mask Values"

Peek 2023-12-06 19-11

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 7, 2023

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK alfonsoromanz, hebasto
Concept ACK luke-jr, BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

nice find

I'm not sure if this is the right approach: user might want to enable Mask values before opening his wallet?

@pablomartin4btc
Copy link
Contributor Author

I'm not sure if this is the right approach: user might want to enable Mask values before opening his wallet?

Interesting point of view, I agree with it... Not sure if other wallet apps do it but ok, I'll remove that "fix" and perhaps other reviewers give their opinions on it. Thanks for the review!

The Transaction View should be only enabled when a wallet is selected.
Therefore it has been added a condition for a selected wallet on
enableHistoryAction() since its availability also depends on the mask
value checkbox.
@pablomartin4btc pablomartin4btc force-pushed the gui-disable-mask-values-and-tx-view-if-no-wallet-selected branch from da7dcdf to b2e531e Compare December 7, 2023 15:52
@pablomartin4btc pablomartin4btc changed the title Fix: Disable both "Mask Values" and "Transaction View" if no wallet is selected Fix: Ensure 'Transaction View' remains disabled if no wallet is selected Dec 7, 2023
@pablomartin4btc
Copy link
Contributor Author

pablomartin4btc commented Dec 7, 2023

I'm not sure if this is the right approach: user might want to enable Mask values before opening his wallet?

I was just giving it a second thought and if we follow the original approach a user could still have the desired behaviour as the following:

check display here

Peek 2023-12-07 13-42

First time the user opens or creates a wallet can tick the checkbox on "Mask Values", this is also persisted, so next time the wallet is open or selected will be on privacy mode. The fix was not changing the current value of the "Mask Values", so if a user updates QT to the version containing the fix it won't impact on their setup.

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

utACK

@MarnixCroes
Copy link
Contributor

I'm not sure if this is the right approach: user might want to enable Mask values before opening his wallet?

I was just giving it a second thought and if we follow the original approach a user could still have the desired behaviour as the following:
check display here

First time the user opens or creates a wallet can tick the checkbox on "Mask Values", this is also persisted, so next time the wallet is open or selected will be on privacy mode. The fix was not changing the current value of the "Mask Values", so if a user updates QT to the version containing the fix it won't impact on their setup.

@pablomartin4btc yes correct it remembers if Mask Values is enabled or not.

Being only able to change it requires a wallet to be open, to me would feel like a bug, no?
that's more my "concern"

@pablomartin4btc
Copy link
Contributor Author

Being only able to change it requires a wallet to be open, to me would feel like a bug, no? that's more my "concern"

Fair enough, thanks for sharing your insights.

Copy link
Contributor

@alfonsoromanz alfonsoromanz 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 b2e531e

Screen Recording 2023-12-22 at 18 24 11

@DrahtBot DrahtBot requested a review from luke-jr December 22, 2023 21:27
@pablomartin4btc
Copy link
Contributor Author

Thanks @MarnixCroes, @luke-jr and @alfonsoromanz for reviewing!

Copy link
Contributor

@BrandonOdiwuor BrandonOdiwuor left a comment

Choose a reason for hiding this comment

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

Concept ACK

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 b2e531e, tested on Ubuntu 22.04.

@hebasto hebasto merged commit 6868474 into bitcoin-core:master Feb 11, 2024
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants