This PR makes signal-slot paths to reach setHDStatus and setEncryptionStatus functions shorter and easier to reason about them.
Required to simplify #398 (see #398 (review)).
Note for reviewers. Please verify that “Encrypt Wallet…” menu item, and the following icons
and updated properly in each and every possible scenario.
102@@ -103,7 +103,8 @@ void WalletFrame::setCurrentWallet(WalletModel* wallet_model)
103 walletView->updateGeometry();
104
105 walletStack->setCurrentWidget(walletView);
106- walletView->updateEncryptionStatus();
In commit “qt: Add WalletFrame::currentWalletSet signal” (c0f84d9d4354e128b16524c14b6c2dadb891a1b6)
Note: At was unclear to me at first if this commit was changing behavior. But it does not seem to change anything since the previous updateEncryptionStatus just called updateWalletStatus indirectly.
The connection of the WalletFrame::currentWalletSet signal to
the BitcoinGUI::updateWalletStatus is a shorter and more descriptive way
to call both the setHDStatus and setEncryptionStatus member functions of
the BitcoinGUI class in comparison to using of the
WalletView::updateEncryptionStatus slot.
This job will be done by WalletFrame::currentWalletSet signal being
emitted in the WalletFrame::setCurrentWallet function.
There are no ways BitcoinGUI::updateWalletStatus being called without
an instance of the WalletFrame class.
112@@ -113,6 +113,7 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty
113 connect(walletFrame, &WalletFrame::message, [this](const QString& title, const QString& message, unsigned int style) {
114 this->message(title, message, style);
115 });
116+ connect(walletFrame, &WalletFrame::currentWalletSet, [this] { updateWalletStatus(); });
updateWalletStatus is already a slot, or am I missing something?
UPDATED: #403 (review) ~No, we don’t.~
~The small benefit is that my editor could easy find this call site of updateWalletStatus() function member. It fails to find it by pointer &BitcoinGUI::updateWalletStatus.~
Are there any drawbacks of such an approach?
I strictly prefer QObject::connect(const QObject *sender, PointerToMemberFunction signal, Functor functor) overload over QObject::connect(const QObject *sender, PointerToMemberFunction signal, const QObject *receiver, PointerToMemberFunction method, Qt::ConnectionType type) one for the following reasons:
[this] { updateWalletStatus(); } vs this, &BitcoinGUI::updateWalletStatusWe really shouldn’t be bloating our codebase just because your editor doesn’t work.
Absolutely agree. But this is not that case, no?
Btw, this–very helpful–syntax is already actively used in the code base. E.g. https://github.com/bitcoin/bitcoin/pull/14123 and other prs by @promag.
Are there any drawbacks of such an approach?
It simply looks like unnecessary indirection, and at least raises question “why?” for some random future code reader.
Also, “verbosiness” of [this] { updateWalletStatus(); } vs this, &BitcoinGUI::updateWalletStatus is, IMHO, debatable. Former has more of []{();} vs $:: “eye-piercing” stuff . But that’s probably just personal preference :) .
I like argument that we explicitly see (absence of) arguments, though I would expect lambda usage for actually more complex invocation where you do have to fiddle with arguments, etc.
Anyway, not gonna resist too much, would still give ACK.
tACK b8aa84b1a116599a6dd3b9ddb4e6c178a6688b1b
tested on Ubuntu 20.04, and macOS 12. I compared the behavior of the icons as I expect them on master with this PR. I cannot guarantee that I tested every variation possible, but my test wallets do cover a wide range of scenarios.