refactor: Revamp ClientModel code to handle core signals
#581
Conversation
No behavior change.
No behavior change.
No behavior change.
|
Friendly ping @promag @ryanofsky |
| m_handler_show_progress = m_node.handleShowProgress(std::bind(ShowProgress, this, std::placeholders::_1, std::placeholders::_2)); | ||
| m_handler_show_progress = m_node.handleShowProgress( | ||
| [this](const std::string& title, int progress, [[maybe_unused]] bool resume_possible) { | ||
| Q_EMIT showProgress(QString::fromStdString(title), progress); |
There was a problem hiding this comment.
| m_handler_notify_num_connections_changed = m_node.handleNotifyNumConnectionsChanged(std::bind(NotifyNumConnectionsChanged, this, std::placeholders::_1)); | ||
| m_handler_notify_num_connections_changed = m_node.handleNotifyNumConnectionsChanged( | ||
| [this](int new_num_connections) { | ||
| Q_EMIT numConnectionsChanged(new_num_connections); |
src/qt/clientmodel.cpp
Outdated
| m_handler_banned_list_changed = m_node.handleBannedListChanged( | ||
| [this]() { | ||
| qDebug() << QString("%1: Requesting update for peer banlist").arg(__func__); | ||
| banTableModel->refresh(); |
There was a problem hiding this comment.
I think this is wrong because BanTableModel::refresh() needs to be called on the Qt main thread.
Maybe:
GUIUtil::ObjectInvoke(banTableModel, [=] {
banTableModel->refresh();
});
shaavan
left a comment
shaavan
left a comment
There was a problem hiding this comment.
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:
- A function is used in which
QMetaObject::invokeMethod(obj,member, conn_type, args) is used, which invoked the member signal or slot on the object, with the given args. Reference. - This function is then bounded (using std::bind) to “this” and placeholders if needed, which would be replaced by the required argument for the function. Reference.
In this PR’s implementation:
- Instead of creating a function that would
invokeMethod(), it directly creates it as a lambda function, where the argument values were earlier bounded (using std::bind). - It uses Q_EMIT to emit the respective signal immediately
- All the Q_EMIT signals are bounded under a
connectwith Qt::ConnectionType asQt::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().
void BanTableModel::refresh()
{
Q_EMIT layoutAboutToBeChanged();
priv->refreshBanlist(m_node);
Q_EMIT layoutChanged();
}
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.
|
Thank you for your review.
Want to update? :) |
Oops 😅! Let me do so now. And retest this PR ASAP! |
No behavior change.
No behavior change.
No behavior change.
Function names are self-described.
4 participants
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.