From aeabd2d6255db3f39a955be1869dd3d8c0aee710 Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Fri, 23 Aug 2019 17:04:55 +0800 Subject: [PATCH 1/2] Clean up PeerListWidget class --- src/gui/properties/peerlistwidget.cpp | 208 +++++++++++--------------- src/gui/properties/peerlistwidget.h | 18 +-- 2 files changed, 90 insertions(+), 136 deletions(-) diff --git a/src/gui/properties/peerlistwidget.cpp b/src/gui/properties/peerlistwidget.cpp index 28aa4fc09..47a37d104 100644 --- a/src/gui/properties/peerlistwidget.cpp +++ b/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 peers = torrent->peers(); - QSet oldPeersSet = m_peerItems.keys().toSet(); + QSet 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) diff --git a/src/gui/properties/peerlistwidget.h b/src/gui/properties/peerlistwidget.h index 747b95d22..69d8f11fb 100644 --- a/src/gui/properties/peerlistwidget.h +++ b/src/gui/properties/peerlistwidget.h @@ -30,15 +30,11 @@ #define PEERLISTWIDGET_H #include -#include -#include #include -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 m_peerItems; - QHash m_peerAddresses; - QSet m_missingFlags; - QPointer m_resolver; PropertiesWidget *m_properties; + Net::ReverseResolution *m_resolver; + QHash m_peerItems; bool m_resolveCountries; }; From 0891cd4878a53bfcb59d08f8740f5cc4a2c91935 Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Mon, 26 Aug 2019 01:21:36 +0800 Subject: [PATCH 2/2] Avoid unecessary copying the parameter Using forwarding reference here so that we won't get unnecessary copies of the parameter passed to `slot`, for example a lambda function. --- src/base/net/downloadmanager.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/base/net/downloadmanager.h b/src/base/net/downloadmanager.h index b92ad3e95..a4fd7b2ac 100644 --- a/src/base/net/downloadmanager.h +++ b/src/base/net/downloadmanager.h @@ -119,7 +119,7 @@ namespace Net static DownloadManager *instance(); template - void download(const DownloadRequest &downloadRequest, Context context, Func slot); + void download(const DownloadRequest &downloadRequest, Context context, Func &&slot); void registerSequentialService(const ServiceID &serviceID); @@ -150,7 +150,7 @@ namespace Net }; template - void DownloadManager::download(const DownloadRequest &downloadRequest, Context context, Func slot) + void DownloadManager::download(const DownloadRequest &downloadRequest, Context context, Func &&slot) { const DownloadHandler *handler = download(downloadRequest); connect(handler, &DownloadHandler::finished, context, slot);