killemov / Shift

A minimalistic approach to maximum control of your Transmission. (Web UI)
https://forum.transmissionbt.com/viewtopic.php?f=8&t=12555
260 stars 20 forks source link

adding trackers to torrents (using FreeBSD): not working #23

Closed ghost closed 2 years ago

ghost commented 2 years ago

so I have tried the above and it's not working

some info:

any ideas what I could be doing wrong? Thanks

killemov commented 2 years ago

You did nothing wrong.

I did a debug session on version 3.00 (bb6b5a062e) and found that adding some trackers to all torrents indeed has stopped working. Transmission responds with success but nothing happens. I also tried to add these same trackers to an individual torrent and that works just like it should. For both ways the exact same single function is used. When adding trackers to all torrents an empty array is used as a parameter to indicate ALL torrents instead of an array with a single id. So the problem appears to be a change in how an empty array used to be interpreted as ALL torrents. Maybe @ckerr from the Transmission team could give some insight. I will file an issue right away.

ckerr commented 2 years ago

I believe that it's incorrect to pass an empty array [] of ids and expect Transmission to treat that as shorthand for 'all torrents'. The spec back to 2.81 (and possibly older? 2.81 is as far back as I looked) says that omitting the ids parameter is the way to apply to all torrents. It says nothing about empty arrays.

Looks like the issue may be coming from https://github.com/killemov/Shift/blob/e32e0f9883d8443aaf90be94efbdf1c84b0d8681/shift.js#L392 where [] is used as the default ids parameter if none is provided. I haven't read the code fully but maybe this should be more like https://github.com/transmission/transmission/blob/0973cfd96d29546589a6952847edec26c59389f7/web/src/remote.js#L129 where ids is left out entirely unless provided by the caller.

killemov commented 2 years ago

@ckerr Thank you for going the extra mile with checking my code. I already knew where the problem is and how it could be fixed. I implemented it this way a long time ago and tested it. Since then I try to keep Shift backwards compatible. I will fix the problem by either removing the ids parameter in case of an empty array or just declare it undefined. My very small concern is that old versions of Transmission won't understand the missing ids parameter or the undefined value.

killemov commented 2 years ago

@mrjayviper Looking at the changes for the RPC-spec for 4.0 this functionality may become deprecated in the near future. This because the field trackerAdd will become deprecated. @ckerr Maybe NOT deprecate the trackerAdd field?

killemov commented 2 years ago

@ckerr Just a thought after committing this. Maybe introduce an explicit "all" string ( ids: "all" ) to prevent possible damaging effects.

ckerr commented 2 years ago

@ckerr Just a thought after committing this. Maybe introduce an explicit "all" string ( ids: "all" ) to prevent possible damaging effects.

I'd be happy to review a PR that adds that, if you want to write it. Sounds like a nice API quality improvement.

killemov commented 2 years ago

I'd be happy to review a PR that adds that, if you want to write it. Sounds like a nice API quality improvement.

Sure does. But it's highly unlikely there will ever come a PR from me. I have a commitment to keep Shift the best it can be and that's it. And pitching some ideas when the opportunity arises. You do recall why the /upload endpoint could be removed right? (Yes, browsers became better but still ...)

@ckerr Maybe NOT deprecate the trackerAdd field?

IF I would want to keep the "Add trackers to all torrents" functionality alive after trackerAdd is removed then I'd have to:

How will copies of trackers be dealt with? (client-side?)

Maybe add an extra parameter to indicate the sent trackerList should be appended to the existing trackerList instead of replacing it. Or make it a separate method altogether.

ckerr commented 2 years ago

That's a good use case for trackerAdd. Please open an issue for de-deprecating it