Merge bitcoin-core/gui#18: Add peertablesortproxy module

5a4a15d2b4456272fd8aa080195f40a09576ae01 qt, refactor: Drop no longer used PeerTableModel::getRowByNodeId func (Hennadii Stepanov)
9a9f180df0d51396fee2468681df6dd935b0248e qt, refactor: Drop no longer used PeerTableModel::sort function (Hennadii Stepanov)
778a64af209e4fa692a3aca8376ba1bd5e1af881 qt: Use PeerTableSortProxy for sorting peer table (Hennadii Stepanov)
df2d165ba9e0acc53f36a326f68f57ad9c297872 qt: Add peertablesortproxy module (Hennadii Stepanov)

Pull request description:

  The "Peers" table in the "Node" window does not hold multiple selection after sorting.

  This PR introduces a `QSortFilterProxyModel` subclass, that is a standard Qt [practice](https://doc.qt.io/qt-5/model-view-programming.html#custom-sorting-models) for such cases.

  Now the sorting code is encapsulated into the dedicated Qt class, and we do not need to maintain it.

  Fixes #283 (additionally).

  ---

  On **master** (7ae86b3c6845873ca96650fc69beb4ae5285c801):
  - rows are sorted by "Ping", and a selection is made
  ![Screenshot from 2020-11-28 22-53-11](https://user-images.githubusercontent.com/32963518/100525900-96eaed00-31cc-11eb-86e7-72ede3b8b33c.png)

  - rows are sorted by "NodeId", and the previous selection is _lost_
  ![Screenshot from 2020-11-28 22-53-21](https://user-images.githubusercontent.com/32963518/100525904-9c483780-31cc-11eb-957c-06f53d7d31ab.png)

  With **this PR**:
  - rows are sorted by "Ping", and a selection is made
  ![Screenshot from 2020-11-28 22-39-41](https://user-images.githubusercontent.com/32963518/100525776-06aca800-31cc-11eb-8c4e-9c6566fe80fe.png)

  - rows are sorted by "NodeId", and the row are still selected
  ![Screenshot from 2020-11-28 22-39-53](https://user-images.githubusercontent.com/32963518/100525791-2348e000-31cc-11eb-8b78-716a5551d7ec.png)

ACKs for top commit:
  jarolrod:
    re-ACK 5a4a15d2b4456272fd8aa080195f40a09576ae01, tested on macOS 11.2 Qt 5.15.2 after rebase
  promag:
    Tested ACK 5a4a15d2b4456272fd8aa080195f40a09576ae01.

Tree-SHA512: f81c1385892fbf1a46ffb98b42094ca1cc97da52114bbbc94fedb553899b1f18c26a349e186bba6e27922a89426bd61e8bc88b1f7832512dbe211b5f834e076e

9 files changed, 91 insertions, 155 deletions

diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include
index a573247afa..caa8500ffa 100644
--- a/src/Makefile.qt.include
+++ b/src/Makefile.qt.include
@@ -61,6 +61,7 @@ QT_MOC_CPP = \
   qt/moc_optionsmodel.cpp \
   qt/moc_overviewpage.cpp \
   qt/moc_peertablemodel.cpp \
+  qt/moc_peertablesortproxy.cpp \
   qt/moc_paymentserver.cpp \
   qt/moc_psbtoperationsdialog.cpp \
   qt/moc_qrimagewidget.cpp \
@@ -134,6 +135,7 @@ BITCOIN_QT_H = \
   qt/overviewpage.h \
   qt/paymentserver.h \
   qt/peertablemodel.h \
+  qt/peertablesortproxy.h \
   qt/platformstyle.h \
   qt/psbtoperationsdialog.h \
   qt/qrimagewidget.h \
@@ -232,6 +234,7 @@ BITCOIN_QT_BASE_CPP = \
   qt/optionsdialog.cpp \
   qt/optionsmodel.cpp \
   qt/peertablemodel.cpp \
+  qt/peertablesortproxy.cpp \
   qt/platformstyle.cpp \
   qt/qvalidatedlineedit.cpp \
   qt/qvaluecombobox.cpp \
diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp
index f2c555de52..04161020b2 100644
--- a/src/qt/clientmodel.cpp
+++ b/src/qt/clientmodel.cpp
@@ -8,6 +8,7 @@
 #include <qt/guiconstants.h>
 #include <qt/guiutil.h>
 #include <qt/peertablemodel.h>
+#include <qt/peertablesortproxy.h>
 
 #include <clientversion.h>
 #include <interfaces/handler.h>
@@ -38,7 +39,11 @@ ClientModel::ClientModel(interfaces::Node& node, OptionsModel *_optionsModel, QO
 {
     cachedBestHeaderHeight = -1;
     cachedBestHeaderTime = -1;
+
     peerTableModel = new PeerTableModel(m_node, this);
+    m_peer_table_sort_proxy = new PeerTableSortProxy(this);
+    m_peer_table_sort_proxy->setSourceModel(peerTableModel);
+
     banTableModel = new BanTableModel(m_node, this);
 
     QTimer* timer = new QTimer;
@@ -184,6 +189,11 @@ PeerTableModel *ClientModel::getPeerTableModel()
     return peerTableModel;
 }
 
+PeerTableSortProxy* ClientModel::peerTableSortProxy()
+{
+    return m_peer_table_sort_proxy;
+}
+
 BanTableModel *ClientModel::getBanTableModel()
 {
     return banTableModel;
diff --git a/src/qt/clientmodel.h b/src/qt/clientmodel.h
index 7ac4cc040b..7a199ef19c 100644
--- a/src/qt/clientmodel.h
+++ b/src/qt/clientmodel.h
@@ -17,6 +17,7 @@ class BanTableModel;
 class CBlockIndex;
 class OptionsModel;
 class PeerTableModel;
+class PeerTableSortProxy;
 enum class SynchronizationState;
 
 namespace interfaces {
@@ -54,6 +55,7 @@ public:
     interfaces::Node& node() const { return m_node; }
     OptionsModel *getOptionsModel();
     PeerTableModel *getPeerTableModel();
+    PeerTableSortProxy* peerTableSortProxy();
     BanTableModel *getBanTableModel();
 
     //! Return number of connections, default is in- and outbound (total)
@@ -96,6 +98,7 @@ private:
     std::unique_ptr<interfaces::Handler> m_handler_notify_header_tip;
     OptionsModel *optionsModel;
     PeerTableModel *peerTableModel;
+    PeerTableSortProxy* m_peer_table_sort_proxy{nullptr};
     BanTableModel *banTableModel;
 
     //! A thread to interact with m_node asynchronously
diff --git a/src/qt/peertablemodel.cpp b/src/qt/peertablemodel.cpp
index 3459bf4cf8..6c4e326011 100644
--- a/src/qt/peertablemodel.cpp
+++ b/src/qt/peertablemodel.cpp
@@ -11,56 +11,19 @@
 
 #include <utility>
 
-#include <QDebug>
 #include <QList>
 #include <QTimer>
 
-bool NodeLessThan::operator()(const CNodeCombinedStats &left, const CNodeCombinedStats &right) const
-{
-    const CNodeStats *pLeft = &(left.nodeStats);
-    const CNodeStats *pRight = &(right.nodeStats);
-
-    if (order == Qt::DescendingOrder)
-        std::swap(pLeft, pRight);
-
-    switch (static_cast<PeerTableModel::ColumnIndex>(column)) {
-    case PeerTableModel::NetNodeId:
-        return pLeft->nodeid < pRight->nodeid;
-    case PeerTableModel::Address:
-        return pLeft->addrName.compare(pRight->addrName) < 0;
-    case PeerTableModel::ConnectionType:
-        return pLeft->m_conn_type < pRight->m_conn_type;
-    case PeerTableModel::Network:
-        return pLeft->m_network < pRight->m_network;
-    case PeerTableModel::Ping:
-        return pLeft->m_min_ping_time < pRight->m_min_ping_time;
-    case PeerTableModel::Sent:
-        return pLeft->nSendBytes < pRight->nSendBytes;
-    case PeerTableModel::Received:
-        return pLeft->nRecvBytes < pRight->nRecvBytes;
-    case PeerTableModel::Subversion:
-        return pLeft->cleanSubVer.compare(pRight->cleanSubVer) < 0;
-    } // no default case, so the compiler can warn about missing cases
-    assert(false);
-}
-
 // private implementation
 class PeerTablePriv
 {
 public:
     /** Local cache of peer information */
     QList<CNodeCombinedStats> cachedNodeStats;
-    /** Column to sort nodes by (default to unsorted) */
-    int sortColumn{-1};
-    /** Order (ascending or descending) to sort nodes by */
-    Qt::SortOrder sortOrder;
-    /** Index of rows by node ID */
-    std::map<NodeId, int> mapNodeRows;
 
     /** Pull a full list of peers from vNodes into our cache */
     void refreshPeers(interfaces::Node& node)
     {
-        {
             cachedNodeStats.clear();
 
             interfaces::Node::NodesStats nodes_stats;
@@ -74,17 +37,6 @@ public:
                 stats.nodeStateStats = std::get<2>(node_stats);
                 cachedNodeStats.append(stats);
             }
-        }
-
-        if (sortColumn >= 0)
-            // sort cacheNodeStats (use stable sort to prevent rows jumping around unnecessarily)
-            std::stable_sort(cachedNodeStats.begin(), cachedNodeStats.end(), NodeLessThan(sortColumn, sortOrder));
-
-        // build index map
-        mapNodeRows.clear();
-        int row = 0;
-        for (const CNodeCombinedStats& stats : cachedNodeStats)
-            mapNodeRows.insert(std::pair<NodeId, int>(stats.nodeStats.nodeid, row++));
     }
 
     int size() const
@@ -194,10 +146,7 @@ QVariant PeerTableModel::data(const QModelIndex &index, int role) const
         } // no default case, so the compiler can warn about missing cases
         assert(false);
     } else if (role == StatsRole) {
-        switch (index.column()) {
-        case NetNodeId: return QVariant::fromValue(rec);
-        default: return QVariant();
-        }
+        return QVariant::fromValue(rec);
     }
 
     return QVariant();
@@ -239,19 +188,3 @@ void PeerTableModel::refresh()
     priv->refreshPeers(m_node);
     Q_EMIT layoutChanged();
 }
-
-int PeerTableModel::getRowByNodeId(NodeId nodeid)
-{
-    std::map<NodeId, int>::iterator it = priv->mapNodeRows.find(nodeid);
-    if (it == priv->mapNodeRows.end())
-        return -1;
-
-    return it->second;
-}
-
-void PeerTableModel::sort(int column, Qt::SortOrder order)
-{
-    priv->sortColumn = column;
-    priv->sortOrder = order;
-    refresh();
-}
diff --git a/src/qt/peertablemodel.h b/src/qt/peertablemodel.h
index 0823235ec0..9c7bc25da2 100644
--- a/src/qt/peertablemodel.h
+++ b/src/qt/peertablemodel.h
@@ -30,18 +30,6 @@ struct CNodeCombinedStats {
 };
 Q_DECLARE_METATYPE(CNodeCombinedStats*)
 
-class NodeLessThan
-{
-public:
-    NodeLessThan(int nColumn, Qt::SortOrder fOrder) :
-        column(nColumn), order(fOrder) {}
-    bool operator()(const CNodeCombinedStats &left, const CNodeCombinedStats &right) const;
-
-private:
-    int column;
-    Qt::SortOrder order;
-};
-
 /**
    Qt model providing information about connected peers, similar to the
    "getpeerinfo" RPC call. Used by the rpc console UI.
@@ -53,7 +41,6 @@ class PeerTableModel : public QAbstractTableModel
 public:
     explicit PeerTableModel(interfaces::Node& node, QObject* parent);
     ~PeerTableModel();
-    int getRowByNodeId(NodeId nodeid);
     void startAutoRefresh();
     void stopAutoRefresh();
 
@@ -80,7 +67,6 @@ public:
     QVariant headerData(int section, Qt::Orientation orientation, int role) const override;
     QModelIndex index(int row, int column, const QModelIndex &parent) const override;
     Qt::ItemFlags flags(const QModelIndex &index) const override;
-    void sort(int column, Qt::SortOrder order) override;
     /*@}*/
 
 public Q_SLOTS:
diff --git a/src/qt/peertablesortproxy.cpp b/src/qt/peertablesortproxy.cpp
new file mode 100644
index 0000000000..78932da8d4
--- /dev/null
+++ b/src/qt/peertablesortproxy.cpp
@@ -0,0 +1,43 @@
+// Copyright (c) 2020 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <qt/peertablesortproxy.h>
+
+#include <qt/peertablemodel.h>
+#include <util/check.h>
+
+#include <QModelIndex>
+#include <QString>
+#include <QVariant>
+
+PeerTableSortProxy::PeerTableSortProxy(QObject* parent)
+    : QSortFilterProxyModel(parent)
+{
+}
+
+bool PeerTableSortProxy::lessThan(const QModelIndex& left_index, const QModelIndex& right_index) const
+{
+    const CNodeStats left_stats = Assert(sourceModel()->data(left_index, PeerTableModel::StatsRole).value<CNodeCombinedStats*>())->nodeStats;
+    const CNodeStats right_stats = Assert(sourceModel()->data(right_index, PeerTableModel::StatsRole).value<CNodeCombinedStats*>())->nodeStats;
+
+    switch (static_cast<PeerTableModel::ColumnIndex>(left_index.column())) {
+    case PeerTableModel::NetNodeId:
+        return left_stats.nodeid < right_stats.nodeid;
+    case PeerTableModel::Address:
+        return left_stats.addrName.compare(right_stats.addrName) < 0;
+    case PeerTableModel::ConnectionType:
+        return left_stats.m_conn_type < right_stats.m_conn_type;
+    case PeerTableModel::Network:
+        return left_stats.m_network < right_stats.m_network;
+    case PeerTableModel::Ping:
+        return left_stats.m_min_ping_time < right_stats.m_min_ping_time;
+    case PeerTableModel::Sent:
+        return left_stats.nSendBytes < right_stats.nSendBytes;
+    case PeerTableModel::Received:
+        return left_stats.nRecvBytes < right_stats.nRecvBytes;
+    case PeerTableModel::Subversion:
+        return left_stats.cleanSubVer.compare(right_stats.cleanSubVer) < 0;
+    } // no default case, so the compiler can warn about missing cases
+    assert(false);
+}
diff --git a/src/qt/peertablesortproxy.h b/src/qt/peertablesortproxy.h
new file mode 100644
index 0000000000..1879f6b400
--- /dev/null
+++ b/src/qt/peertablesortproxy.h
@@ -0,0 +1,25 @@
+// Copyright (c) 2020 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_QT_PEERTABLESORTPROXY_H
+#define BITCOIN_QT_PEERTABLESORTPROXY_H
+
+#include <QSortFilterProxyModel>
+
+QT_BEGIN_NAMESPACE
+class QModelIndex;
+QT_END_NAMESPACE
+
+class PeerTableSortProxy : public QSortFilterProxyModel
+{
+    Q_OBJECT
+
+public:
+    explicit PeerTableSortProxy(QObject* parent = nullptr);
+
+protected:
+    bool lessThan(const QModelIndex& left_index, const QModelIndex& right_index) const override;
+};
+
+#endif // BITCOIN_QT_PEERTABLESORTPROXY_H
diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp
index 85412ca75b..006f60e7a1 100644
--- a/src/qt/rpcconsole.cpp
+++ b/src/qt/rpcconsole.cpp
@@ -9,13 +9,14 @@
 #include <qt/rpcconsole.h>
 #include <qt/forms/ui_debugwindow.h>
 
+#include <chainparams.h>
+#include <interfaces/node.h>
+#include <netbase.h>
 #include <qt/bantablemodel.h>
 #include <qt/clientmodel.h>
+#include <qt/peertablesortproxy.h>
 #include <qt/platformstyle.h>
 #include <qt/walletmodel.h>
-#include <chainparams.h>
-#include <interfaces/node.h>
-#include <netbase.h>
 #include <rpc/client.h>
 #include <rpc/server.h>
 #include <util/strencodings.h>
@@ -606,7 +607,7 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
         connect(model, &ClientModel::mempoolSizeChanged, this, &RPCConsole::setMempoolSize);
 
         // set up peer table
-        ui->peerWidget->setModel(model->getPeerTableModel());
+        ui->peerWidget->setModel(model->peerTableSortProxy());
         ui->peerWidget->verticalHeader()->hide();
         ui->peerWidget->setSelectionBehavior(QAbstractItemView::SelectRows);
         ui->peerWidget->setSelectionMode(QAbstractItemView::ExtendedSelection);
@@ -627,10 +628,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);
-        // peer table signal handling - update peer details when new nodes are added to the model
-        connect(model->getPeerTableModel(), &PeerTableModel::layoutChanged, this, &RPCConsole::peerLayoutChanged);
-        // peer table signal handling - cache selected node ids
-        connect(model->getPeerTableModel(), &PeerTableModel::layoutAboutToBeChanged, this, &RPCConsole::peerLayoutAboutToChange);
+        connect(model->getPeerTableModel(), &PeerTableModel::layoutChanged, this, &RPCConsole::updateDetailWidget);
 
         // set up ban table
         ui->banlistWidget->setModel(model->getBanTableModel());
@@ -1014,67 +1012,6 @@ void RPCConsole::updateTrafficStats(quint64 totalBytesIn, quint64 totalBytesOut)
     ui->lblBytesOut->setText(GUIUtil::formatBytes(totalBytesOut));
 }
 
-void RPCConsole::peerLayoutAboutToChange()
-{
-    cachedNodeids.clear();
-    for (const QModelIndex& peer : GUIUtil::getEntryData(ui->peerWidget, PeerTableModel::NetNodeId)) {
-        const auto stats = peer.data(PeerTableModel::StatsRole).value<CNodeCombinedStats*>();
-        cachedNodeids.append(stats->nodeStats.nodeid);
-    }
-}
-
-void RPCConsole::peerLayoutChanged()
-{
-    if (!clientModel || !clientModel->getPeerTableModel())
-        return;
-
-    bool fUnselect = false;
-    bool fReselect = false;
-
-    if (cachedNodeids.empty()) // no node selected yet
-        return;
-
-    // find the currently selected row
-    int selectedRow = -1;
-    QModelIndexList selectedModelIndex = ui->peerWidget->selectionModel()->selectedIndexes();
-    if (!selectedModelIndex.isEmpty()) {
-        selectedRow = selectedModelIndex.first().row();
-    }
-
-    // check if our detail node has a row in the table (it may not necessarily
-    // be at selectedRow since its position can change after a layout change)
-    int detailNodeRow = clientModel->getPeerTableModel()->getRowByNodeId(cachedNodeids.first());
-
-    if (detailNodeRow < 0)
-    {
-        // detail node disappeared from table (node disconnected)
-        fUnselect = true;
-    }
-    else
-    {
-        if (detailNodeRow != selectedRow)
-        {
-            // detail node moved position
-            fUnselect = true;
-            fReselect = true;
-        }
-    }
-
-    if (fUnselect && selectedRow >= 0) {
-        clearSelectedNode();
-    }
-
-    if (fReselect)
-    {
-        for(int i = 0; i < cachedNodeids.size(); i++)
-        {
-            ui->peerWidget->selectRow(clientModel->getPeerTableModel()->getRowByNodeId(cachedNodeids.at(i)));
-        }
-    }
-
-    updateDetailWidget();
-}
-
 void RPCConsole::updateDetailWidget()
 {
     const QList<QModelIndex> selected_peers = GUIUtil::getEntryData(ui->peerWidget, PeerTableModel::NetNodeId);
diff --git a/src/qt/rpcconsole.h b/src/qt/rpcconsole.h
index b9806e40c9..5182d60a0d 100644
--- a/src/qt/rpcconsole.h
+++ b/src/qt/rpcconsole.h
@@ -118,10 +118,6 @@ public Q_SLOTS:
     void browseHistory(int offset);
     /** Scroll console view to end */
     void scrollToEnd();
-    /** Handle selection caching before update */
-    void peerLayoutAboutToChange();
-    /** Handle updated peer information */
-    void peerLayoutChanged();
     /** Disconnect a selected node on the Peers tab */
     void disconnectSelectedNode();
     /** Ban a selected node on the Peers tab */