This PR makes PeerTableModel handle a peer addition/removal in a right way. See:
Fixes #160.
Fixes #191.
tACK f73d340abeedf7e583fe9b2a8b3685a745a9199e
Confirming that the PR fixes the issue laid out in #160 on macOS 11.1 with Qt 5.15.2.
On master: If I select a peer, and a peer above it is disconnected, then the peer I had selected becomes unselected. Below are screenshots exhibiting this behavior.
Master: I have selected Peer ID 32
Master: Peer ID 32 is no longer selected because Peer ID 29 is above it and disconnected
on this PR: This behavior no longer occurs. If I select a peer, it remains selected even if a peer above it is disconnected.
PR: Peer ID 14 is selected
PR: Peer ID 0 has disconnected. Peer ID 14 remains selected.
54@@ -54,6 +55,7 @@ class ClientModel : public QObject
55 interfaces::Node& node() const { return m_node; }
56 OptionsModel *getOptionsModel();
57 PeerTableModel *getPeerTableModel();
58+ PeerTableSortProxy* peerTableSortProxy();
noexcept, adding that one kinda seems out of scope.
Though since other similar getters here does not use noexcept, adding that one kinda seems out of scope.
Going to leave it as is.
155+ interfaces::Node::NodesStats nodes_stats;
156+ m_node.getNodesStats(nodes_stats);
157+ decltype(m_peers_data) new_peers_data;
158+ new_peers_data.reserve(nodes_stats.size());
159+ for (const auto& node_stats : nodes_stats) {
160+ CNodeCombinedStats stats;
These stats has quite a few members, and they all gonna be overwritten after construction. Since it’s loop, consider initializing in this wat:
0const CNodeCombinedStats stats{std::get<0>(node_stats), std::get<2>(node_stats), std::get<1>(node_stats)};
It’s most dissapointing that QList does not have emplace() here…
82+Q_SIGNALS:
83+ void changed();
84+
85 private:
86+ //! Internal peer data structure.
87+ QList<CNodeCombinedStats> m_peers_data{};
{} seems redundant, it’s not fundamental type like int, pointer or bool to need this kind of explicit default initialization, constructor will be invoked regardless.
61@@ -74,22 +62,25 @@ class PeerTableModel : public QAbstractTableModel
62
63 /** @name Methods overridden from QAbstractTableModel
64 @{*/
65- int rowCount(const QModelIndex &parent) const override;
66- int columnCount(const QModelIndex &parent) const override;
67- QVariant data(const QModelIndex &index, int role) const override;
68- QVariant headerData(int section, Qt::Orientation orientation, int role) const override;
69- QModelIndex index(int row, int column, const QModelIndex &parent) const override;
70+ int rowCount(const QModelIndex& parent = QModelIndex()) const override;
= {} instead of = QModelIndex().
10+#include <QModelIndex>
11+#include <QString>
12+#include <QVariant>
13+
14+PeerTableSortProxy::PeerTableSortProxy(QObject* parent)
15+ : QSortFilterProxyModel(parent)
{} in m_peer_table_sort_proxy{nullptr};, maybe worth using {} in QSortFilterProxyModel{parent} too (meaning in the new code)?
8@@ -9,13 +9,14 @@
9 #include <qt/rpcconsole.h>
10 #include <qt/forms/ui_debugwindow.h>
11
12+#include <chainparams.h>
13+#include <interfaces/node.h>
#include <interfaces/node.h> is needed?
#include <qt/peertablesortproxy.h>, and other includes are sorted only. I’d leave the need of the interfaces/node.h header out of this pr scope.
154-}
155+ interfaces::Node::NodesStats nodes_stats;
156+ m_node.getNodesStats(nodes_stats);
157+ decltype(m_peers_data) new_peers_data;
158+ new_peers_data.reserve(nodes_stats.size());
159+ for (const auto& node_stats : nodes_stats) {
Ugh, my suggestion was not good enought, you can actually move values out of that local nodes_stats:
0 for (auto&& node_stats : nodes_stats) {
1 const CNodeCombinedStats stats{std::get<0>(std::move(node_stats)),
2 std::get<2>(std::move(node_stats)),
3 std::get<1>(std::move(node_stats))};
(note loop variable is no longer cost reference!)
Or even with an algorithm (also moves, at least it does on local test), if one would care about “No raw loops” mantra :)
0 std::transform(nodes_stats.begin(), nodes_stats.end(), std::back_inserter(new_peers_data), [](auto&& i) {
1 return CNodeCombinedStats{std::get<0>(std::move(i)), std::get<2>(std::move(i)), std::get<1>(std::move(i))};
2 });
Though moving std::transform seems fishy, not sure if it’s “legal” to move source… But gcc 10.2.1 does that.
Ugh, my suggestion was not good enought…
It seems like a kind of premature optimization, no?
std::transform variant do look particularly “noisy” (i.e. “optimization” not worth the effort/readability cost).
tACK ec3075d7342b65bdd72e34f5705f6cdc8a948b67, tested on Debian Sid with Qt 5.15.2.
Tested using two bitcoind’s connected to PR’s bitcoin-qt in regtest mode. Did some reconnects, and so far table seems to work fine.
See Qt docs for QAbstractTableModel and QAbstractItemModel classes.
This commit does not change behavior.
Tested ACK e102c90e41fb82e534c155bd7cfb9d0e9852c092 on macos 11.2.3 with depends build.
Also ran with -server and while some peer was selected removing/adding others from -cli didn’t mess with the selection. Also didn’t observe weird behaviors while resizing columns.
158+ for (const auto& node_stats : nodes_stats) {
159+ const CNodeCombinedStats stats{std::get<0>(node_stats), std::get<2>(node_stats), std::get<1>(node_stats)};
160+ new_peers_data.append(stats);
161+ }
162+
163+ // Handle a new peer addition or an old peer in a right way. See:
nit, or something similar
0 // Handle new peer addition or removal as suggested in Qt Docs. See:
161+ }
162+
163+ // Handle a new peer addition or an old peer in a right way. See:
164+ // - https://doc.qt.io/qt-5/model-view-programming.html#inserting-and-removing-rows
165+ // - https://doc.qt.io/qt-5/model-view-programming.html#resizable-models
166+ // We use the fact that interfaces::Node::getNodesStats
0 // We take advantage of the fact that the std::vector returned by interfaces::Node::getNodesStats
162+
163+ // Handle a new peer addition or an old peer in a right way. See:
164+ // - https://doc.qt.io/qt-5/model-view-programming.html#inserting-and-removing-rows
165+ // - https://doc.qt.io/qt-5/model-view-programming.html#resizable-models
166+ // We use the fact that interfaces::Node::getNodesStats
167+ // returns std::vector naturally sorted by nodeid.
0 // is sorted by nodeid.
ACK e102c90e41fb82e534c155bd7cfb9d0e9852c092
Tested functionality on Arch Linux Qt 5.15.2, macOS 11.3 Qt 5.15.2, and cross-compile from Linux to Windows 10.
PeerTablePriv class would no longer be needed.The below screenshots show that this PR will close the mutated version of #160 (see, #160 (comment)). While I am not including videos showing that this PR also fixes #191, I can confirm that it does on all three tested platforms.
| Select Peer 6 | Disconnect Peer 0, Peer 7 is now selected ❌ |
|---|---|
 |
 |
| Select Peer 9 | Disconnect Peer 1, Peer 9 remains selected ✅ |
|---|---|
 |
 |
| Select Peer 6 | Disconnect Peer 0, Peer 7 is now selected ❌ |
|---|---|
 |
 |
| Select Peer 8 | Disconnect Peer 0, Peer 8 remains selected ✅ |
|---|---|
 |
 |
Note: Difference between UI on master and PR is because master includes ab86ac7739b27f2c45ae72ce4c4ebecf324b3e90
| Select Peer 4 | Disconnect Peer 0, Peer 6 is now selected ❌ |
|---|---|
 |
 |
| Select Peer 5 | Disconnect Peer 1, Peer 5 remains selected ✅ |
|---|---|
 |
 |
This change fixes a bug when a multiple rows selection gets inconsistent
after a peer addition/removal.
re-ACK ecbd911
Only changes since last review are implementation of comment nits