Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Make paths to update Encryption and HD wallet statuses simpler #403

Merged
merged 4 commits into from
Aug 26, 2021
Merged

refactor: Make paths to update Encryption and HD wallet statuses simpler #403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7d0d4c0
qt: Add WalletFrame::currentWalletSet signal
hebasto Aug 11, 2021
37dcf16
qt, refactor: Emit WalletView::encryptionStatusChanged signal directly
hebasto Aug 11, 2021
fcdc8b0
qt, refactor: Drop redundant signalling in WalletView::setWalletModel
hebasto Aug 11, 2021
b8aa84b
qt, refactor: Replace `if` check with `assert`
hebasto Aug 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions src/qt/bitcoingui.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(); });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to wrap that call with lambda? I see that updateWalletStatus is already a slot, or am I missing something?

Copy link
Member Author

@hebasto hebasto Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@hebasto hebasto Aug 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(); } vs this, &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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. No need for an apology. Feel free to mark as resolved and merge etc.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

setCentralWidget(walletFrame);
} else
#endif // ENABLE_WALLET
Expand Down Expand Up @@ -694,7 +695,6 @@ void BitcoinGUI::addWallet(WalletModel* walletModel)
});
connect(wallet_view, &WalletView::encryptionStatusChanged, this, &BitcoinGUI::updateWalletStatus);
connect(wallet_view, &WalletView::incomingTransaction, this, &BitcoinGUI::incomingTransaction);
connect(wallet_view, &WalletView::hdEnabledStatusChanged, this, &BitcoinGUI::updateWalletStatus);
connect(this, &BitcoinGUI::setPrivacy, wallet_view, &WalletView::setPrivacy);
wallet_view->setPrivacy(isPrivacyModeActivated());
const QString display_name = walletModel->getDisplayName();
Expand Down Expand Up @@ -1340,9 +1340,8 @@ void BitcoinGUI::setEncryptionStatus(int status)

void BitcoinGUI::updateWalletStatus()
{
if (!walletFrame) {
return;
}
assert(walletFrame);

WalletView * const walletView = walletFrame->currentWalletView();
if (!walletView) {
return;
Expand Down
3 changes: 2 additions & 1 deletion src/qt/walletframe.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ void WalletFrame::setCurrentWallet(WalletModel* wallet_model)
walletView->updateGeometry();

walletStack->setCurrentWidget(walletView);
walletView->updateEncryptionStatus();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Q_EMIT currentWalletSet();
}

void WalletFrame::removeWallet(WalletModel* wallet_model)
Expand Down
1 change: 1 addition & 0 deletions src/qt/walletframe.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class WalletFrame : public QFrame
Q_SIGNALS:
void createWalletButtonClicked();
void message(const QString& title, const QString& message, unsigned int style);
void currentWalletSet();

private:
QStackedWidget *walletStack;
Expand Down
11 changes: 1 addition & 10 deletions src/qt/walletview.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,6 @@ void WalletView::setWalletModel(WalletModel *_walletModel)

// Handle changes in encryption status
connect(_walletModel, &WalletModel::encryptionStatusChanged, this, &WalletView::encryptionStatusChanged);
updateEncryptionStatus();

// update HD status
Q_EMIT hdEnabledStatusChanged();

// Balloon pop-up for new transaction
connect(_walletModel->getTransactionTableModel(), &TransactionTableModel::rowsInserted, this, &WalletView::processNewTransaction);
Expand Down Expand Up @@ -211,11 +207,6 @@ void WalletView::showOutOfSyncWarning(bool fShow)
overviewPage->showOutOfSyncWarning(fShow);
}

void WalletView::updateEncryptionStatus()
{
Q_EMIT encryptionStatusChanged();
}

void WalletView::encryptWallet()
{
if(!walletModel)
Expand All @@ -224,7 +215,7 @@ void WalletView::encryptWallet()
dlg.setModel(walletModel);
dlg.exec();

updateEncryptionStatus();
Q_EMIT encryptionStatusChanged();
}

void WalletView::backupWallet()
Expand Down
5 changes: 0 additions & 5 deletions src/qt/walletview.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,9 +103,6 @@ public Q_SLOTS:
/** Show used receiving addresses */
void usedReceivingAddresses();

/** Re-emit encryption status signal */
void updateEncryptionStatus();

/** Show progress dialog e.g. for rescan */
void showProgress(const QString &title, int nProgress);

Expand All @@ -117,8 +114,6 @@ public Q_SLOTS:
void message(const QString &title, const QString &message, unsigned int style);
/** Encryption status of wallet changed */
void encryptionStatusChanged();
/** HD-Enabled status of wallet changed (only possible during startup) */
void hdEnabledStatusChanged();
/** Notify that a new transaction appeared */
void incomingTransaction(const QString& date, int unit, const CAmount& amount, const QString& type, const QString& address, const QString& label, const QString& walletName);
/** Notify that the out of sync warning icon has been pressed */
Expand Down