Merge bitcoin-core/gui#164: Handle peer addition/removal in a right way

ecbd91153875c8cdd5b92b840afc116f65e457fb qt: Handle peer addition/removal in a right way (Hennadii Stepanov)
1b66f6e556631a1a2d89aefba70a79894bd14fcd qt: Drop PeerTablePriv class (Hennadii Stepanov)
efb7e5aa962d4a4047061996bbb50b6da4592cbc qt, refactor: Use default arguments for overridden functions (Hennadii Stepanov)

Pull request description:

  This PR makes `PeerTableModel` handle a peer addition/removal in a right way. See:
  - https://doc.qt.io/qt-5/model-view-programming.html#inserting-and-removing-rows
  - https://doc.qt.io/qt-5/model-view-programming.html#resizable-models

  Fixes #160.

  Fixes #191.

ACKs for top commit:
  jarolrod:
    re-ACK ecbd911
  promag:
    reACK ecbd91153875c8cdd5b92b840afc116f65e457fb just improvements to the comment since last review.

Tree-SHA512: 074935d67f78561724218e8b33822e2de16749f873c29054926b720ffcd642f08249a222b563983cf65a9b716290aa14e2372c47fc04e5f401f759db25ca710f

src/qt/peertablemodel.cpp

@@ -14,53 +14,11 @@
 #include <QList>
 #include <QTimer>
 
-// private implementation
-class PeerTablePriv
-{
-public:
-    /** Local cache of peer information */
-    QList<CNodeCombinedStats> cachedNodeStats;
-
-    /** Pull a full list of peers from vNodes into our cache */
-    void refreshPeers(interfaces::Node& node)
-    {
-            cachedNodeStats.clear();
-
-            interfaces::Node::NodesStats nodes_stats;
-            node.getNodesStats(nodes_stats);
-            cachedNodeStats.reserve(nodes_stats.size());
-
-            for (const auto& node_stats : nodes_stats)
-            {
-                CNodeCombinedStats stats;
-                stats.nodeStats = std::get<0>(node_stats);
-                stats.fNodeStateStatsAvailable = std::get<1>(node_stats);
-                stats.nodeStateStats = std::get<2>(node_stats);
-                cachedNodeStats.append(stats);
-            }
-    }
-
-    int size() const
-    {
-        return cachedNodeStats.size();
-    }
-
-    CNodeCombinedStats *index(int idx)
-    {
-        if (idx >= 0 && idx < cachedNodeStats.size())
-            return &cachedNodeStats[idx];
-
-        return nullptr;
-    }
-};
-
 PeerTableModel::PeerTableModel(interfaces::Node& node, QObject* parent) :
     QAbstractTableModel(parent),
     m_node(node),
     timer(nullptr)
 {
-    priv.reset(new PeerTablePriv());
-
     // set up timer for auto refresh
     timer = new QTimer(this);
     connect(timer, &QTimer::timeout, this, &PeerTableModel::refresh);
@@ -85,15 +43,15 @@ void PeerTableModel::stopAutoRefresh()
     timer->stop();
 }
 
-int PeerTableModel::rowCount(const QModelIndex &parent) const
+int PeerTableModel::rowCount(const QModelIndex& parent) const
 {
     if (parent.isValid()) {
         return 0;
     }
-    return priv->size();
+    return m_peers_data.size();
 }
 
-int PeerTableModel::columnCount(const QModelIndex &parent) const
+int PeerTableModel::columnCount(const QModelIndex& parent) const
 {
     if (parent.isValid()) {
         return 0;
@@ -101,7 +59,7 @@ int PeerTableModel::columnCount(const QModelIndex &parent) const
     return columns.length();
 }
 
-QVariant PeerTableModel::data(const QModelIndex &index, int role) const
+QVariant PeerTableModel::data(const QModelIndex& index, int role) const
 {
     if(!index.isValid())
         return QVariant();
@@ -174,19 +132,52 @@ Qt::ItemFlags PeerTableModel::flags(const QModelIndex &index) const
     return retval;
 }
 
-QModelIndex PeerTableModel::index(int row, int column, const QModelIndex &parent) const
+QModelIndex PeerTableModel::index(int row, int column, const QModelIndex& parent) const
 {
     Q_UNUSED(parent);
-    CNodeCombinedStats *data = priv->index(row);
 
-    if (data)
+    if (0 <= row && row < rowCount() && 0 <= column && column < columnCount()) {
-        return createIndex(row, column, data);
+        return createIndex(row, column, const_cast<CNodeCombinedStats*>(&m_peers_data[row]));
+    }
+
     return QModelIndex();
 }
 
 void PeerTableModel::refresh()
 {
-    Q_EMIT layoutAboutToBeChanged();
+    interfaces::Node::NodesStats nodes_stats;
-    priv->refreshPeers(m_node);
+    m_node.getNodesStats(nodes_stats);
-    Q_EMIT layoutChanged();
+    decltype(m_peers_data) new_peers_data;
+    new_peers_data.reserve(nodes_stats.size());
+    for (const auto& node_stats : nodes_stats) {
+        const CNodeCombinedStats stats{std::get<0>(node_stats), std::get<2>(node_stats), std::get<1>(node_stats)};
+        new_peers_data.append(stats);
+    }
+
+    // Handle peer addition or removal as suggested in Qt Docs. See:
+    // - https://doc.qt.io/qt-5/model-view-programming.html#inserting-and-removing-rows
+    // - https://doc.qt.io/qt-5/model-view-programming.html#resizable-models
+    // We take advantage of the fact that the std::vector returned
+    // by interfaces::Node::getNodesStats is sorted by nodeid.
+    for (int i = 0; i < m_peers_data.size();) {
+        if (i < new_peers_data.size() && m_peers_data.at(i).nodeStats.nodeid == new_peers_data.at(i).nodeStats.nodeid) {
+            ++i;
+            continue;
+        }
+        // A peer has been removed from the table.
+        beginRemoveRows(QModelIndex(), i, i);
+        m_peers_data.erase(m_peers_data.begin() + i);
+        endRemoveRows();
+    }
+
+    if (m_peers_data.size() < new_peers_data.size()) {
+        // Some peers have been added to the end of the table.
+        beginInsertRows(QModelIndex(), m_peers_data.size(), new_peers_data.size() - 1);
+        m_peers_data.swap(new_peers_data);
+        endInsertRows();
+    } else {
+        m_peers_data.swap(new_peers_data);
+    }
+
+    Q_EMIT changed();
 }

src/qt/peertablemodel.h

@@ -8,10 +8,11 @@
 #include <net_processing.h> // For CNodeStateStats
 #include <net.h>
 
-#include <memory>
-
 #include <QAbstractTableModel>
+#include <QList>
+#include <QModelIndex>
 #include <QStringList>
+#include <QVariant>
 
 class PeerTablePriv;
 
@@ -61,18 +62,23 @@ public:
 
     /** @name Methods overridden from QAbstractTableModel
         @{*/
-    int rowCount(const QModelIndex &parent) const override;
+    int rowCount(const QModelIndex& parent = QModelIndex()) const override;
-    int columnCount(const QModelIndex &parent) const override;
+    int columnCount(const QModelIndex& parent = QModelIndex()) const override;
-    QVariant data(const QModelIndex &index, int role) const override;
+    QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override;
-    QVariant headerData(int section, Qt::Orientation orientation, int role) const override;
+    QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const override;
-    QModelIndex index(int row, int column, const QModelIndex &parent) const override;
+    QModelIndex index(int row, int column, const QModelIndex& parent = QModelIndex()) const override;
     Qt::ItemFlags flags(const QModelIndex &index) const override;
     /*@}*/
 
 public Q_SLOTS:
     void refresh();
 
+Q_SIGNALS:
+    void changed();
+
 private:
+    //! Internal peer data structure.
+    QList<CNodeCombinedStats> m_peers_data{};
     interfaces::Node& m_node;
     const QStringList columns{
         /*: Title of Peers Table column which contains a
@@ -99,7 +105,6 @@ private:
         /*: Title of Peers Table column which contains the peer's
             User Agent string. */
         tr("User Agent")};
-    std::unique_ptr<PeerTablePriv> priv;
     QTimer *timer;
 };
 

src/qt/rpcconsole.cpp

@@ -727,7 +727,7 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
 
         // peer table signal handling - update peer details when selecting new node
         connect(ui->peerWidget->selectionModel(), &QItemSelectionModel::selectionChanged, this, &RPCConsole::updateDetailWidget);
-        connect(model->getPeerTableModel(), &PeerTableModel::layoutChanged, this, &RPCConsole::updateDetailWidget);
+        connect(model->getPeerTableModel(), &PeerTableModel::changed, this, &RPCConsole::updateDetailWidget);
 
         // set up ban table
         ui->banlistWidget->setModel(model->getBanTableModel());