Auto add public trackers for magnet/torrent task #162
Conversation
adding public tracker list for each download task.
There was a problem hiding this comment.
Hello again, thanks so much for taking the time to do this!
I think your basic approach to insert the trackers at the time the task is added makes sense. The outlines of your implementation also make sense, though I've left some comments around how to make it match the surrounding code better and be more robust.
The review I just did is incomplete -- I tried to point out some changes that will probably require a bunch of moving code around, but I haven't actually tried to use this feature myself yet. Once I do, I think I'll have some more comments (mostly around how it feels to the user), but I wanted to get back to you reasonably soon.
Thanks again!
| @@ -51,6 +51,8 @@ export function onStoredStateChange(storedState: State) { | |||
| backgroundState.showNonErrorNotifications = | |||
| storedState.settings.notifications.enableFeedbackNotifications; | |||
|
|
|||
| backgroundState.torrentTrackers = updateAndGetTorrentTrackers(storedState); | |||
There was a problem hiding this comment.
I would prefer a shift of responsibility around this call that looks kind of like the notifications block, above. Something like:
- The background task state keeps a copy of the settings, rather than the file in
actions/. - When the settings changes, it's this code (and not
updateAndGetTorrentTrackers) that decides if anything needs to be done. - It passes the bare minimum of information to other methods that they need to do their jobs.
There was a problem hiding this comment.
So your are suggesting not the module itself but backgroundStates to hold cachedTrackersList? I'll try it.
There was a problem hiding this comment.
Yeah. backgroundState is the place to store anything long-lived that doesn't belong as part of the configuration of the extension. Earlier versions of the code actually used the pattern you had here, but I changed it because it got too hard to manage.
| console.debug("updateAndGetTorrentTrackers was called", new Error().stack); | ||
| console.debug("cached trackers:", cachedTrackers.length); | ||
|
|
||
| const flag = storedState.settings.torrentTrackers.enablePublicTrackers; |
There was a problem hiding this comment.
I think there's a bug around this flag.
- User enables public trackers.
- The extension fetches and holds onto the list.
- User disables public trackers.
- User adds a new torrent task.
- Public trackers are added anyway.
I think something should clear the list if the flag is unset.
| )} | ||
| style={{ flex: 1 }} | ||
| value={this.state.publicTrackerURL} | ||
| onChange={(e) => { |
There was a problem hiding this comment.
Something in this chain of behavior needs to be debounced or otherwise delayed; as currently written, I think this is going to trigger a new request to load the tracker list on every keystroke, even while the URL is only partially written.
There was a problem hiding this comment.
how about "onblur" event?
There was a problem hiding this comment.
I'm not sure how reliably onblur fires, especially if you do something like close the settings page without clicking outside of the input box. I'm imagining one of two solutions here:
- Debounce requests to the provided URL (you can use
lodash-es/debounce), which doesn't involve any changes to the UI. - Add a button to explicitly "save settings and fetch list", sort of like that which exists for the host/port/username/password configuration section. Changes to the UI required, but no clever debouncing or the like.
Having written these two down, I think the second is preferable: it's clearer about what's happening to the user, it follows existing UI patterns and it doesn't require much cleverness.
|
GREAT !!! |
|
Hi, I appreciate you opening this PR but it looks like it's gone a little stale. I'm going to close it out so I can keep better track of what is and is not active on my bug tracker. If you'd like to work on it again, let me know and I can reopen it. |
try implementing issue #137
**- add a configuration option to enable "adding public trackers"; **
There is a new conf option called "Automatically add trackers to new tasks form a trackslist URL", and also the option to specify tracker url. eg: https://raw.githubusercontent.com/ngosang/trackerslist/master/trackers_best_ip.txt
- if the option is checked, for each successfully created task, add the additional trackers from the list above. (There is tracker adding API available for download station API)
My solution is slightly different: trackers when you adding new magnet/torrents before sending to DS-API.
Appreciate for any help/comments ❤️ and thanks to @seansfkelley for such a great work.