Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Show downloading files in peers list. Attempt #2 #4867

Merged
merged 2 commits into from
Mar 14, 2016
Merged

Show downloading files in peers list. Attempt #2 #4867

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b24bdcb
Revert "Temporarily revert PR #2885 (filename column in peers view)."
zeule Feb 27, 2016
8baa144
Use correct piece size while calling torrent_info::map_block()
zeule Feb 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/base/bittorrent/peerinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,3 +408,8 @@ QString PeerInfo::flagsDescription() const
{
return m_flagsDescription;
}

int PeerInfo::downloadingPieceIndex() const
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning to our discussion about the right design...
Instead to create a logical for external code QStringList PeerInfo::downloadingFiles() method, you put in the interface int PeerInfo::downloadingPieceIndex() method which nobody wants and which in this case can be attributed to the implementation details (let me remind you again that our classes are not wrappers around the libtorrent classes). Because of this mistake you move part of the logic in the calling code:

torrent->info().filesForPiece(peer.downloadingPieceIndex())

, although it should contain only following:

peer.downloadingFiles()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree and will change the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, not everything looks good after some thinking. Here is the problem:

TorrentInfo::filesForPiece() is good function (and I need it for another similar feature, highlighting of the downloading files and listing them in the tooltip for the torrent progress bars, not submitted yet). Thus should we implement PeerInfo::downloadingFiles() we either need to pass a reference to TorrentInfo instance there (currently it has libtorrent::peer_info or to duplicate code ( 👎 ). At the same time the current solution seems to be normalized (in terms of relational DB normalization). I guess the libtorrent authors arrived to the current design not unintentionally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thus should we implement PeerInfo::downloadingFiles() we either need to pass a reference to TorrentInfo instance there (currently it has libtorrent::peer_info or to duplicate code ( 👎 ).

In this case we should store TorrentHandle pointer inside PerrInfo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TorrentInfo::filesForPiece() is good function (and I need it for another similar feature, highlighting of the downloading files and listing them in the tooltip for the torrent progress bars, not submitted yet).

PeerInfo::downloadingFiles() is not enough for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the main difference between the TorrentHandle and TorrentInfo classes from your point of view? Could you tell me, please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the main difference between the TorrentHandle and TorrentInfo classes from your point of view? Could you tell me, please?

What does that have to this issue? And what about my point of view? They do not depend on it.
TorrentInfo is some static torrent data (metadata and other such things are usually stored in the torrent file). TorrentHandle is a dynamic torrent data (i.e. its current downloading/seeding state etc.) and provides a management interface. TorrentHandle also includes static torrent information and provides some wrappers to it for convenience.

But why do you ask that? I just wish you hadn't used low-level features in the GUI code.
This is reminiscent of the following situation (I like to make Parallels with real life):
You come in the cafe and want to eat, say, a cheeseburger. But the waiter brings you all the ingredients separately and you have to cook himself a cheeseburger.
Kitchen should prepare, and the client has to get his cheeseburger.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the answers!

This has to do with the second use case. The use case of the progress bar. There I show piece contents and file extension over pieces in the torrent progress bars. There are no peers in that case. There might be no downloading process either. I need just static information about the torrent layout. Needed functions come from the torrent_info class and I added an utility function to the TorrentInfo class.

From the answer you gave me I see now that this is in agreement with your view too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Let's postpone this question for now. As I said, it's hard for me to discuss something I have not seen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is: #4936

{
return m_nativeInfo.downloading_piece_index;
}
1 change: 1 addition & 0 deletions src/base/bittorrent/peerinfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ namespace BitTorrent
#ifndef DISABLE_COUNTRIES_RESOLUTION
QString country() const;
#endif
int downloadingPieceIndex() const;

private:
void calcRelevance(const TorrentHandle *torrent);
Expand Down
14 changes: 14 additions & 0 deletions src/base/bittorrent/torrentinfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,20 @@ QByteArray TorrentInfo::metadata() const
return QByteArray(m_nativeInfo->metadata().get(), m_nativeInfo->metadata_size());
}

QStringList TorrentInfo::filesForPiece(int pieceIndex) const
{
if (pieceIndex < 0)
return QStringList();

std::vector<libtorrent::file_slice> files(
nativeInfo()->map_block(pieceIndex, 0, nativeInfo()->piece_size(pieceIndex)));
QStringList res;
for (const libtorrent::file_slice& s: files) {
res.append(filePath(s.file_index));
}
return res;
}

void TorrentInfo::renameFile(uint index, const QString &newPath)
{
if (!isValid()) return;
Expand Down
1 change: 1 addition & 0 deletions src/base/bittorrent/torrentinfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ namespace BitTorrent
QList<TrackerEntry> trackers() const;
QList<QUrl> urlSeeds() const;
QByteArray metadata() const;
QStringList filesForPiece(int pieceIndex) const;

void renameFile(uint index, const QString &newPath);

Expand Down
2 changes: 1 addition & 1 deletion src/gui/properties/peerlistdelegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ class PeerListDelegate: public QItemDelegate {

public:
enum PeerListColumns {COUNTRY, IP, PORT, CONNECTION, FLAGS, CLIENT, PROGRESS, DOWN_SPEED, UP_SPEED,
TOT_DOWN, TOT_UP, RELEVANCE, IP_HIDDEN, COL_COUNT};
TOT_DOWN, TOT_UP, RELEVANCE, DOWNLOADING_PIECE, IP_HIDDEN, COL_COUNT};

public:
PeerListDelegate(QObject *parent) : QItemDelegate(parent) {}
Expand Down
16 changes: 12 additions & 4 deletions src/gui/properties/peerlistwidget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ PeerListWidget::PeerListWidget(PropertiesWidget *parent)
m_listModel->setHeaderData(PeerListDelegate::TOT_DOWN, Qt::Horizontal, tr("Downloaded", "i.e: total data downloaded"));
m_listModel->setHeaderData(PeerListDelegate::TOT_UP, Qt::Horizontal, tr("Uploaded", "i.e: total data uploaded"));
m_listModel->setHeaderData(PeerListDelegate::RELEVANCE, Qt::Horizontal, tr("Relevance", "i.e: How relevant this peer is to us. How many pieces it has that we don't."));
m_listModel->setHeaderData(PeerListDelegate::DOWNLOADING_PIECE, Qt::Horizontal, tr("Files", "i.e. files that are being downloaded right now"));
// Proxy model to support sorting without actually altering the underlying model
m_proxyModel = new PeerListSortModel();
m_proxyModel->setDynamicSortFilter(true);
Expand Down Expand Up @@ -294,14 +295,14 @@ void PeerListWidget::loadPeers(BitTorrent::TorrentHandle *const torrent, bool fo
QString peerIp = addr.ip.toString();
if (m_peerItems.contains(peerIp)) {
// Update existing peer
updatePeer(peerIp, peer);
updatePeer(peerIp, torrent, peer);
oldeersSet.remove(peerIp);
if (forceHostnameResolution && m_resolver)
m_resolver->resolve(peerIp);
}
else {
// Add new peer
m_peerItems[peerIp] = addPeer(peerIp, peer);
m_peerItems[peerIp] = addPeer(peerIp, torrent, peer);
m_peerAddresses[peerIp] = addr;
// Resolve peer host name is asked
if (m_resolver)
Expand All @@ -319,7 +320,7 @@ void PeerListWidget::loadPeers(BitTorrent::TorrentHandle *const torrent, bool fo
}
}

QStandardItem* PeerListWidget::addPeer(const QString &ip, const BitTorrent::PeerInfo &peer)
QStandardItem* PeerListWidget::addPeer(const QString& ip, BitTorrent::TorrentHandle *const torrent, const BitTorrent::PeerInfo &peer)
{
int row = m_listModel->rowCount();
// Adding Peer to peer list
Expand Down Expand Up @@ -349,10 +350,14 @@ QStandardItem* PeerListWidget::addPeer(const QString &ip, const BitTorrent::Peer
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);

return m_listModel->item(row, PeerListDelegate::IP);
}

void PeerListWidget::updatePeer(const QString &ip, const BitTorrent::PeerInfo &peer)
void PeerListWidget::updatePeer(const QString &ip, BitTorrent::TorrentHandle *const torrent, const BitTorrent::PeerInfo &peer)
{
QStandardItem *item = m_peerItems.value(ip);
int row = item->row();
Expand All @@ -376,6 +381,9 @@ void PeerListWidget::updatePeer(const QString &ip, const BitTorrent::PeerInfo &p
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)
Expand Down
4 changes: 2 additions & 2 deletions src/gui/properties/peerlistwidget.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ class PeerListWidget: public QTreeView
~PeerListWidget();

void loadPeers(BitTorrent::TorrentHandle *const torrent, bool forceHostnameResolution = false);
QStandardItem *addPeer(const QString &ip, const BitTorrent::PeerInfo &peer);
void updatePeer(const QString &ip, const BitTorrent::PeerInfo &peer);
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 updatePeerHostNameResolutionState();
void updatePeerCountryResolutionState();
void clear();
Expand Down