[1015] improve trackers tab

[DjLegolas]

In here I'll replace the Trackers tab to a more informative as can be found in other clients.

closes: https://dev.deluge-torrent.org/ticket/1015

[DjLegolas]

So a little update: I have managed to create the tab in both GTK and Web UIs. Both with the same information, which is:

1. Tracker URL
2. Status
3. Number of peers in that tracker
4. tracker message(?)

Need to figure out how to do it for DHT, PeX and LSD...

GTK:

Web:

[doadin]

@DjLegolas Just some thoughts, seeds added? :) Also maybe double click option for opening edit trackers.

As far as dht pex lsd goes, seems we need to track it ourselves, libtorrent doesn't provide it. https://github.com/arvidn/libtorrent/blob/RC_2_0/include/libtorrent/peer_info.hpp#L219 static constexpr peer_source_flags_t tracker static constexpr peer_source_flags_t dht static constexpr peer_source_flags_t pex static constexpr peer_source_flags_t lsd Seems we can use dht_outgoing_get_peers_alert to run an update, to count, per info_hash. https://github.com/arvidn/libtorrent/blob/RC_2_0/include/libtorrent/alert_types.hpp#L2307

for lsd, lsd_peer_alert lsd https://github.com/arvidn/libtorrent/blob/RC_2_0/include/libtorrent/alert_types.hpp#L1914

flags? session status example https://github.com/deluge-torrent/deluge/blob/8ff4683780921111f26fe051e0274aac8afe8bf3/deluge/core/torrent.py#L1076 https://github.com/deluge-torrent/deluge/blob/8ff4683780921111f26fe051e0274aac8afe8bf3/deluge/ui/gtk3/peers_tab.py#L67 session = component.get('Core').session.status() peerinfo = self.session.get_peer_info()

or more accurate get_peer_info() from torrent_handle. https://libtorrent.org/reference-Torrent_Handle.html#get-peer-info https://github.com/deluge-torrent/deluge/blob/8ff4683780921111f26fe051e0274aac8afe8bf3/deluge/core/torrent.py#L201

something like that should get peer_info which holds peer flags dht and so on. to get flags https://gist.github.com/ardhipoetra/457b35882d16289b4c50#file-lt_test-py-L59

Hopefully some of this makes sense/is helpful.

[doadin]

you can see how qbittorent handles it here: https://github.com/qbittorrent/qBittorrent/blob/0cc318664d97697d64a48955aaeb360b6f4d32ab/src/gui/properties/trackerlistwidget.cpp#L322 https://github.com/qbittorrent/qBittorrent/blob/7a910a8cc11a60bfbf294dbfb1bbf0514ba91db7/src/base/bittorrent/peerinfo.cpp#L48

Since it seems more involved I would vote what you have now as good maybe + seeds. And could add the others in another PR.

[cas--]

Why is there a second tab instead of updating existing trackers tab in GTKUI?

I am seeing changes to tracker_status type but we need to maintain version compatibility and this would be a breaking change. What I would probably suggest here is a new trackers_status status property and deprecate tracker_status. Be aware from libtorrent docs that num_peers in alert: "These are not necessarily all new peers, some of them may already be connected".

[DjLegolas]

Why is there a second tab instead of updating existing trackers tab in GTKUI?

It helped me while implementing the new one. I'll replace the original tab when I will rebase everything with better commits.

I am seeing changes to tracker_status type but we need to maintain version compatibility and this would be a breaking change. What I would probably suggest here is a new trackers_status status property and deprecate tracker_status.

Hmm, good idea. Will do.

Be aware from libtorrent docs that num_peers in alert: "These are not necessarily all new peers, some of them may already be connected".

Yeah, will probably try to do something like qBittorrent (as @doadin suggested)...

[DjLegolas]

Ok, most of the work is done. Added DHT, PeX and LSD also.

And the fail in test_get_seo_svg_with_sni is something new with the site itself (they done have an icon anymore...)

[doadin]

@DjLegolas idk if you tried a windows build yet, or I cought you mid changes or anything, but I just did and got:

Traceback (most recent call last):
  File "deluge-script.pyw", line 33, in <module>
  File "deluge\ui\ui_entry.py", line 140, in start_ui
  File "deluge\ui\gtk3\__init__.py", line 55, in start
  File "deluge\common.py", line 1353, in run_profiled
  File "deluge\ui\gtk3\__init__.py", line 49, in run
  File "deluge\ui\gtk3\gtkui.py", line 200, in __init__
  File "deluge\ui\gtk3\torrentdetails.py", line 128, in __init__
ImportError: cannot import name 'TrackersTab' from 'deluge.ui.gtk3.trackers_tab' (E:\Program Files\Deluge\deluge\ui\gtk3\trackers_tab.pyc)

https://github.com/doadin/deluge/actions/runs/1904315730

[DjLegolas]

@doadin whoops... forgot to edit some of the names when copied everything back to the original tracker_tab.py file... Thanks!

[doadin]

@DjLegolas np, just trying to help, make sure its working, ill make another run and leave another review. Thanks for doing this nice to finally have this!

[DjLegolas]

Found another issue so fixed it too. In addition, I have added back the private tracker info to the details tab.

@doadin check now please.

[doadin]

@DjLegolas ok I see where its counting connected only while seeding now. I deleted them but if you saw my other message you can ignore those. It would be my vote to have DHT data displayed the same as trackers where it doesn't matter if they are connected or not.

And not to try and bring it up a million times but seeds vs peers? deluge/core/torrent.py maybe have something like

            if peer.flags & peer.connecting or peer.flags & peer.handshake:
                continue

            if peer.source & peer.dht & peer.seed:
                ret[0]['seed']['count'] += 1
            else:
                ret[0]['peer']['count'] += 1
            if peer.source & peer.pex & peer.seed:
                ret[0]['seed']['count'] += 1
            else:
                ret[0]['peer']['count'] += 1
            if peer.source & peer.lsd & peer.seed:
                ret[0]['seed']['count'] += 1
            else:
                ret[0]['peer']['count'] += 1

edit: basically working nicely It would be my vote to have DHT data displayed the same as trackers where it doesn't matter if they are connected or not. and have a seeds & peer columns other than that seems to be working and for what its worth I approve. In any case its 100% better than what we had.

[cas--]

@DjLegolas Remember that clients have to be able to handle connecting to an older version of Deluge daemon so there is no guarantee that the new torrent status keys will be available

[cas--]

It's looking good :+1:

Some further points:

• Private tracker should be placed below Pieces in the Details tab, otherwise it leaves no space for download path
• If any of DHT, PEX or LSD are disabled just don't show them. However I don't like that they are included in the trackers list, should they not be included at the top of Peers tab instead? Can we perhaps change the scope of the this PR to just the trackers tab and leave the peers for another PR?
• For editing how about if a tracker in the list is double-clicked it opens the edit tracker dialog? This would be a simple first-step and more complete solution added later.
• What is the difference between Status and Message column?

[cas--]

This is a quick mockup of what you could do with peers count in separate PR:

[DjLegolas]

@DjLegolas Remember that clients have to be able to handle connecting to an older version of Deluge daemon so there is no guarantee that the new torrent status keys will be available

Added support for using the old status keys and showing only that one entry.

• Private tracker should be placed below Pieces in the Details tab, otherwise it leaves no space for download path

Changed.

• If any of DHT, PEX or LSD are disabled just don't show them. However I don't like that they are included in the trackers list, should they not be included at the top of Peers tab instead? Can we perhaps change the scope of the this PR to just the trackers tab and leave the peers for another PR?

I'll move all of the DHT, PEX and LSD related things to a new PR when squashing the commits into one.

• For editing how about if a tracker in the list is double-clicked it opens the edit tracker dialog? This would be a simple first-step and more complete solution added later.

Added support for double-click in trackers tab, in addition to right click.

• What is the difference between Status and Message column?

Good question. Status is the short string which shows "Announce OK" or some other error. Message suppose to be the error message for that tracker, when received after sending an announce message. But it looks as it does not being changed. In addition, it seem as I'm using the wrong one: https://libtorrent.org/reference-Trackers.html#announce-infohash But it seem that both are not being updated in python binding.

[cas--]

Message suppose to be the error message for that tracker, when received after sending an announce message. But it looks as it does not being changed.

I don't think tracker.get('message') would return anything since as you linked message is in the announce_infohash and each tracker has multiple endpoints so would need to parse tracker['endpoints'] for the message. However we already set the tracker status using that message in on_alert_tracker_error so I don't think we need to implement that just now. Of course we can look make further improvements in the future but let's keep changes simpler just now.

Again thanks for working on this :slightly_smiling_face:

[DjLegolas]

I don't think tracker.get('message') would return anything since as you linked message is in the announce_infohash and each tracker has multiple endpoints so would need to parse tracker['endpoints'] for the message. However we already set the tracker status using that message in on_alert_tracker_error so I don't think we need to implement that just now. Of course we can look make further improvements in the future but let's keep changes simpler just now.

From what I have seen, tracker['endpoints'] is not being populated with any info. This is why I wrote it is not being updated...

Again thanks for working on this 🙂

No problem🙂

[DjLegolas]

So, after almost 2 months, back to work on this. First, I have removed DHT, PEX and LSD from this MR. Will be added in a later one... Second, I have added cache for Torrent.trackers property. The issue with tracker['endpoints'] was that we are pulling the trackers info from the handler when we call set_trackers with the trackers argument being None. Now, each time there is a call for the trackers, we will use a cache and update it every 5 seconds (just like the status property).

Windows crashes because of issues with test_is_interface_name and test_is_interface tests. Lint fails because of an internal issue in black. Fixed in https://github.com/deluge-torrent/deluge/pull/382

[DjLegolas]

I hope this is the last rebase with updates. I have changed torrent.set_tracker_status method so it will also get a message param. This new param will get the error/warning message from the alert (as this method is only being called when handling alerts). In turn, it will be printed if it have any string in it but the tracker message is empty. Here is how it's going to look:

[cas--]

Just a quick note to say I had a look at this a few weeks ago and I have concerns about handling of the lt tracker data in the core, I feel it should be parsed to not expose lt data structures and only include required fields.

Let's try to understand how to correctly expose tracker data in core and built out the UIs from there

[DjLegolas]

@cas-- I have refactored the trackers property so it will return a dict with the needed data only. This way, we will both not expose the underling lt tracker data and still have it cached.

[DjLegolas]

depends on: #427

[DjLegolas]

@cas-- I have rebased onto develop branch.

In addition, I just noticed something. Part of the change is the validation of the daemon version using the new daemon_version_check_min method. This means that we need to put some hard-coded version number as parameter, in which the new feature was introduced. This is something that might be handled in a proper way. What do you think?