Clean up PeerListWidget class

src/gui/properties/peerlistwidget.cpp

@@ -44,6 +44,7 @@
 #include "base/bittorrent/peerinfo.h"
 #include "base/bittorrent/session.h"
 #include "base/bittorrent/torrenthandle.h"
+#include "base/global.h"
 #include "base/logger.h"
 #include "base/net/geoipmanager.h"
 #include "base/net/reverseresolution.h"
@@ -114,15 +115,15 @@ PeerListWidget::PeerListWidget(PropertiesWidget *parent)
     // To also mitigate the above issue, we have to resize each column when
     // its size is 0, because explicitly 'showing' the column isn't enough
     // in the above scenario.
-    for (int i = 0; i < PeerListDelegate::IP_HIDDEN; ++i)
+    for (int i = 0; i < PeerListDelegate::IP_HIDDEN; ++i) {
         if ((columnWidth(i) <= 0) && !isColumnHidden(i))
             resizeColumnToContents(i);
+    }
     // Context menu
     setContextMenuPolicy(Qt::CustomContextMenu);
     connect(this, &QWidget::customContextMenuRequested, this, &PeerListWidget::showPeerListMenu);
     // List delegate
-    m_listDelegate = new PeerListDelegate(this);
-    setItemDelegate(m_listDelegate);
+    setItemDelegate(new PeerListDelegate(this));
     // Enable sorting
     setSortingEnabled(true);
     // IP to Hostname resolver
@@ -149,8 +150,6 @@ PeerListWidget::PeerListWidget(PropertiesWidget *parent)
 PeerListWidget::~PeerListWidget()
 {
     saveSettings();
-    if (m_resolver)
-        delete m_resolver;
 }
 
 void PeerListWidget::displayToggleColumnsMenu(const QPoint &)
@@ -201,28 +200,31 @@ void PeerListWidget::updatePeerHostNameResolutionState()
     if (Preferences::instance()->resolvePeerHostNames()) {
         if (!m_resolver) {
             m_resolver = new Net::ReverseResolution(this);
-            connect(m_resolver.data(), &Net::ReverseResolution::ipResolved, this, &PeerListWidget::handleResolved);
-            loadPeers(m_properties->getCurrentTorrent(), true);
+            connect(m_resolver, &Net::ReverseResolution::ipResolved, this, &PeerListWidget::handleResolved);
+            loadPeers(m_properties->getCurrentTorrent());
         }
     }
-    else if (m_resolver) {
+    else {
         delete m_resolver;
+        m_resolver = nullptr;
     }
 }
 
 void PeerListWidget::updatePeerCountryResolutionState()
 {
-    if (Preferences::instance()->resolvePeerCountries() != m_resolveCountries) {
-        m_resolveCountries = !m_resolveCountries;
-        if (m_resolveCountries) {
-            loadPeers(m_properties->getCurrentTorrent());
-            showColumn(PeerListDelegate::COUNTRY);
-            if (columnWidth(PeerListDelegate::COUNTRY) <= 0)
-                resizeColumnToContents(PeerListDelegate::COUNTRY);
-        }
-        else {
-            hideColumn(PeerListDelegate::COUNTRY);
-        }
+    const bool resolveCountries = Preferences::instance()->resolvePeerCountries();
+    if (resolveCountries == m_resolveCountries)
+        return;
+
+    m_resolveCountries = resolveCountries;
+    if (m_resolveCountries) {
+        loadPeers(m_properties->getCurrentTorrent());
+        showColumn(PeerListDelegate::COUNTRY);
+        if (columnWidth(PeerListDelegate::COUNTRY) <= 0)
+            resizeColumnToContents(PeerListDelegate::COUNTRY);
+    }
+    else {
+        hideColumn(PeerListDelegate::COUNTRY);
     }
 }
 
@@ -243,17 +245,17 @@ void PeerListWidget::showPeerListMenu(const QPoint &)
             int peerCount = 0;
             for (const BitTorrent::PeerAddress &addr : peersList) {
                 if (torrent->connectPeer(addr)) {
-                    Logger::instance()->addMessage(tr("Manually adding peer '%1'...").arg(addr.ip.toString()));
                     ++peerCount;
+                    LogMsg(tr("Peer \"%1\" added to \"%2\"").arg(addr.ip.toString(), torrent->name()));
                 }
                 else {
-                    Logger::instance()->addMessage(tr("The peer '%1' could not be added to this torrent.").arg(addr.ip.toString()), Log::WARNING);
+                    LogMsg(tr("Failed to add peer \"%1\" to \"%2\".").arg(addr.ip.toString(), torrent->name()), Log::WARNING);
                 }
             }
             if (peerCount < peersList.length())
-                QMessageBox::information(this, tr("Peer addition"), tr("Some peers could not be added. Check the Log for details."));
+                QMessageBox::information(this, tr("Adding peers"), tr("Some peers cannot be added. Check the Log for details."));
             else if (peerCount > 0)
-                QMessageBox::information(this, tr("Peer addition"), tr("The peers were added to this torrent."));
+                QMessageBox::information(this, tr("Adding peers"), tr("Peers are added to this torrent."));
         });
     }
 
@@ -276,18 +278,16 @@ void PeerListWidget::showPeerListMenu(const QPoint &)
 void PeerListWidget::banSelectedPeers()
 {
     // Confirm first
-    int ret = QMessageBox::question(this, tr("Ban peer permanently"), tr("Are you sure you want to permanently ban the selected peers?"),
-                                    tr("&Yes"), tr("&No"),
-                                    QString(), 0, 1);
-    if (ret) return;
+    const QMessageBox::StandardButton btn = QMessageBox::question(this, tr("Ban peer permanently")
+        , tr("Are you sure you want to permanently ban the selected peers?"));
+    if (btn != QMessageBox::Yes) return;
 
     const QModelIndexList selectedIndexes = selectionModel()->selectedRows();
     for (const QModelIndex &index : selectedIndexes) {
-        int row = m_proxyModel->mapToSource(index).row();
-        QString ip = m_listModel->data(m_listModel->index(row, PeerListDelegate::IP_HIDDEN)).toString();
-        qDebug("Banning peer %s...", ip.toLocal8Bit().data());
-        Logger::instance()->addMessage(tr("Manually banning peer '%1'...").arg(ip));
+        const int row = m_proxyModel->mapToSource(index).row();
+        const QString ip = m_listModel->item(row, PeerListDelegate::IP_HIDDEN)->text();
         BitTorrent::Session::instance()->banIP(ip);
+        LogMsg(tr("Peer \"%1\" is manually banned").arg(ip));
     }
     // Refresh list
     loadPeers(m_properties->getCurrentTorrent());
@@ -297,29 +297,27 @@ void PeerListWidget::copySelectedPeers()
 {
     const QModelIndexList selectedIndexes = selectionModel()->selectedRows();
     QStringList selectedPeers;
+
     for (const QModelIndex &index : selectedIndexes) {
-        int row = m_proxyModel->mapToSource(index).row();
-        QString ip = m_listModel->data(m_listModel->index(row, PeerListDelegate::IP_HIDDEN)).toString();
-        QString myport = m_listModel->data(m_listModel->index(row, PeerListDelegate::PORT)).toString();
-        if (!ip.contains('.')) // IPv6
-            selectedPeers << '[' + ip + "]:" + myport;
-        else // IPv4
-            selectedPeers << ip + ':' + myport;
+        const int row = m_proxyModel->mapToSource(index).row();
+        const QString ip = m_listModel->item(row, PeerListDelegate::IP_HIDDEN)->text();
+        const QString port = m_listModel->item(row, PeerListDelegate::PORT)->text();
+
+        if (!ip.contains('.'))  // IPv6
+            selectedPeers << ('[' + ip + "]:" + port);
+        else  // IPv4
+            selectedPeers << (ip + ':' + port);
     }
+
     QApplication::clipboard()->setText(selectedPeers.join('\n'));
 }
 
 void PeerListWidget::clear()
 {
-    qDebug("clearing peer list");
     m_peerItems.clear();
-    m_peerAddresses.clear();
-    m_missingFlags.clear();
-    int nbrows = m_listModel->rowCount();
-    if (nbrows > 0) {
-        qDebug("Cleared %d peers", nbrows);
+    const int nbrows = m_listModel->rowCount();
+    if (nbrows > 0)
         m_listModel->removeRows(0, nbrows);
-    }
 }
 
 void PeerListWidget::loadSettings()
@@ -332,63 +330,50 @@ void PeerListWidget::saveSettings() const
     Preferences::instance()->setPeerListState(header()->saveState());
 }
 
-void PeerListWidget::loadPeers(BitTorrent::TorrentHandle *const torrent, bool forceHostnameResolution)
+void PeerListWidget::loadPeers(const BitTorrent::TorrentHandle *torrent)
 {
     if (!torrent) return;
 
     const QVector<BitTorrent::PeerInfo> peers = torrent->peers();
-    QSet<QString> oldPeersSet = m_peerItems.keys().toSet();
+    QSet<QString> existingPeers;
+    for (auto i = m_peerItems.cbegin(); i != m_peerItems.cend(); ++i)
+        existingPeers << i.key();
 
     for (const BitTorrent::PeerInfo &peer : peers) {
-        BitTorrent::PeerAddress addr = peer.address();
+        const BitTorrent::PeerAddress addr = peer.address();
         if (addr.ip.isNull()) continue;
 
-        QString peerIp = addr.ip.toString();
-        if (m_peerItems.contains(peerIp)) {
-            // Update existing peer
-            updatePeer(peerIp, torrent, peer);
-            oldPeersSet.remove(peerIp);
-            if (forceHostnameResolution && m_resolver)
-                m_resolver->resolve(peerIp);
-        }
-        else {
-            // Add new peer
-            m_peerItems[peerIp] = addPeer(peerIp, torrent, peer);
-            m_peerAddresses[peerIp] = addr;
-            // Resolve peer host name is asked
-            if (m_resolver)
-                m_resolver->resolve(peerIp);
-        }
+        const QString peerIp = addr.ip.toString();
+        bool isNewPeer = false;
+        updatePeer(peerIp, torrent, peer, isNewPeer);
+        if (!isNewPeer)
+            existingPeers.remove(peerIp);
     }
-    // Delete peers that are gone
-    for (const QString &ip : oldPeersSet) {
-        m_missingFlags.remove(ip);
-        m_peerAddresses.remove(ip);
-        QStandardItem *item = m_peerItems.take(ip);
+
+    // Remove peers that are gone
+    for (const QString &ip : asConst(existingPeers)) {
+        const QStandardItem *item = m_peerItems.take(ip);
         m_listModel->removeRow(item->row());
     }
 }
 
-QStandardItem *PeerListWidget::addPeer(const QString &ip, BitTorrent::TorrentHandle *const torrent, const BitTorrent::PeerInfo &peer)
+void PeerListWidget::updatePeer(const QString &ip, const BitTorrent::TorrentHandle *torrent, const BitTorrent::PeerInfo &peer, bool &isNewPeer)
 {
-    int row = m_listModel->rowCount();
-    // Adding Peer to peer list
-    m_listModel->insertRow(row);
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::IP), ip);
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::IP), ip, Qt::ToolTipRole);
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::PORT), peer.address().port);
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::IP_HIDDEN), ip);
-    if (m_resolveCountries) {
-        const QIcon ico = UIThemeManager::instance()->getFlagIcon(peer.country());
-        if (!ico.isNull()) {
-            m_listModel->setData(m_listModel->index(row, PeerListDelegate::COUNTRY), ico, Qt::DecorationRole);
-            const QString countryName = Net::GeoIPManager::CountryName(peer.country());
-            m_listModel->setData(m_listModel->index(row, PeerListDelegate::COUNTRY), countryName, Qt::ToolTipRole);
-        }
-        else {
-            m_missingFlags.insert(ip);
-        }
+    auto itemIter = m_peerItems.find(ip);
+    isNewPeer = (itemIter == m_peerItems.end());
+    if (isNewPeer) {
+        // new item
+        const int row = m_listModel->rowCount();
+        m_listModel->insertRow(row);
+        m_listModel->setData(m_listModel->index(row, PeerListDelegate::IP), ip);
+        m_listModel->setData(m_listModel->index(row, PeerListDelegate::IP), ip, Qt::ToolTipRole);
+        m_listModel->setData(m_listModel->index(row, PeerListDelegate::PORT), peer.address().port);
+        m_listModel->setData(m_listModel->index(row, PeerListDelegate::IP_HIDDEN), ip);
+
+        itemIter = m_peerItems.insert(ip, m_listModel->item(row, PeerListDelegate::IP));
     }
+
+    const int row = (*itemIter)->row();
     m_listModel->setData(m_listModel->index(row, PeerListDelegate::CONNECTION), peer.connectionType());
     m_listModel->setData(m_listModel->index(row, PeerListDelegate::FLAGS), peer.flags());
     m_listModel->setData(m_listModel->index(row, PeerListDelegate::FLAGS), peer.flagsDescription(), Qt::ToolTipRole);
@@ -399,60 +384,37 @@ QStandardItem *PeerListWidget::addPeer(const QString &ip, BitTorrent::TorrentHan
     m_listModel->setData(m_listModel->index(row, PeerListDelegate::TOT_DOWN), peer.totalDownload());
     m_listModel->setData(m_listModel->index(row, PeerListDelegate::TOT_UP), peer.totalUpload());
     m_listModel->setData(m_listModel->index(row, PeerListDelegate::RELEVANCE), peer.relevance());
-    QStringList downloadingFiles(torrent->info().filesForPiece(peer.downloadingPieceIndex()));
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::DOWNLOADING_PIECE), downloadingFiles.join(QLatin1Char(';')));
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::DOWNLOADING_PIECE), downloadingFiles.join(QLatin1Char('\n')), Qt::ToolTipRole);
 
-    return m_listModel->item(row, PeerListDelegate::IP);
-}
+    const QStringList downloadingFiles {torrent->info().filesForPiece(peer.downloadingPieceIndex())};
+    m_listModel->setData(m_listModel->index(row, PeerListDelegate::DOWNLOADING_PIECE), downloadingFiles.join(';'));
+    m_listModel->setData(m_listModel->index(row, PeerListDelegate::DOWNLOADING_PIECE), downloadingFiles.join('\n'), Qt::ToolTipRole);
+
+    if (m_resolver)
+        m_resolver->resolve(ip);
 
-void PeerListWidget::updatePeer(const QString &ip, BitTorrent::TorrentHandle *const torrent, const BitTorrent::PeerInfo &peer)
-{
-    QStandardItem *item = m_peerItems.value(ip);
-    int row = item->row();
     if (m_resolveCountries) {
-        const QIcon ico = UIThemeManager::instance()->getFlagIcon(peer.country());
-        if (!ico.isNull()) {
-            m_listModel->setData(m_listModel->index(row, PeerListDelegate::COUNTRY), ico, Qt::DecorationRole);
+        const QIcon icon = UIThemeManager::instance()->getFlagIcon(peer.country());
+        if (!icon.isNull()) {
+            m_listModel->setData(m_listModel->index(row, PeerListDelegate::COUNTRY), icon, Qt::DecorationRole);
             const QString countryName = Net::GeoIPManager::CountryName(peer.country());
             m_listModel->setData(m_listModel->index(row, PeerListDelegate::COUNTRY), countryName, Qt::ToolTipRole);
-            m_missingFlags.remove(ip);
         }
     }
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::CONNECTION), peer.connectionType());
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::PORT), peer.address().port);
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::FLAGS), peer.flags());
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::FLAGS), peer.flagsDescription(), Qt::ToolTipRole);
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::CLIENT), peer.client().toHtmlEscaped());
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::PROGRESS), peer.progress());
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::DOWN_SPEED), peer.payloadDownSpeed());
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::UP_SPEED), peer.payloadUpSpeed());
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::TOT_DOWN), peer.totalDownload());
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::TOT_UP), peer.totalUpload());
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::RELEVANCE), peer.relevance());
-    QStringList downloadingFiles(torrent->info().filesForPiece(peer.downloadingPieceIndex()));
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::DOWNLOADING_PIECE), downloadingFiles.join(QLatin1String(";")));
-    m_listModel->setData(m_listModel->index(row, PeerListDelegate::DOWNLOADING_PIECE), downloadingFiles.join(QLatin1String("\n")), Qt::ToolTipRole);
 }
 
 void PeerListWidget::handleResolved(const QString &ip, const QString &hostname)
 {
     QStandardItem *item = m_peerItems.value(ip, nullptr);
-    if (item) {
-        qDebug("Resolved %s -> %s", qUtf8Printable(ip), qUtf8Printable(hostname));
+    if (item)
         item->setData(hostname, Qt::DisplayRole);
-    }
 }
 
-void PeerListWidget::handleSortColumnChanged(int col)
+void PeerListWidget::handleSortColumnChanged(const int col)
 {
-    if (col == PeerListDelegate::COUNTRY) {
-        qDebug("Sorting by decoration");
+    if (col == PeerListDelegate::COUNTRY)
         m_proxyModel->setSortRole(Qt::ToolTipRole);
-    }
-    else {
+    else
         m_proxyModel->setSortRole(Qt::DisplayRole);
-    }
 }
 
 void PeerListWidget::wheelEvent(QWheelEvent *event)

src/gui/properties/peerlistwidget.h

@@ -30,15 +30,11 @@
 #define PEERLISTWIDGET_H
 
 #include <QHash>
-#include <QPointer>
-#include <QSet>
 #include <QTreeView>
 
-class QSortFilterProxyModel;
 class QStandardItem;
 class QStandardItemModel;
 
-class PeerListDelegate;
 class PeerListSortModel;
 class PropertiesWidget;
 
@@ -46,7 +42,6 @@ namespace BitTorrent
 {
     class TorrentHandle;
     class PeerInfo;
-    struct PeerAddress;
 }
 
 namespace Net
@@ -62,9 +57,7 @@ public:
     explicit PeerListWidget(PropertiesWidget *parent);
     ~PeerListWidget() override;
 
-    void loadPeers(BitTorrent::TorrentHandle *const torrent, bool forceHostnameResolution = false);
-    QStandardItem *addPeer(const QString &ip, BitTorrent::TorrentHandle *const torrent, const BitTorrent::PeerInfo &peer);
-    void updatePeer(const QString &ip, BitTorrent::TorrentHandle *const torrent, const BitTorrent::PeerInfo &peer);
+    void loadPeers(const BitTorrent::TorrentHandle *torrent);
     void updatePeerHostNameResolutionState();
     void updatePeerCountryResolutionState();
     void clear();
@@ -80,16 +73,15 @@ private slots:
     void handleResolved(const QString &ip, const QString &hostname);
 
 private:
+    void updatePeer(const QString &ip, const BitTorrent::TorrentHandle *torrent, const BitTorrent::PeerInfo &peer, bool &isNewPeer);
+
     void wheelEvent(QWheelEvent *event) override;
 
     QStandardItemModel *m_listModel;
-    PeerListDelegate *m_listDelegate;
     PeerListSortModel *m_proxyModel;
-    QHash<QString, QStandardItem *> m_peerItems;
-    QHash<QString, BitTorrent::PeerAddress> m_peerAddresses;
-    QSet<QString> m_missingFlags;
-    QPointer<Net::ReverseResolution> m_resolver;
     PropertiesWidget *m_properties;
+    Net::ReverseResolution *m_resolver;
+    QHash<QString, QStandardItem *> m_peerItems;
     bool m_resolveCountries;
 };