Fix: Ensure 'Transaction View' remains disabled if no wallet is selected #780

pablomartin4btc commented at 5:10 AM on December 7, 2023: contributor

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.

<details> <summary>Current behavior display on master</summary>

Peek 2023-12-06 19-18

</details>

<details> <summary>Correction display from this branch</summary>

Peek 2023-12-07 13-07

</details>

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.

<details> <summary>Original correction display disabling "Mask Values" </summary>

Peek 2023-12-06 19-11

</details>

DrahtBot commented at 5:10 AM on December 7, 2023: contributor

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.

MarnixCroes commented at 9:22 AM on December 7, 2023: contributor

nice find

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

pablomartin4btc commented at 3:46 PM on December 7, 2023: contributor

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!

qt: update widgets availability on wallet selection

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.

b2e531e70a

pablomartin4btc force-pushed on Dec 7, 2023

pablomartin4btc renamed this:
~~Fix: Disable both "Mask Values" and "Transaction View" if no wallet is selected~~
Fix: Ensure 'Transaction View' remains disabled if no wallet is selected
on Dec 7, 2023

pablomartin4btc commented at 4:54 PM on December 7, 2023: 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:

<details> <summary>check display here</summary>

Peek 2023-12-07 13-42

</details>

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.

luke-jr approved

luke-jr commented at 6:48 PM on December 17, 2023: member

utACK

MarnixCroes commented at 10:19 PM on December 18, 2023: 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"