WebUI Add Limit Share Ratio context menu option #8598

Piccirello · 2018-03-14T07:04:42Z

GUI:

WebUI:

Chocobo1 · 2018-03-14T11:52:25Z

src/webui/api/torrentscontroller.cpp

+ ratioLimit = -1;
+
+ qlonglong seedingTimeLimit = params()["seedingTimeLimit"].toLongLong();
+ if (seedingTimeLimit == 0)


use <=?
And also in above instance.

Why do you drop zero?

FYI, setRatioLimit/setSeedingTimeLimit checks its params for incorrect values.

I was trying to handle the case where values of 0 should be treated as no limit. But the GUI doesn't seem to exhibit this behavior, so removing this code.

Actually, it does do this in UpDownRatioDlg::ratio(). But this is WebUI-implementation specific so I'm now handling this in shareratio.html's js logic.

Chocobo1 · 2018-03-14T11:54:49Z

src/webui/www/private/scripts/mocha-init.js

+ if (hashes.length === 1) {
+ var hash = hashes[0];
+ var row = torrentsTable.rows[hash].full_data;
+ console.log(row);


Is this intended?

Don't commit code in the early hours of the morning. Removing this.

Chocobo1 · 2018-03-14T11:55:10Z

src/webui/www/private/scripts/mocha-init.js

+ height: 175
+ });
+ }
+


I will remove this empty line.

Chocobo1 · 2018-03-14T11:56:13Z

src/webui/www/private/scripts/mocha-init.js

@@ -163,6 +162,33 @@ initializeWindows = function() {
 }
 };

+ shareRatioFN = function() {
+ console.log("Limit share ratio...");


Chocobo1 · 2018-03-14T11:56:51Z

src/webui/www/private/scripts/mocha-init.js

@@ -387,13 +412,12 @@ initializeWindows = function() {
 if (hashes.length == 1) {
 var hash = hashes[0];
 var row = torrentsTable.rows[hash];
- if (row) {
- var name = row.full_data.name;
+ if (row)


Please keep the curly braces, better safe than sorry.

This conditional's code block doesn't seem complex enough to warrant curly braces.

Chocobo1 · 2018-03-14T12:00:04Z

src/webui/www/private/shareratio.html

+ };
+
+ // select current values
+ if (hashesList.length === 1) {


how about if (hashesList.length !== 1) return;? and you save some indention.

I've replaced this code.

Chocobo1 · 2018-03-14T12:01:35Z

src/webui/www/private/shareratio.html

+ if ($('setMinutes').get('checked'))
+ seedingTimeLimitValue = $('minutes').get('value');
+ }
+ else return false;


I would add braces here to be coherent.

Apparently, it is not accepted to write consistent code in web development, as well as adhere to any other rules... Nothing personal, it follows from all my observations of the web code.

Chocobo1 · 2018-03-14T12:02:25Z

src/webui/www/private/shareratio.html

+ function getSelectedRadioValue(name) {
+ var radios = document.getElementsByName(name);
+
+ for (var i = 0; i < radios.length; i++) {


++i and instances below

In this context the results are identical, and i++ is a lot more common in JavaScript. We have 39 instances of i++ in our js codebase, but only 1 instance of ++i.

I'm merely pointing out the coding style, until the coding guidelines extends to JS, I can't/won't force you to change it.

In this context the results are identical, and i++ is a lot more common in JavaScript.

Of course and I often wonder why that is the case in JS world given ++i, i++ have the same counterparts in C++...

Chocobo1 · 2018-03-14T12:08:41Z

src/webui/www/private/scripts/misc.js

@@ -76,6 +76,11 @@ function friendlyPercentage(value) {
 return percentage.toFixed(1) + "%";
 }

+function friendlyFloat(value, precision) {
+ var mult = Math.pow(10, precision);
+ return Math.round(value * mult) / mult;


I fell over this... why multiply then divide?
can't you do return Math.round(value).toFixed(2)?

This is so that we can round to a specific digit. Otherwise Math.round(value) will round to the nearest int, with toFixed(2) adding two zeros.

Thanks for explaining it.
AFAIK javascript numbers are always float type, then this function can be just return value.toFixed(precision); right?

AFAIK javascript numbers are always float type, then this function can be just return value.toFixed(precision); right?

You're right. That's a simpler solution too.

Chocobo1 · 2018-03-14T12:13:22Z

src/webui/www/private/index.html

@@ -120,6 +120,7 @@ <h1 class="applicationTitle">qBittorrent Web User Interface <span class="version
 </li>
 <li class="separator"><a href="#DownloadLimit"><img src="theme/kt-set-max-download-speed" alt="QBT_TR(Limit download rate...)QBT_TR[CONTEXT=TransferListWidget]"/> QBT_TR(Limit download rate...)QBT_TR[CONTEXT=TransferListWidget]</a></li>
 <li><a href="#UploadLimit"><img src="theme/kt-set-max-upload-speed" alt="QBT_TR(Limit upload rate...)QBT_TR[CONTEXT=TransferListWidget]"/> QBT_TR(Limit upload rate...)QBT_TR[CONTEXT=TransferListWidget]</a></li>
+ <li><a href="#ShareRatio"><img src="theme/ratio" alt="QBT_TR(Limit share ratio...)QBT_TR[CONTEXT=TransferListWidget]"/> QBT_TR(Limit share ratio...)QBT_TR[CONTEXT=TransferListWidget]</a></li>


for the img src, there is already one at icons/skin/ratio.png, the webUI cannot access there?

That version is 256x256 and 5x larger than this icon.

It's "usually" better to provide large image and scale it down than scale up small image, especially when high-dpi monitor is getting common.

I meant 5x larger in terms of file size. In the WebUI this image has a static, specified height of 16px x 16px.

I meant 5x larger in terms of file size.

Surely our bottleneck isn't here... and almost all icons in the icon folder are 256x256, obviously you can see the motivation.
If you really insist adding the icon, move it to www/private/image folder.

In the WebUI this image has a static, specified height of 16px x 16px.

Would be better if we used em as unit.

Would be better if we used em as unit.

This would be a great goal for a separate PR. I don't feel comfortable switching units for this small change.

glassez · 2018-03-15T06:16:18Z

src/webui/api/torrentscontroller.cpp

@@ -622,6 +622,26 @@ void TorrentsController::setDownloadLimitAction()
 applyToTorrents(hashes, [limit](BitTorrent::TorrentHandle *torrent) { torrent->setDownloadLimit(limit); });
 }

+void TorrentsController::setShareLimitAction()


I would use setShareLimits

Will update this.

glassez · 2018-03-15T07:40:16Z

src/webui/api/torrentscontroller.cpp

+ ratioLimit = -1;
+
+ qlonglong seedingTimeLimit = params()["seedingTimeLimit"].toLongLong();
+ if (seedingTimeLimit == 0)


Why do you drop zero?

FYI, setRatioLimit/setSeedingTimeLimit checks its params for incorrect values.

Piccirello · 2018-03-19T08:23:31Z

Are per-torrent rate limits supposed to be persisted between application restarts? They appear not to be.

glassez · 2018-03-19T09:15:05Z

They appear not to be.

What makes you think that?

Piccirello · 2018-03-19T18:33:16Z

What makes you think that?

In my testing, custom ratios aren't persisted across restarts. "No limit" does appear to be persisted correctly. To reproduce:

Limit torrent's share ratio to custom ratio/minutes
Exit qBittorrent
Launch qBittorrent
Verify share ratio is set back to global

Arathen · 2018-03-20T02:20:26Z

custom ratios aren't persisted across restarts

This hasn't been my experience. I make extensive use of per-torrent share ratios (the reason I requested them for the WebUI) and if they didnt persist across restarts it would be a show stopper for me. What I have found and reported previously under #7335 is that the default global share ratio does not stick. If I set a default share ratio it gets applied to all torrents which dont already have an explicit per-torrent ratio set, but then after a reasonably short period of time (I havent timed it but its less than an hour or so) it just reverts back to Bittorrent\MaxRatio=-1 in the config file for no apparent reason that ive been able to identify.

Per-torrent share ratios are ok though. Im seeding a couple of hundred long-term torrents, all of which have per-torrent share ratios which have persisted through numerous application (and server) restarts.

Piccirello · 2018-03-20T06:45:04Z

@Arathen what version of qBittorrent are you running?

I'm experiencing this both on master and on 4.0.4. Can anyone else try to reproduce this and see if it works as expected?

Arathen · 2018-03-20T07:21:44Z

@Piccirello Im running master.

Chocobo1 · 2018-03-20T07:36:55Z

Can anyone else try to reproduce this and see if it works as expected?

It work as expected, I only tested in GUI.
Are you using webUI? or GUI?

Piccirello · 2018-03-20T19:18:01Z

Are you using webUI? or GUI?

I first noticed it when testing the WebUI feature, but verified in the GUI. When testing master I'm exclusively checking the GUI.

If anyone can confirm that the WebUI functionality works post-restart, I can just chalk it up to a local client issue.

Arathen · 2018-03-21T01:30:23Z

Confirmed. I grabbed down your PR, compiled and ran nox with it. Over the course of the last hour and a half I've successfully set per-torrent ratio limits via the WebUI and restarted the daemon half a dozen times. All per-torrent limits are preserved through the restarts just fine. I have not compiled or tested the GUI.

Thank you again for implementing this @Piccirello. It truly is a game changing feature for me :)

Piccirello · 2018-03-22T17:26:03Z

Thanks for confirming @Arathen. And glad I could get this feature implemented. With luck we can get this in 4.0.5.

Arathen · 2018-03-23T03:16:08Z

One minor issue that i've noticed with this - it doesn't seem to work on multiple torrent selections. It works great for a single torrent, but when you Control or Shift click to select multiple lines, clicking the "Save" button after setting a share ratio seems to do nothing.

Like I said, its a minor issue. The overall functionality as it is now is completely usable and far outweighs the glitch.

Piccirello · 2018-03-23T09:17:02Z

One minor issue that i've noticed with this - it doesn't seem to work on multiple torrent selections. It works great for a single torrent, but when you Control or Shift click to select multiple lines, clicking the "Save" button after setting a share ratio seems to do nothing.

I'm so glad you caught this. This issue should now be fixed with the latest commit. Your testing is much appreciated!

While working on this, I figured out my "local" issue with saving torrent share limits. Turns out that the torrents for which share limits aren't being persisted are all in an error state. Torrents in a good state (Paused, Downloading, Seeding, etc.) successfully persist share ratios across application reboots.

Chocobo1 · 2018-03-24T14:44:28Z

src/webui/www/private/shareratio.html

+<!DOCTYPE html>
+<html lang="${LANG}">
+<head>
+ <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />


PR #8621 is merged now, can you please apply the changes to here?

Updated to reflect the changes.

Chocobo1 · 2018-03-24T14:45:09Z

src/webui/www/private/shareratio.html

+ <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
+ <title>QBT_TR(Torrent Upload/Download Ratio Limiting)QBT_TR[CONTEXT=UpDownRatioDlg]</title>
+ <link rel="stylesheet" href="css/style.css" type="text/css" />
+ <script type="text/javascript" src="scripts/mootools-1.2-core-yc.js" charset="utf-8"></script>


and the script tags too.

Chocobo1 · 2018-03-24T14:46:06Z

src/webui/www/private/shareratio.html

+ <script type="text/javascript" src="scripts/mootools-1.2-more.js" charset="utf-8"></script>
+ <script type="text/javascript" src="scripts/misc.js"></script>
+ <script type="text/javascript">
+ console.log("shareratio.html");


Leftover. I've removed it.

Chocobo1 · 2018-04-03T17:03:10Z

src/webui/api/torrentscontroller.cpp

+
+ const qreal ratioLimit = params()["ratioLimit"].toDouble();
+ const qlonglong seedingTimeLimit = params()["seedingTimeLimit"].toLongLong();
+ const QStringList hashes {params()["hashes"].split('|')};


for consistency please change to: const QStringList hashes = params()["hashes"].split('|'); or you can choose using the curly braces in the above 2 lines instead.

And now its time to squash commits!

I'll turn a blind eye to the coding style comments on JS for now, mind you we also updated the coding guidelines recently (#8629 & #8640).

Chocobo1 · 2018-04-04T11:25:06Z

src/webui/www/private/shareratio.html

+ <title>QBT_TR(Torrent Upload/Download Ratio Limiting)QBT_TR[CONTEXT=UpDownRatioDlg]</title>
+ <link rel="stylesheet" href="css/style.css" type="text/css" />
+ <script src="scripts/mootools-1.2-core-yc.js"></script>
+ <script src="scripts/mootools-1.2-more.js"></script>


Since #8672 is merged, please rebase this PR and also you'll need to change this to: scripts/lib/mootools-1.2-core-yc.js, scripts/lib/mootools-1.2-more.js.

Piccirello · 2018-04-09T05:09:25Z

All commits have been squashed and rebased

Chocobo1

LGTM

Chocobo1 · 2018-04-10T07:04:15Z

@Piccirello
Due to #8622 merged, there are conflicts now...
I can help you resolve it (and also reformat it) if you don't have the time.
I tested the PR and it's OK in current form.

Piccirello · 2018-04-11T00:27:54Z

I can help you resolve it (and also reformat it) if you don't have the time.

That would be a huge help. Finding time has been tough lately, and now I’m without a computer for the next several days.

Closes qbittorrent#6815, qbittorrent#7602.

Chocobo1 · 2018-04-11T15:52:47Z

That would be a huge help. Finding time has been tough lately, and now I’m without a computer for the next several days.

Okay, its done now.

Piccirello · 2018-04-13T13:11:37Z

@Chocobo1 thanks for taking care of this

glassez

You need to change Wiki accordingly (both pages for current and new API).

Chocobo1 · 2018-04-14T16:40:03Z

@Chocobo1 thanks for taking care of this

No problem and thanks a lot for this.

Piccirello force-pushed the limit-share-ratio branch from 10c46dc to cf18fd0 Compare March 14, 2018 07:08

Chocobo1 reviewed Mar 14, 2018

View reviewed changes

src/webui/www/private/scripts/mocha-init.js Outdated

height: 175

});

}

Copy link

Member

Chocobo1 Mar 14, 2018

Choose a reason for hiding this comment

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

I will remove this empty line.

Chocobo1 reviewed Mar 14, 2018

View reviewed changes

glassez added the WebUI WebUI-related issues/changes label Mar 14, 2018

glassez reviewed Mar 15, 2018

View reviewed changes

Piccirello force-pushed the limit-share-ratio branch 2 times, most recently from 09564ab to 7768630 Compare March 19, 2018 07:55

Chocobo1 reviewed Mar 24, 2018

View reviewed changes

Chocobo1 reviewed Apr 3, 2018

View reviewed changes

Chocobo1 reviewed Apr 4, 2018

View reviewed changes

Piccirello force-pushed the limit-share-ratio branch 2 times, most recently from 0fa8746 to adb0d2b Compare April 9, 2018 05:01

Chocobo1 previously approved these changes Apr 9, 2018

View reviewed changes

FranciscoPombal mentioned this pull request Apr 9, 2018

Next version number? #8710

Closed

Arathen mentioned this pull request Apr 11, 2018

WebUI Per torrent share ratio setting #8714

Closed

Piccirello added 3 commits April 11, 2018 23:45

WebUI rename property to match its definition

3f0fbc0

Remove unused/superfluous variables

1f42ab8

WebUI Add Limit Share Ratio context menu option

9f36b54

Closes qbittorrent#6815, qbittorrent#7602.

Chocobo1 dismissed their stale review via 9f36b54 April 11, 2018 15:50

Chocobo1 force-pushed the limit-share-ratio branch from adb0d2b to 9f36b54 Compare April 11, 2018 15:50

Chocobo1 approved these changes Apr 13, 2018

View reviewed changes

glassez approved these changes Apr 14, 2018

View reviewed changes

Chocobo1 merged commit a70942e into qbittorrent:master Apr 14, 2018

Piccirello mentioned this pull request Apr 16, 2018

[Wishlist] Set share ratio per torrent in the WebUI #7602

Closed

Piccirello mentioned this pull request Aug 25, 2018

[Wishlist] Add support for setting ratio limit via WebUI API #3425

Closed

Piccirello deleted the limit-share-ratio branch September 3, 2018 06:21

Piccirello mentioned this pull request Oct 23, 2018

[Feature Request] WebUI API allow changing torrent share ratio #9033

Closed

WebUI Add Limit Share Ratio context menu option #8598

WebUI Add Limit Share Ratio context menu option #8598

Conversation

Piccirello commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piccirello commented Mar 19, 2018

glassez commented Mar 19, 2018

Piccirello commented Mar 19, 2018

Arathen commented Mar 20, 2018

Piccirello commented Mar 20, 2018

Arathen commented Mar 20, 2018

Chocobo1 commented Mar 20, 2018

Piccirello commented Mar 20, 2018

Arathen commented Mar 21, 2018

Piccirello commented Mar 22, 2018

Arathen commented Mar 23, 2018

Piccirello commented Mar 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piccirello commented Apr 9, 2018

Chocobo1 left a comment

Choose a reason for hiding this comment

Chocobo1 commented Apr 10, 2018

Piccirello commented Apr 11, 2018

Chocobo1 commented Apr 11, 2018

Piccirello commented Apr 13, 2018

glassez left a comment

Choose a reason for hiding this comment

Chocobo1 commented Apr 14, 2018

Chocobo1 Mar 19, 2018 •

edited

Loading