Set priority for multiple files in one WebAPI request #9541

Piccirello · 2018-09-19T06:38:44Z

This API was never changed during the API revamp some months back

src/webui/api/torrentscontroller.cpp

Chocobo1 · 2018-09-19T09:22:58Z

src/webui/api/torrentscontroller.cpp

-        torrent->setFilePriority(fileID, priority);
+    BitTorrent::TorrentHandle *const torrent = BitTorrent::Session::instance()->findTorrent(hash);
+    if (torrent && torrent->hasMetadata()) {
+        for (const auto &fileID : fileIDs) {


really, QString isn't much longer to type than auto.

Any benefit to using an explicit type vs auto?

I believe CODING_GUIDELINES has enough explaination: https://github.com/qbittorrent/qBittorrent/blob/master/CODING_GUIDELINES.md#9-misc

Recommendations for modern C++ are to use auto quite liberally. We get no benefit from an explicit type here.

Recommendations for modern C++ are to use auto quite liberally.

Source?
IMO you're using auto a bit too liberal. auto is more like a shortcut, not be used like var in javascript.

From Item 5: Prefer auto to explicit type declarations in Effective Modern C++ by Scott Meyers:

Using [auto] saves typing, sure, but it also prevents correctness and performance issues that can bedevil manual type declaration ... There are thus several reasons to prefer auto over explicit type declarations ... The fact of the matter is that writing types explicitly often does little more than introduce opportunities for subtle errors, either in correctness or efficiency or both.

Ah, thank you, however I don't see he mentions anything about readability, a google search gives a handful of debates about readability of using auto.

When we discussed our rules about using auto keyword, we wanted to find a balance between faster code writing and readability. So don't abuse it, please.

src/webui/api/torrentscontroller.cpp

src/webui/www/private/scripts/prop-files.js

src/webui/api/torrentscontroller.cpp

glassez · 2018-09-19T09:32:24Z

src/webui/api/torrentscontroller.cpp

-    int fileID = params()["id"].toInt();
-    int priority = params()["priority"].toInt();
-    BitTorrent::TorrentHandle *const torrent = BitTorrent::Session::instance()->findTorrent(hash);
+    const QStringList fileIDs {params()["id"].split('|')};


I would change param name, but it breaks API compatibility...

I want your thoughts: we add an optional ids param that takes priority over the id param. Or we can change the name and bump the web api version.

We shouldn't produce incompatible changes so frequently if possible.
So maybe we should have some way to keep track of these "non-urgent" changes, and then make all the changes at once when we have the required changes?

Maybe keep the list of postponed changes near the API version declaration, e.g. "When your changes require increasing of minor version you should also apply the following:" and then list of changes.
What do you say? @Chocobo1?

What do you say? @Chocobo1?

I don't think I get your idea.

However I think the commit bumping API version should also include the api code changes, so it is possible to look up by reading git log.

I don't think I get your idea.

@Chocobo1, take this case for example.
Meaning of param 'id' was changed. Now it provide list of file IDs. It should cause changing of param name but it breaks API compatibility. It's really bad idea to make incompatible changes too frequently.
We can proceed to use old name for some time and just mark it for future changing.
When we make changes that cannot be made in a compatible way, we can add all pending changes with them.

We can proceed to use old name for some time and just mark it for future changing.

I don't object the idea, however I must say from my experience, revisiting an older code after some time takes much more time for the developer and outcome is usually not good enough quality.
If @Piccirello is interested, we could try it out in this case (experiment).

however I must say from my experience, revisiting an older code after some time takes much more time for the developer and outcome is usually not good enough quality.

So I suggest to keep list with postponed changes. We can have some instructions, where, what and how it should be changed. Or we can even have commented changes inplace.

Well, I'm not insisting. I just thought we shouldn't break API too often.

I would be interested, maybe maintaining a separate branch with breaking api changes that can be merged pre-minor version bump.

maybe maintaining a separate branch with breaking api changes that can be merged pre-minor version bump.

And have opened PR for it.
Let's try something!

src/webui/api/torrentscontroller.cpp

Chocobo1 · 2018-09-23T04:57:13Z

src/webui/api/torrentscontroller.cpp

    }
+
+    for (const auto &id : fileIdsInts)


const int id : fileIdsInts

From the coding guidelines:

Declarations for which one can gather enough information about the object interface (type) from its name or the usage pattern .. or the right part of the expression nicely fit here.

Seems we can gather enough info about the type from the variable name fileIdsInts.

The variable name does not matter. This paragraph means something completely different:

auto *varName = static_cast<RSS::Feed *>(rssItemPtr);

Or:

auto *session = BitTorrent::Session::instance();

src/webui/api/torrentscontroller.cpp

glassez

@Piccirello, you forgot to fix this.

src/webui/api/torrentscontroller.cpp

Piccirello · 2018-09-27T04:21:11Z

@glassez whoops, good catch. Pushing a new commit.

src/base/bittorrent/torrenthandle.h

src/webui/api/torrentscontroller.cpp

src/gui/addnewtorrentdialog.cpp

src/gui/properties/propertieswidget.cpp

src/gui/properties/proplistdelegate.cpp

src/webui/api/torrentscontroller.cpp

src/gui/torrentcontentmodelitem.h

src/base/bittorrent/filepriority.h

src/webui/api/torrentscontroller.cpp

src/base/bittorrent/filepriority.h

glassez

@Chocobo1, feel free to merge it.

Closes qbittorrent#6259.

Closes qbittorrent#9070.

Priority select boxes would frequently go blank due to an unexpected priority value. On first load, the torrent-scoped file checkbox's state was inconsistent with the state of the torrent's files.

Chocobo1 · 2018-12-11T05:33:00Z

Thank you!

Chocobo1 reviewed Sep 19, 2018

View reviewed changes

src/webui/api/torrentscontroller.cpp Outdated Show resolved Hide resolved

Chocobo1 reviewed Sep 19, 2018

View reviewed changes

src/webui/api/torrentscontroller.cpp Show resolved Hide resolved

glassez added the WebAPI WebAPI-related issues/changes label Sep 19, 2018

Chocobo1 reviewed Sep 19, 2018

View reviewed changes

src/webui/api/torrentscontroller.cpp Outdated Show resolved Hide resolved

Chocobo1 reviewed Sep 19, 2018

View reviewed changes

src/webui/www/private/scripts/prop-files.js Outdated Show resolved Hide resolved

glassez reviewed Sep 19, 2018

View reviewed changes

Piccirello force-pushed the webui-file-priority branch from 00f64e6 to f23dad0 Compare September 22, 2018 20:13

Chocobo1 reviewed Sep 23, 2018

View reviewed changes

src/webui/api/torrentscontroller.cpp Outdated Show resolved Hide resolved

Chocobo1 reviewed Sep 23, 2018

View reviewed changes

src/webui/api/torrentscontroller.cpp Outdated Show resolved Hide resolved

Piccirello force-pushed the webui-file-priority branch 4 times, most recently from 8292784 to 995d92b Compare September 24, 2018 04:56

glassez requested changes Sep 27, 2018

View reviewed changes

src/webui/api/torrentscontroller.cpp Outdated Show resolved Hide resolved

glassez requested changes Sep 27, 2018

View reviewed changes

src/base/bittorrent/torrenthandle.h Outdated Show resolved Hide resolved

src/base/bittorrent/torrenthandle.h Outdated Show resolved Hide resolved

Piccirello force-pushed the webui-file-priority branch 4 times, most recently from d08519b to dba8b59 Compare October 22, 2018 04:12

glassez requested changes Oct 22, 2018

View reviewed changes

src/base/bittorrent/torrenthandle.h Outdated Show resolved Hide resolved

src/webui/api/torrentscontroller.cpp Outdated Show resolved Hide resolved

src/webui/api/torrentscontroller.cpp Outdated Show resolved Hide resolved

Piccirello force-pushed the webui-file-priority branch 2 times, most recently from e6d3a11 to 942dcbd Compare October 24, 2018 04:30

glassez previously requested changes Oct 25, 2018

View reviewed changes

src/gui/addnewtorrentdialog.cpp Outdated Show resolved Hide resolved

glassez reviewed Oct 25, 2018

View reviewed changes

src/gui/addnewtorrentdialog.cpp Outdated Show resolved Hide resolved

glassez reviewed Oct 25, 2018

View reviewed changes

src/gui/properties/propertieswidget.cpp Outdated Show resolved Hide resolved

glassez reviewed Oct 25, 2018

View reviewed changes

src/gui/properties/proplistdelegate.cpp Outdated Show resolved Hide resolved

glassez reviewed Oct 25, 2018

View reviewed changes

src/gui/properties/proplistdelegate.cpp Outdated Show resolved Hide resolved

Piccirello requested a review from Chocobo1 November 21, 2018 22:56

Chocobo1 reviewed Nov 24, 2018

View reviewed changes

src/webui/api/torrentscontroller.cpp Outdated Show resolved Hide resolved

Piccirello force-pushed the webui-file-priority branch from 1c80468 to 33942f2 Compare November 25, 2018 04:24

Chocobo1 previously approved these changes Nov 25, 2018

View reviewed changes

glassez reviewed Nov 25, 2018

View reviewed changes

src/gui/torrentcontentmodelitem.h Outdated Show resolved Hide resolved

Piccirello dismissed Chocobo1’s stale review via 9ff6e9c December 1, 2018 07:01

Piccirello force-pushed the webui-file-priority branch from 33942f2 to 9ff6e9c Compare December 1, 2018 07:01

glassez requested changes Dec 1, 2018

View reviewed changes

src/base/bittorrent/filepriority.h Outdated Show resolved Hide resolved

src/webui/api/torrentscontroller.cpp Outdated Show resolved Hide resolved

Piccirello force-pushed the webui-file-priority branch from 9ff6e9c to 9a1e415 Compare December 3, 2018 00:10

glassez previously approved these changes Dec 3, 2018

View reviewed changes

Chocobo1 reviewed Dec 3, 2018

View reviewed changes

src/webui/api/torrentscontroller.cpp Outdated Show resolved Hide resolved

Chocobo1 mentioned this pull request Dec 3, 2018

Use Javascript strict mode #9959

Merged

Piccirello dismissed glassez’s stale review via 5e4e2b4 December 7, 2018 06:21

Piccirello force-pushed the webui-file-priority branch 2 times, most recently from 5e4e2b4 to 76bb830 Compare December 8, 2018 00:02

Chocobo1 previously approved these changes Dec 8, 2018

View reviewed changes

glassez reviewed Dec 8, 2018

View reviewed changes

src/base/bittorrent/filepriority.h Outdated Show resolved Hide resolved

glassez reviewed Dec 8, 2018

View reviewed changes

AceLewis mentioned this pull request Dec 8, 2018

WebUi list of improvements that need to be made to the UI #9796

Open

61 tasks

Piccirello added 5 commits December 10, 2018 23:21

Replace prio namespace with FilePriority enum class

f27dc97

Set priority for multiple files in one WebAPI request

c5b8f62

Closes qbittorrent#6259.

Fix incorrect priority value sent from WebUI

a44ed9c

Closes qbittorrent#9070.

Fix display bugs in WebUI Files tab. Remove <IE9 support

6601516

Priority select boxes would frequently go blank due to an unexpected priority value. On first load, the torrent-scoped file checkbox's state was inconsistent with the state of the torrent's files.

Update Copyright email address

57e6254

Piccirello dismissed Chocobo1’s stale review via 57e6254 December 11, 2018 04:21

Piccirello force-pushed the webui-file-priority branch from 76bb830 to 57e6254 Compare December 11, 2018 04:21

Chocobo1 approved these changes Dec 11, 2018

View reviewed changes

Chocobo1 merged commit cf9d903 into qbittorrent:master Dec 11, 2018

Piccirello deleted the webui-file-priority branch July 15, 2019 00:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set priority for multiple files in one WebAPI request #9541

Set priority for multiple files in one WebAPI request #9541

Piccirello commented Sep 19, 2018 •

edited

Loading

Chocobo1 Sep 19, 2018

Piccirello Sep 22, 2018

Chocobo1 Sep 23, 2018

Piccirello Sep 23, 2018

Chocobo1 Sep 24, 2018

Piccirello Sep 24, 2018

Chocobo1 Sep 24, 2018

glassez Sep 24, 2018

glassez Sep 19, 2018

Piccirello Sep 22, 2018

glassez Sep 23, 2018

glassez Sep 23, 2018

Chocobo1 Sep 24, 2018

glassez Sep 24, 2018

Chocobo1 Sep 25, 2018

glassez Sep 25, 2018

Piccirello Sep 27, 2018

glassez Sep 27, 2018

Chocobo1 Sep 23, 2018

Piccirello Sep 23, 2018 •

edited

Loading

glassez Sep 24, 2018

glassez left a comment

Piccirello commented Sep 27, 2018

glassez left a comment

Chocobo1 commented Dec 11, 2018

Set priority for multiple files in one WebAPI request #9541

Set priority for multiple files in one WebAPI request #9541

Conversation

Piccirello commented Sep 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

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 Sep 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glassez left a comment

Choose a reason for hiding this comment

Piccirello commented Sep 27, 2018

glassez left a comment

Choose a reason for hiding this comment

Chocobo1 commented Dec 11, 2018

Piccirello commented Sep 19, 2018 •

edited

Loading

Piccirello Sep 23, 2018 •

edited

Loading