refactor: Make paths to update Encryption and HD wallet statuses simpler #403
Conversation
| @@ -103,7 +103,8 @@ void WalletFrame::setCurrentWallet(WalletModel* wallet_model) | |||
| walletView->updateGeometry(); | |||
|
|
|||
| walletStack->setCurrentWidget(walletView); | |||
| walletView->updateEncryptionStatus(); | |||
There was a problem hiding this comment.
In commit "qt: Add WalletFrame::currentWalletSet signal" (c0f84d9)
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.
| @@ -113,6 +113,7 @@ BitcoinGUI::BitcoinGUI(interfaces::Node& node, const PlatformStyle *_platformSty | |||
| connect(walletFrame, &WalletFrame::message, [this](const QString& title, const QString& message, unsigned int style) { | |||
| this->message(title, message, style); | |||
| }); | |||
| connect(walletFrame, &WalletFrame::currentWalletSet, [this] { updateWalletStatus(); }); | |||
There was a problem hiding this comment.
Do we really need to wrap that call with lambda? I see that updateWalletStatus is already a slot, or am I missing something?
There was a problem hiding this comment.
UPDATED: #403 (comment)
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?
There was a problem hiding this comment.
The drawback is writing more verbose / unnecessary code, for no reason other than to appease your editor. If it's been questioned during review, it's just as likely someone will open a PR to remove it later on, citing the same reasoning, that, it's confusing / unnecessary. We really shouldn't be bloating our codebase just because your editor doesn't work.
There was a problem hiding this comment.
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:
- the former states the used signal-slot parameters explicitly, i.e., here it is clear that we use no signal parameters
- the former is less verbose, compare
[this] { updateWalletStatus(); }vsthis, &BitcoinGUI::updateWalletStatus
We 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. bitcoin/bitcoin#14123 and other prs by @promag.
There was a problem hiding this comment.
Thanks for providing the actual reasoning; this should have been the first comment. I don't really have an opinion on the code, but was always going to leave the response above, regardless of the change, because the justification "it's better for my editor", is not a good one.
There was a problem hiding this comment.
No worries. No need for an apology. Feel free to mark as resolved and merge etc.
There was a problem hiding this comment.
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 b8aa84b 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. Notes on commits
|
|
Code review ACK b8aa84b. Did build on Debian Sid with Qt 5.15.2 but no actual testing performed. |
This PR makes signal-slot paths to reach
setHDStatusandsetEncryptionStatusfunctions shorter and easier to reason about them.Required to simplify #398 (see #398 (comment)).
Note for reviewers. Please verify that "Encrypt Wallet..." menu item, and the following icons
and updated properly in each and every possible scenario.