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

Handle .txt URL in tracker list #20115

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

JackHack96
Copy link

This commit adds the ability to parse .txt URL in the automatic tracker add list, as requested by #6865

@@ -73,6 +73,9 @@
#include <QThreadPool>
#include <QTimer>
#include <QUuid>
#include <QNetworkAccessManager>
Copy link
Member

Choose a reason for hiding this comment

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

QNetworkAccessManager isn't intended to be used directly in qBittorrent. Net::DownloadManager component should be used instead.

reply->deleteLater();
});

// Wait for the reply to finish
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem like a good idea.

Copy link
Author

Choose a reason for hiding this comment

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

May I ask you why do you think it's bad? I've used this technique already in other cases and it seemed to work good without memory leaks, but I'm actually not ax expert in Qt development, if there's a better way I'm curios to learn.

Copy link
Member

Choose a reason for hiding this comment

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

Creating a local event loop inside another one makes the flow of the program even more confusing, while making a false impression of its straightforwardness, which can lead to difficult-to-diagnose errors. After encountering such errors a couple of times, we preferred to get rid of such local event loops (although we never created them directly, but there are some Qt functions doing this behind the scene, e.g. Dialog::exec).

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense. Okay I'll try to refactor this code.

@glassez
Copy link
Member

glassez commented Dec 11, 2023

Personally I dislike to mix it into regular additional trackers list. IMO, it should be separated from manually specified trackers list.

@JackHack96
Copy link
Author

Personally I dislike to mix it into regular additional trackers list. IMO, it should be separated from manually specified trackers list.

Yeah I imagined that, this is the reason I was skeptical of making a pull request.
I did this because it was the most straightforward way and it could just work even with the Web UI without touching the GUI code, as I find this feature useful by myself first.

I'll try to extend the interface so that it'll have two tracker list.

@JackHack96
Copy link
Author

Today I had some free time and I tried to implement the feature by extracting it from the enhanced edition.

@stalkerok

This comment was marked as resolved.

src/base/bittorrent/sessionimpl.cpp Fixed Show fixed Hide fixed
src/gui/optionsdialog.cpp Fixed Show fixed Hide fixed
@stalkerok
Copy link
Contributor

stalkerok commented Jan 27, 2024

If you change the link and click "refresh" trackers are not refreshed the first time. But so it works well, nothing else found.
The ability to add multiple URLs is lost in this implementation.

void SessionImpl::updatePublicTracker()
{
Preferences *const pref = Preferences::instance();
Net::DownloadManager::instance()->download(Net::DownloadRequest(pref->customizeTrackersListUrl()).userAgent(QStringLiteral("qBittorrent Enhanced/" QBT_VERSION_2)), Preferences::instance()->useProxyForGeneralPurposes(), this, &SessionImpl::handlePublicTrackerTxtDownloadFinished);
Copy link

Choose a reason for hiding this comment

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

You forgot to change User-Agent :|

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@JackHack96
Copy link
Author

If you change the link and click "refresh" trackers are not refreshed the first time. But so it works well, nothing else found. The ability to add multiple URLs is lost in this implementation.

Yep I noticed that too, and I can reproduce the same bug with the Enhanced Edition. I'll try to investigate it.

@luzpaz
Copy link
Contributor

luzpaz commented Mar 20, 2024

@JackHack96 any progress? (btw, thanks for working on this!)

@JackHack96
Copy link
Author

@JackHack96 any progress? (btw, thanks for working on this!)

Unfortunately no, I tried to debug the refresh button problem some weeks ago but without any results.

This commit adds the ability to parse .txt URL in the automatic tracker add
list, as requested by qbittorrent#6865
This commit adds an option for automatic tracker population and update, taken
from qBittorrent Enhanced Edition.
This commit fulfill the request qbittorrent#6865.
This commit fix the User Agent used when setting the public tracker URL.
It also simplifies two switch cases as indicated by the GitHub code
scanner bot.
This commit fix the behaviour of the fetch button for automatic tracker
population.
@JackHack96
Copy link
Author

I've managed to fix the behavior of the fetch button, so now the patch should be ready.

Though I suggest at this point to change the name of the pull request to something like: "Handle automatic tracker fetch from URL", to suggest the fact that it only supports one URL at a time.

@stalkerok

This comment was marked as resolved.

@stalkerok
Copy link
Contributor

stalkerok commented Apr 8, 2024

Was testing an older version by mistake, no problems now.
Although, no, I was in a hurry...
The button works fine, but now when you open the settings again, the trackers from the old list are shown.

2024-04-08.20-12-25.mp4

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

Successfully merging this pull request may close these issues.

None yet

5 participants