Show downloading files in peers list. Attempt #2 #4867
Conversation
| @@ -408,3 +408,8 @@ QString PeerInfo::flagsDescription() const | |||
| { | |||
| return m_flagsDescription; | |||
| } | |||
|
|
|||
| int PeerInfo::downloadingPieceIndex() const | |||
There was a problem hiding this comment.
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()There was a problem hiding this comment.
Agree and will change the code.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
What is the main difference between the TorrentHandle and TorrentInfo classes from your point of view? Could you tell me, please?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK. Let's postpone this question for now. As I said, it's hard for me to discuss something I have not seen.
|
Leaving a small comment here without looking the above discussion. This goes hand in hand with #4767 |
|
#4767 is now merged. Should I merge this too? |
|
@sledgehammer999, I believe the problem that has been leading to a crash was fixed, so, yes, please merge this one. |
|
Just checked with Qt5 build: the "Files" column can be hid and shown back. |
|
Yeah, all are automatic with new columns being added. |
Show downloading files in peers list. Attempt #2
|
@sledgehammer999 your behavior causes my concern. This PR had the unsolved problem. However, it refers not to the user-visible functionality but to code design. Have we stopped to pay attention to this? |
This reverts reversion and fixes torrent_info::map_block() call, providing the correct piece size to it. Hopefully now it will not lead to crashes like in issue #4597