<details> <summary>Currenlty on <code>master</code>, when the "mask values" checkbox is ticked if the user selects a different wallet, the history action is enable and if the user clicks on it can see all the transactions in the transaction view.</summary>
</details> <details> <summary>This PR fixes it.</summary>
</details>
Note for maintainers: this needs to be backported to 25.x and 26.x.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | hebasto |
| Stale ACK | alfonsoromanz |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Tested ACK d3da5025f61534d126fef12f198afb65f3a17897
This feels like the wrong place to fix it. Why not inside setWalletActionsEnabled (or whatever is enabling it to begin with)?
This feels like the wrong place to fix it. Why not inside
setWalletActionsEnabled(or whatever is enabling it to begin with)?
Yeah, it makes more sense, I'll rework it. Thanks!
713 | @@ -714,6 +714,8 @@ void BitcoinGUI::addWallet(WalletModel* walletModel) 714 | WalletView* wallet_view = new WalletView(walletModel, platformStyle, walletFrame); 715 | if (!walletFrame->addView(wallet_view)) return; 716 | 717 | + wallet_view->setPrivacy(isPrivacyModeActivated());
Why move this up top? Now it's after the signal is connected, so there's a tiny chance of a race...
Since I moved enableHistoryAction within setWalletActionsEnabled I thought there was a evaluation of the wallet_view privacy there... taking that back down.
done.
767 | @@ -769,6 +768,7 @@ void BitcoinGUI::setCurrentWallet(WalletModel* wallet_model) 768 | break; 769 | } 770 | } 771 | + setWalletActionsEnabled(true);
This shouldn't be necessary.
I'll check, thanks.
It doesn't work without it...
Sorry, that was because setWalletActionsEnabled was not working properly, and setWalletActionsEnabled was not working because I misread your suggestion and I was still calling the function enableHistoryAction incorrectly instead of just using directly historyAction->setEnabled. It fixed now in 260d6eb. Thanks!
I'm pretty sure you can just modify the existing line in setWalletActionsEnabled to:
historyAction->setEnabled(enabled && !isPrivacyModeActivated());
historyAction->setEnabled(enabled && !isPrivacyModeActivated());
Just replacing the operator with || (e.g. when the user closes all wallets, all tabs will be disabled except for Transactions).
This one-line change works without anything else:
Sorry, I made another mistake, thanks for double checking.
To other reviewers: please hold on till next push, thanks.
Updates:
@pablomartin4btc Did you consider https://github.com/bitcoinknots/bitcoin/issues/83?
@pablomartin4btc Did you consider bitcoinknots/bitcoin#83?
Yeah, in fact the "switch to Overview" tab when changing/ selecting wallets, was introduced in #718, I can fix it here in a 2nd. commit which will do soon.
... was introduced in #718...
Sure about PR number? ('cause there is no such a number in this repo)
oh! my bad.. a typo there... how lucky! I'll play the lottery with that one today... I meant #708, I think it's the only place related to the "mask value" where I switched to the overview tab when the mask value checkbox on the menu is ticked but still need to check it.
@hebasto, I couldn't reproduce the issue described in https://github.com/bitcoinknots/bitcoin/issues/83, trying both master and this PR, also checked the fix in knots which is similar to this PR code change. It would be good to know what was the state of the "mask value" during the use case described in https://github.com/bitcoinknots/bitcoin/issues/83, the only thing I can think of it's that the issue is the same one described above on this PR but when that happens loading a second wallet would disable the Transactions view tab so it can't be selected. Perhaps @luke-jr can describe the steps to reproduce it.
@hebasto, the issue mentioned in https://github.com/bitcoinknots/bitcoin/issues/83 is fixed by this PR.
I've done a bit of investigation and the problem (in master, not on this PR) was not "switching" between wallets (selecting another wallet from the combo box won't show the overview page while the user is landing on the transactions tab), but when the user opens another wallet (which hasn't been loaded before) and the current wallet view is on the transactions/ history tab, then the system sets the overview page as a "default view" lets say (the problematic call has been removed from this PR and it was introduced by #708 originally).
<!--2e250dc3d92b2c9115b66051148d6e47-->
🤔 There hasn't been much activity lately and the CI seems to be failing.
If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.
Making sure that if the privacy mode is activaded during
the wallet selection, the transaction view is not shown.
Updates:
<!--8ac04cdde196e94527acabf64b896448-->
There hasn't been much activity lately. What is the status here?
Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
ACK 0dc337f73d013e342b880746292f1c3247b287cf, tested on Fedora 43.
I agree with this comment. Following this direction, further improvements to the m_mask_values_action connections could be made by switching to a lambda.
Backported to 31.x in https://github.com/bitcoin/bitcoin/pull/34800.
