Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection

pablomartin4btc commented at 8:47 pm on April 9, 2024: contributor

Peek 2024-04-09 17-37

Peek 2024-04-09 17-45

Note for maintainers: this needs to be backported to 25.x and 26.x.

DrahtBot commented at 8:47 pm on April 9, 2024: 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
Stale ACK	alfonsoromanz

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

Conflicts

No conflicts as of last run.

alfonsoromanz approved

alfonsoromanz commented at 12:54 pm on April 18, 2024: contributor

Tested ACK d3da5025f61534d126fef12f198afb65f3a17897

luke-jr changes_requested

luke-jr commented at 10:47 pm on April 21, 2024: member

This feels like the wrong place to fix it. Why not inside setWalletActionsEnabled (or whatever is enabling it to begin with)?

pablomartin4btc commented at 2:46 pm on April 22, 2024: contributor

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!

pablomartin4btc force-pushed on May 22, 2024

pablomartin4btc commented at 10:43 am on May 22, 2024: contributor

Updates:

Addressed @luke-jr’s feedback.

DrahtBot added the label CI failed on May 22, 2024

in src/qt/bitcoingui.cpp:717 in 6c3b02717e outdated

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());

luke-jr commented at 11:32 pm on May 22, 2024:

Why move this up top? Now it’s after the signal is connected, so there’s a tiny chance of a race…

pablomartin4btc commented at 8:10 am on May 23, 2024:

Since I moved enableHistoryAction within setWalletActionsEnabled I thought there was a evaluation of the wallet_view privacy there… taking that back down.

pablomartin4btc commented at 8:45 am on May 23, 2024:

done.

in src/qt/bitcoingui.cpp:770 in 6c3b02717e outdated

767@@ -769,6 +768,7 @@ void BitcoinGUI::setCurrentWallet(WalletModel* wallet_model)
768             break;
769         }
770     }
771+    setWalletActionsEnabled(true);

luke-jr commented at 11:39 pm on May 22, 2024:

This shouldn’t be necessary.

pablomartin4btc commented at 8:04 am on May 23, 2024:

I’ll check, thanks.

pablomartin4btc commented at 8:29 am on May 23, 2024:

It doesn’t work without it…

pablomartin4btc commented at 11:09 am on May 24, 2024:

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!

luke-jr changes_requested

luke-jr commented at 11:46 pm on May 22, 2024: member

I’m pretty sure you can just modify the existing line in setWalletActionsEnabled to:

0    historyAction->setEnabled(enabled && !isPrivacyModeActivated());

pablomartin4btc commented at 8:34 am on May 23, 2024: contributor

0    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).~~

pablomartin4btc force-pushed on May 23, 2024

pablomartin4btc commented at 8:39 am on May 23, 2024: contributor

Updates:

Addressed @luke-jr’s feedback.

luke-jr commented at 6:22 pm on May 23, 2024: member

This one-line change works without anything else:

#815#pullrequestreview-2072577992

pablomartin4btc commented at 10:29 am on May 24, 2024: contributor

This one-line change works without anything else:

#815 (review)

Sorry, I made another mistake, thanks for double checking.

To other reviewers: please hold on till next push, thanks.

gui: Fix TransactionsView on setCurrentWallet

Making sure that if the privacy mode is activaded during
the wallet selection, the transaction view is not shown.

260d6eb927

pablomartin4btc force-pushed on May 24, 2024

pablomartin4btc commented at 11:14 am on May 24, 2024: contributor

Updates:

Incorporated @luke-jr’s suggestion, simplifying the fix.

hebasto commented at 2:43 pm on July 15, 2024: member

@pablomartin4btc Did you consider https://github.com/bitcoinknots/bitcoin/issues/83?

pablomartin4btc commented at 2:59 pm on July 15, 2024: contributor

@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.

hebasto commented at 3:11 pm on July 15, 2024: member

@pablomartin4btc

… was introduced in #718…

Sure about PR number? (‘cause there is no such a number in this repo)

pablomartin4btc commented at 6:06 pm on July 15, 2024: contributor

@pablomartin4btc

… 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.

pablomartin4btc commented at 3:19 pm on July 17, 2024: contributor

@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.

pablomartin4btc commented at 4:32 pm on July 30, 2024: contributor

@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).

Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection #815

Reviews

Conflicts