flood icon indicating copy to clipboard operation
flood copied to clipboard

server: qBittorrent: Update the status filter behavior

Open slikie opened this issue 4 years ago • 10 comments

server: qBittorrent: Update the status filter behavior so that UX is in sync with qB GUI/WebUI.

Description

Mainly appends 'pausedDL' torrents to 'Downloading' status.

I understand rtorrent/rutorrent may have the design of classifying torrent status, and I'm not sure if you want to unify the logic for the current three backends, but this PR would be more natural logically for qBittorrent users.

Adjustment based on: https://github.com/CzBiX/qb-web/blob/809d5bab4b5b4566cd3d74eb6e5df243fb4a17c1/src/utils.ts#L4

Types of changes

  • [ ] Breaking change (changes that break backward compatibility of public API or CLI - semver MAJOR)
  • [ ] New feature (non-breaking change which adds functionality - semver MINOR)
  • [x] Bug fix (non-breaking change which fixes an issue - semver PATCH)

slikie avatar Mar 10 '21 18:03 slikie

Codecov Report

Merging #237 (ecb300f) into master (02aa24e) will decrease coverage by 0.16%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
- Coverage   78.79%   78.63%   -0.17%     
==========================================
  Files          57       57              
  Lines        9211     9212       +1     
  Branches      917      908       -9     
==========================================
- Hits         7258     7244      -14     
- Misses       1945     1957      +12     
- Partials        8       11       +3     
Impacted Files Coverage Δ
...services/qBittorrent/util/torrentPropertiesUtil.ts 60.63% <100.00%> (+0.42%) :arrow_up:
server/config/passport.ts 85.36% <0.00%> (-4.88%) :arrow_down:
server/models/Users.ts 91.17% <0.00%> (-2.95%) :arrow_down:
server/services/interfaces/clientGatewayService.ts 97.70% <0.00%> (-1.15%) :arrow_down:
server/services/index.ts 95.62% <0.00%> (-0.55%) :arrow_down:
...rver/services/Transmission/clientRequestManager.ts 74.44% <0.00%> (-0.32%) :arrow_down:
server/services/rTorrent/clientGatewayService.ts 70.50% <0.00%> (-0.24%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 02aa24e...ecb300f. Read the comment docs.

codecov[bot] avatar Mar 10 '21 18:03 codecov[bot]

None of the changes is effective. Note that switch statements fallthroughs till break and a torrent can’t be downloading and stopped at the same time.

Additionally, rTorrent and ruTorrent are two distinct pieces of software. It is inappropriate to use rTorrent/ruTorrent. Flood connects to rTorrent only, and it doesn’t depend on ruTorrent.

jesec avatar Mar 11 '21 01:03 jesec

The main reason for this PR is that there is currently no apparent way to filter pausedDL torrents for continuing in the current category. I understand that it would be confusing that a torrent is both in downloading and stopped. Do you think it's an idea to rename stopped to paused like qB?

I only mention ruTorrent because it was de-facto the main rTorrent webui before Flood so maybe there is some experience or design to inherit. Disregard if you don't care

slikie avatar Mar 11 '21 05:03 slikie

The main reason for this PR is that there is currently no apparent way to filter pausedDL torrents for continuing in the current category. I understand that it would be confusing that a torrent is both in downloading and stopped. Do you think it's an idea to rename stopped to paused like qB?

I don't think that's necessary. IMO there is zero reason to make "stopped" distinct from "paused". It only creates more confusion.

jesec avatar Mar 13 '21 15:03 jesec

@slikie

Hi, are you still championing this PR?

If yes, you would have to at least make it actually effective. (and respond to my comments)

if no, feel free to close the PR.

jesec avatar Mar 20 '21 16:03 jesec

and a torrent can’t be downloading and stopped at the same time.

I don't think there's any code logic present against that.

None of the changes is effective. Note that switch statements fallthroughs till break

I tested it and it is effective. One torrent is only at a state at the time, it won't need to go past break.

I'm still arguing that the current UX doesn't provide a easy way to resume previously paused torrents that aren't finished downloading, which is a necessary function.

There are a lot of circumstances people need to pause their torrents and resume afterwards, and there isn't an apparent queue in flood right now to address that.

Right now I will need to go stopped status, sort by percent complete, and by no way I can tell if one torrent is finished downloading or stuck at 99%.

Hope that address your confusion.

There are a few ways for this:

  • Transmission/Deluge: add paused and refactor stopped into completed.
  • rutorrent: add pausedDL into Downloading, as I proposed. while there is no stopped category.
  • qBittorrent: add pausedDL into Downloading and refactor stopped into Paused.

slikie avatar Mar 20 '21 17:03 slikie

You don’t need to change any backend code for this.

What you want is a filter that contains “stopped” but not “completed”.

jesec avatar Mar 20 '21 17:03 jesec

You don’t need to change any backend code for this.

What you want is a filter that contains “stopped” but not “completed”.

This.

Current change is unnecessary and simply won’t work. A new paused filter will NOT magically appear.

jesec avatar Mar 21 '21 08:03 jesec

IMO there is zero reason to make "stopped" distinct from "paused". It only creates more confusion.

I was kind of worried about this but I take this as a go-ahead?

slikie avatar Mar 23 '21 07:03 slikie

The commit changes the public API, and as a result, unfortunately, is a breaking change. Per versioning conventions, I can’t do this without a major release.

And, as i said, backend change is totally unnecessary. You can achieve this purely in frontend by filtering “stopped” and “NOT complete”.

It is an option to add a “Paused” filter that does that in the frontend. However, I would prefer a more general method like “Advanced Filtering” (see Flood-UI/flood#312, though, I can implement this myself in the future).

IMO there is zero reason to make "stopped" distinct from "paused". It only creates more confusion.

I was kind of worried about this but I take this as a go-ahead?

No. I don’t mean that we should replace “stopped” with “paused”. I am just not convinced that a new “paused” status would be useful.

jesec avatar Mar 23 '21 07:03 jesec