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

WebUI: Show only hosts in tracker filter list #18190

Merged

Conversation

ttys3
Copy link
Contributor

@ttys3 ttys3 commented Dec 9, 2022

the trackerlist url in the web ui sidebar currently is not friendly for private trackers.

this patch will make it consistent with the gui one, and more friendly for private trackers.

webui before:

image

webui with patch:

image

update 2023-05-30:

image

@ttys3 ttys3 force-pushed the webui-trackerlist-consistent-with-gui branch from 95acaa2 to f155c79 Compare December 9, 2022 15:15
@ttys3 ttys3 changed the title chore: trackerlist url keep only host (consistent with gui) webui trackerlist url keep only host (consistent with gui) Dec 9, 2022
@thalieht
Copy link
Contributor

thalieht commented Dec 9, 2022

Number of torrents containing each tracker never stop growing, as can be seen in second pic. It happens when i open another WebUI tab while another is still active.
Also what about udp trackers?

@thalieht thalieht added the WebUI WebUI-related issues/changes label Dec 9, 2022
@terrytw
Copy link

terrytw commented Dec 10, 2022

Maybe it's better to at least keep the protocol (i.e., http, https, udp)?

@glassez glassez changed the title webui trackerlist url keep only host (consistent with gui) WebUI: Show only hosts in tracker filter list Dec 10, 2022
@glassez glassez requested review from a team December 10, 2022 08:07
@ttys3
Copy link
Contributor Author

ttys3 commented Dec 10, 2022

Maybe it's better to at least keep the protocol (i.e., http, https, udp)?

maybe we do not need to distinguish it is http or udp? if the hostname is the same ?
since only the same tracker will have the same hostname
I think users just not care. or just need to merge into one ?

also I do not see the protocol in the GUI (I only used Linux GUI version)

@ttys3
Copy link
Contributor Author

ttys3 commented Dec 10, 2022

Number of torrents containing each tracker never stop growing, as can be seen in second pic. It happens when i open another WebUI tab while another is still active. Also what about udp trackers?

That's kind of weird, but I did reproduce it too

I'll take a moment to look into why

@ttys3 ttys3 force-pushed the webui-trackerlist-consistent-with-gui branch from 1b8a9dd to 0d0f812 Compare December 10, 2022 14:57
@ttys3
Copy link
Contributor Author

ttys3 commented Dec 10, 2022

That's kind of weird.

I still not figured out the reason

the torrents list got apended again very time when there's more than two tabs open.

I added some debug code in the rendering method:

        for (const [hash, tracker] of trackerList) {
            console.log("tracker.torrents.length %o, %o", tracker.torrents.length, tracker.torrents)
            trackerFilterList.appendChild(createLink(hash, tracker.url + ' (%1)', tracker.torrents.length));
        }

shows that tracker.torrents got lots of duplicated items.

@ttys3
Copy link
Contributor Author

ttys3 commented Dec 10, 2022

the problem is related to the new added code:

merged_torrents = trackerList.get(hash).torrents.concat(torrents);

fixed with:

merged_torrents = trackerList.get(hash).torrents.concat(torrents);
// deduplicate is needed when the webui opens in multi tabs
merged_torrents = merged_torrents.filter((item, pos) => merged_torrents.indexOf(item) === pos);

but I think the filter method to deduplicate may got performance issue when there's many torrents


the reason why we need the merge here is because the web ui api returned trackers may have different url for the same tracker.

for some private trackers, it use diff url for each torrent even for the same tracker.

for example:

torrent1 hash1: `https://example.com/announce?passkey=identify_info1`
torrent2 hash2: `https://example.com/announce?passkey=identify_info2`
torrent3 hash3: `https://example.com/announce?passkey=identify_info3`

so we got the api response of trackers from the server like:

{
"trackers": {
    "https://example.com/announce?passkey=identify_info1": ["hash1"],
	"https://example.com/announce?passkey=identify_info2": ["hash2"],
	"https://example.com/announce?passkey=identify_info3": ["hash3"]
}
}

@terrytw
Copy link

terrytw commented Dec 11, 2022

Maybe it's better to at least keep the protocol (i.e., http, https, udp)?

maybe we do not need to distinguish it is http or udp? if the hostname is the same ? since only the same tracker will have the same hostname I think users just not care. or just need to merge into one ?

also I do not see the protocol in the GUI (I only used Linux GUI version)

Well there is a user case at least for knowing what protocol this tracker is using. A quick debug, if all UDP tracker is not working may lead to the natural assumption that UDP is not working for your network instead of tracker not working.

Just my 2 cents, thank you for the contribution nonetheless.

@stalkerok
Copy link
Contributor

There is no need to distinguish between protocols, in gui everything is fine with this, let it be the same here.

@terrytw terrytw mentioned this pull request Jan 23, 2023
@zotabee
Copy link

zotabee commented Jan 24, 2023

Great job! @ttys3

@glassez glassez requested review from a team and removed request for a team January 24, 2023 10:01
@thalieht
Copy link
Contributor

thalieht commented Jan 24, 2023

I don't know if it's intentional but port 443 and 80 are not appended to the tracker in the filter list while every other port is (that i know of).
Also there is a number at the end of the list (X) which i assume is the number of torrents that are not trackerless? If so it's -1 than what it should be in my case.

@Rubber-Duckie
Copy link

While in the area of code responsible for trackers in the UI, is there a possibility to simply make the tracker table widget sortable by column header. Currently it is a fixed table which is a bit odd. i.e. https://stackoverflow.com/questions/22879397/sorting-tables-with-listview

@github-actions
Copy link

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

@github-actions github-actions bot added the Stale label Mar 27, 2023
@Rubber-Duckie

This comment was marked as spam.

@github-actions github-actions bot removed the Stale label Mar 28, 2023
@terrytw
Copy link

terrytw commented Apr 16, 2023

Right now the trackers are fixed instead of sortable so I don't think it is a problem. I guess the problem is that the torrent number minus 1 issue? This PR fixes quite a significant problem with some private tracker so it would be great if we can see this merged in 4.6.

@Rubber-Duckie

This comment was marked as off-topic.

@vinanrra
Copy link

Hi, any news about this, because it's really hard to check tracker list, because right now I can't expand it to "full" size to read whole info

@tearfur
Copy link
Contributor

tearfur commented May 30, 2023

yes, check TLDs is hard in js (because we need support something like .co.uk).

So I suggest we just change the tracker filters to filter by hostname instead of domain name and do away with the hassle. Unless I am missing a point here.

@glassez @Chocobo1

@ttys3 ttys3 force-pushed the webui-trackerlist-consistent-with-gui branch from 3d4623d to 53743d5 Compare May 30, 2023 18:52
@ttys3
Copy link
Contributor Author

ttys3 commented May 30, 2023

just added some important comment in this commit: cca0d91

I almost forget why I add this code today, it took me a long time to remember why this code is required.

so I think it should be put near the code.

@glassez
Copy link
Member

glassez commented May 31, 2023

So I suggest we just change the tracker filters to filter by hostname instead of domain name and do away with the hassle.

I support that. If torrents refer to several different hosts in the same domain, then this should make sense for them, otherwise why do these domains exist at all? In any case, why should we take responsibility for this?

@ttys3
Copy link
Contributor Author

ttys3 commented May 31, 2023

So I suggest we just change the tracker filters to filter by hostname instead of domain name and do away with the hassle.

I support that. If torrents refer to several different hosts in the same domain, then this should make sense for them, otherwise why do these domains exist at all? In any case, why should we take responsibility for this?

so, we all agree to change this?

do I need to make the changes now?

another thing is, keep the full hostname, do we need including the port ? @glassez @tearfur

@terrytw
Copy link

terrytw commented May 31, 2023

I second the motion to display host name instead of domain name as well.

For port number, I think we need to figure out the level of granularity here. If we include port number, I think it is reasonable to include protocol as well. If we don't include protocol, then might as well leave out port number.

@Chocobo1
Copy link
Member

do I need to make the changes now?

No, I suggest another PR for these changes.

Map is not needed

Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>
@Chocobo1
Copy link
Member

Chocobo1 commented Jun 1, 2023

@qbittorrent/frequent-contributors
Can you take another look? Does it have blockers?

@Chocobo1 Chocobo1 added this to the 4.6.0 milestone Jun 1, 2023
@tearfur
Copy link
Contributor

tearfur commented Jun 2, 2023

another thing is, keep the full hostname, do we need including the port ?

My hunch is that not keeping the protocol and port number is better. I've never seen completely unrelated trackers exist under the same hostname, so it is probably a better idea to keep the text labels short.

Started a new pull request at #19062 to follow up on this.

@Chocobo1
Copy link
Member

Chocobo1 commented Jun 2, 2023

I'm going to merge this, in order to unblock follow up improvements.

@Chocobo1 Chocobo1 merged commit b1492bc into qbittorrent:master Jun 2, 2023
@Chocobo1
Copy link
Member

Chocobo1 commented Jun 2, 2023

Big thanks to @ttys3 and every participants here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebUI WebUI-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants