server: qBittorrent: Update the status filter behavior #237

slikie · 2021-03-10T18:24:26Z

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)
Bug fix (non-breaking change which fixes an issue - semver PATCH)

codecov · 2021-03-10T18:26:58Z

Codecov Report

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

@@            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%)`	⬆️
server/config/passport.ts	`85.36% <0.00%> (-4.88%)`	⬇️
server/models/Users.ts	`91.17% <0.00%> (-2.95%)`	⬇️
server/services/interfaces/clientGatewayService.ts	`97.70% <0.00%> (-1.15%)`	⬇️
server/services/index.ts	`95.62% <0.00%> (-0.55%)`	⬇️
...rver/services/Transmission/clientRequestManager.ts	`74.44% <0.00%> (-0.32%)`	⬇️
server/services/rTorrent/clientGatewayService.ts	`70.50% <0.00%> (-0.24%)`	⬇️

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.

jesec · 2021-03-11T01:17:47Z

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.

slikie · 2021-03-11T05:49:33Z

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

jesec · 2021-03-13T15:32:38Z

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 · 2021-03-20T16:13:33Z

@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.

slikie · 2021-03-20T17:14:01Z

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.

jesec · 2021-03-20T17:18:50Z

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

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

jesec · 2021-03-20T17:20:52Z

server/services/qBittorrent/util/torrentPropertiesUtil.ts

    case 'downloading':
    case 'forcedDL':
      statuses.push('active');
      statuses.push('downloading');
      break;
    case 'pausedDL':
      statuses.push('inactive');
+      statuses.push('downloading');


This is the only effective line.

jesec · 2021-03-20T17:22:58Z

server/services/qBittorrent/util/torrentPropertiesUtil.ts

@@ -62,16 +62,22 @@ export const getTorrentStatusFromState = (state: QBittorrentTorrentState): Torre
      statuses.push('downloading');
      break;
    case 'metaDL':
+      statuses.push('downloading');
+      break;
    case 'downloading':
    case 'forcedDL':
      statuses.push('active');


metaDL previously goes to here. Your change only removes active from the status. Is that intended? metaDL should be an active status.

jesec · 2021-03-20T17:23:31Z

server/services/qBittorrent/util/torrentPropertiesUtil.ts

      statuses.push('stopped');
      break;
    case 'queuedDL':
+      statuses.push('inactive');
+      statuses.push('downloading');
+      break;
    case 'stalledDL':
      statuses.push('inactive');


queuedDL previously goes to here. Your change is not effective.

jesec reviewed Mar 20, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: qBittorrent: Update the status filter behavior #237

server: qBittorrent: Update the status filter behavior #237

slikie commented Mar 10, 2021

codecov bot commented Mar 10, 2021 •

edited

Loading

jesec commented Mar 11, 2021 •

edited

Loading

slikie commented Mar 11, 2021

jesec commented Mar 13, 2021

jesec commented Mar 20, 2021

slikie commented Mar 20, 2021 •

edited

Loading

jesec commented Mar 20, 2021

jesec Mar 20, 2021

jesec Mar 20, 2021

jesec Mar 20, 2021

server: qBittorrent: Update the status filter behavior #237

Are you sure you want to change the base?

server: qBittorrent: Update the status filter behavior #237

Conversation

slikie commented Mar 10, 2021

Description

Types of changes

codecov bot commented Mar 10, 2021 • edited Loading

Codecov Report

jesec commented Mar 11, 2021 • edited Loading

slikie commented Mar 11, 2021

jesec commented Mar 13, 2021

jesec commented Mar 20, 2021

slikie commented Mar 20, 2021 • edited Loading

jesec commented Mar 20, 2021

jesec Mar 20, 2021

Choose a reason for hiding this comment

jesec Mar 20, 2021

Choose a reason for hiding this comment

jesec Mar 20, 2021

Choose a reason for hiding this comment

codecov bot commented Mar 10, 2021 •

edited

Loading

jesec commented Mar 11, 2021 •

edited

Loading

slikie commented Mar 20, 2021 •

edited

Loading