ClientModel code to handle core signals
#581
This PR:
QMetaObject::invokeMethod() “dynamic” calls, i.e., without compile-time checks of a called function name and its parametersstd::binds with lambdas, making parameter permutation (including parameter omitting) explicitAdditionally, debug messages have been unified.
No behavior change.
No behavior change.
No behavior change.
296@@ -306,7 +297,10 @@ static void BlockTipChanged(ClientModel* clientmodel, SynchronizationState sync_
297 void ClientModel::subscribeToCoreSignals()
298 {
299 // Connect signals to client
300- m_handler_show_progress = m_node.handleShowProgress(std::bind(ShowProgress, this, std::placeholders::_1, std::placeholders::_2));
301+ m_handler_show_progress = m_node.handleShowProgress(
302+ [this](const std::string& title, int progress, [[maybe_unused]] bool resume_possible) {
303+ Q_EMIT showProgress(QString::fromStdString(title), progress);
508e2dca5e91c1ff921f01d260fc62f629f1dc9e
This is fine because auto connection is used: https://github.com/bitcoin-core/gui/blob/747cdf1d652d8587e9f2e3d4436c3ecdbf56d0a5/src/qt/bitcoingui.cpp#L593
287@@ -301,7 +288,10 @@ void ClientModel::subscribeToCoreSignals()
288 [this](const std::string& title, int progress, [[maybe_unused]] bool resume_possible) {
289 Q_EMIT showProgress(QString::fromStdString(title), progress);
290 });
291- m_handler_notify_num_connections_changed = m_node.handleNotifyNumConnectionsChanged(std::bind(NotifyNumConnectionsChanged, this, std::placeholders::_1));
292+ m_handler_notify_num_connections_changed = m_node.handleNotifyNumConnectionsChanged(
293+ [this](int new_num_connections) {
294+ Q_EMIT numConnectionsChanged(new_num_connections);
271@@ -284,7 +272,11 @@ void ClientModel::subscribeToCoreSignals()
272 [this](bool network_active) {
273 Q_EMIT networkActiveChanged(network_active);
274 });
275- m_handler_notify_alert_changed = m_node.handleNotifyAlertChanged(std::bind(NotifyAlertChanged, this));
276+ m_handler_notify_alert_changed = m_node.handleNotifyAlertChanged(
277+ [this]() {
278+ qDebug() << "NotifyAlertChanged";
279+ Q_EMIT alertsChanged(getStatusBarWarnings());
9196d8a92837a077f65b3660d5cff523728e7db6
Wrong commit message, should be AlertChanged.
Also using auto connection: https://github.com/bitcoin-core/gui/blob/747cdf1d652d8587e9f2e3d4436c3ecdbf56d0a5/src/qt/overviewpage.cpp#L254 https://github.com/bitcoin-core/gui/blob/747cdf1d652d8587e9f2e3d4436c3ecdbf56d0a5/src/qt/rpcconsole.cpp#L663
264@@ -277,7 +265,11 @@ void ClientModel::subscribeToCoreSignals()
265 qDebug() << "NotifyAlertChanged";
266 Q_EMIT alertsChanged(getStatusBarWarnings());
267 });
268- m_handler_banned_list_changed = m_node.handleBannedListChanged(std::bind(BannedListChanged, this));
269+ m_handler_banned_list_changed = m_node.handleBannedListChanged(
270+ [this]() {
271+ qDebug() << QString("%1: Requesting update for peer banlist").arg(__func__);
272+ banTableModel->refresh();
dd924e3efa021dc2fe50da9fedba4a87ce3026aa
I think this is wrong because BanTableModel::refresh() needs to be called on the Qt main thread.
Maybe:
0 GUIUtil::ObjectInvoke(banTableModel, [=] {
1 banTableModel->refresh();
2 });
Concept ACK
To summarize, this PR is replacing the use of QMetaObject::invokeMethod(), std::bind, and std::placeholder with a more straightforward lambda function and direct Q_EMIT signals.
As per my understanding of the master branch’s implementation:
QMetaObject::invokeMethod(obj, member, conn_type, args) is used, which invoked the member signal or slot on the object, with the given args. Reference.In this PR’s implementation:
invokeMethod(), it directly creates it as a lambda function, where the argument values were earlier bounded (using std::bind).connect with Qt::ConnectionType as Qt::AutoConnection, which will ensure the signal is invoked correctly based on the receiver (if it is in the same thread or not). Reference.However, as @promag mentioned, it is not the case with BanTableModel::refresh().
0void BanTableModel::refresh()
1{
2 Q_EMIT layoutAboutToBeChanged();
3 priv->refreshBanlist(m_node);
4 Q_EMIT layoutChanged();
5}
This function should be invoked under the main thread, but the emit signal it calls (layoutAboutToBeChanged()) is not bound by and connect. Hence, it would be directly emitted irrespective of the current thread. So this might probably cause some issues.
I hope my notes and understanding of this PR will help my fellow reviewers. In case any of my analyses are erroneous, please do correct me.
Testing result:
I successfully compiled this PR in Ubuntu 20.04 with Qt version 5.9.7.
I experimented with banning a few peers and sorting them (as BanTableModel::sort() uses refresh()) based on different categories, and I observed no discrepancy in the behavior.
Hence I observed no change in the behavior of GUI.
Want to update? :)
Oops :sweat_smile:! Let me do so now. And retest this PR ASAP!
No behavior change.
No behavior change.
No behavior change.
Function names are self-described.
Labels
Refactoring