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.

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

[1015] improve trackers tab #379

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

DjLegolas
Copy link
Contributor

@DjLegolas DjLegolas commented Feb 21, 2022

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
Copy link
Contributor Author

DjLegolas commented Feb 21, 2022

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:
gtk

Web:
WEB

@doadin
Copy link
Contributor

doadin commented Feb 22, 2022

@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

self.status = self.handle.status()

self.peers = {}

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

handle: Holds the libtorrent torrent handle

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
Copy link
Contributor

doadin commented Feb 22, 2022

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--
Copy link
Member

cas-- commented Feb 23, 2022

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
Copy link
Contributor Author

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 DjLegolas force-pushed the 1015/improve-trackers-tab branch 3 times, most recently from 7dd35b3 to 86f65bd Compare February 26, 2022 11:37
@DjLegolas DjLegolas marked this pull request as ready for review February 26, 2022 11:40
@DjLegolas
Copy link
Contributor Author

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
Copy link
Contributor

doadin commented Feb 26, 2022

@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
Copy link
Contributor Author

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

@doadin
Copy link
Contributor

doadin commented Feb 26, 2022

@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
Copy link
Contributor Author

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
Copy link
Contributor

doadin commented Feb 27, 2022

@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--
Copy link
Member

cas-- commented Mar 2, 2022

@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--
Copy link
Member

cas-- commented Mar 2, 2022

It's looking good 👍

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--
Copy link
Member

cas-- commented Mar 2, 2022

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

image

@DjLegolas
Copy link
Contributor Author

@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--
Copy link
Member

cas-- commented Mar 8, 2022

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 🙂

@DjLegolas
Copy link
Contributor Author

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
Copy link
Contributor Author

DjLegolas commented Apr 30, 2022

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 #382

@DjLegolas DjLegolas force-pushed the 1015/improve-trackers-tab branch 3 times, most recently from 78379ed to ceb2ab5 Compare June 30, 2022 15:49
@DjLegolas
Copy link
Contributor Author

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:
image

@cas--
Copy link
Member

cas-- commented Mar 7, 2023

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

depends on: #427

The current way the trackers are being update is either by adding a new
torrent to the session (on startup or new one), or by changing the
trackers.
This leads to data not being updated in the `torrent.trackers` dict.
To solve this, a cache mechanism was added which will update the dict
on changes or every 5 seconds.
This will help us also get an update regarding `lt.announce_endpoint` in
 each of the trackers.
As part of the changes needed for a more informative trackers tab in the
UIs, we need the info for each of the trackers in the torrent.
For this, new status keys were added, and `tracker_status` is now
considered deprecated.

For tracker info, the keys are:
`trackers_status` - a dict of tracker_url->{status, message}
`trackers_peers` - a dict of tracker_url->number of peers in the tracker

`trackers_status` will contain the status of the torrent and also the
message from the relevant alert.
First, added trackers tab to the WebUI.
Second, now we can view all the trackers and view each:
 * status
 * peers count
 * additional message
Third, moved the private torrent info to the details tab.

closes: https://dev.deluge-torrent.org/ticket/1015
@DjLegolas
Copy link
Contributor Author

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants