Add save path and category editing to WebUI #9228

Piccirello · 2018-07-23T00:06:25Z

Before:

After:

GUI for comparison:

Closes #9223, closes #5716.

glassez · 2018-07-23T04:59:19Z

src/base/bittorrent/torrenthandle.cpp

@@ -1210,13 +1210,13 @@ void TorrentHandle::setName(const QString &name)
    }
 }

-bool TorrentHandle::setCategory(const QString &category)
+bool TorrentHandle::setCategory(const QString &category, const QString &savePath)


Please revert this change. It produces incorrect interface.

The bigger issue is that this function already supports creating a new category. I'll refactor the existing code.

If you want to change Core logic, please do it in separate PR. It isn't required here.

The bigger issue is that this function already supports creating a new category.

In a good way, we should allow only assign an existing category to the torrent.

Piccirello · 2018-07-23T05:56:19Z

This PR contains several, closely related changes:

Addition of save path to webui categories
Ability to edit categories from webui

glassez · 2018-07-23T17:00:15Z

src/base/bittorrent/torrenthandle.cpp

-                if (!m_session->addCategory(category))
-                    return false;
-        }
+        if (!category.isEmpty())


This if requires braces since it has multiline body.

glassez · 2018-07-23T18:04:19Z

src/webui/api/torrentscontroller.cpp

@@ -801,8 +801,14 @@ void TorrentsController::setCategoryAction()

    const QStringList hashes {params()["hashes"].split('|')};
    const QString category {params()["category"].trimmed()};
-    applyToTorrents(hashes, [category](BitTorrent::TorrentHandle *torrent)
+    const QString savePath {params()["savePath"].trimmed()};


setCategory is (should) used to assign torrent to existing category. Why this inconsistent parameter?

This setCategory API receives requests for both new and existing categories. The savePath is used when the category is new.

The savePath is used when the category is new.

It's incorrect interface.

It’s the existing interface. We can address it in a separate PR.

Really? Either you don't know what the "interface" is, or you're just kidding... There are no incorrect parameters before your changes. torrent.setCategory(category) is correct interface but torrent.setCategory(category, savePath) isn't since "savePath" has nothing to do with "assign torrent(s) to category".

glassez

You should squash last commit since it just a fixup of one of the previous.

glassez · 2018-07-30T14:03:20Z

src/webui/api/synccontroller.cpp

@@ -320,7 +320,7 @@ namespace
 //  - "full_update": full data update flag
 //  - "torrents": dictionary contains information about torrents.
 //  - "torrents_removed": a list of hashes of removed torrents
-//  - "categories": list of categories
+//  - "categories": map of categories info


Please replace "map" with "dictionary" since such term is used in other place.

Both are used throughout the documentation

glassez · 2018-07-30T14:05:21Z

src/webui/api/synccontroller.cpp


-    data["categories"] = categories;
+    data["categories"] = categoriesList;


TODO: This change causes minor API version change.

Chocobo1 · 2018-08-01T04:28:07Z

src/webui/api/torrentscontroller.cpp

    applyToTorrents(hashes, [category](BitTorrent::TorrentHandle *torrent)
    {
+        auto *session = BitTorrent::Session::instance();


this variable can be inlined.

Chocobo1 · 2018-08-01T04:29:06Z

src/webui/api/synccontroller.cpp

+    for (auto key : categories.keys()) {
+        categoriesList << QVariantMap {
+            {"name", key},
+            {"savePath", categories.value(key)},


should the comma at the end be removed?

Chocobo1 · 2018-08-01T04:30:36Z

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

@@ -120,3 +120,9 @@ function escapeHtml(str) {
    div.appendChild(document.createTextNode(str));
    return div.innerHTML;
 }
+
+function safeTrim(value) {
+    if (value === undefined || value === null)


please add parentheses to each comparison.

Chocobo1 · 2018-08-01T04:30:57Z

src/webui/www/private/newcategory.html

    <script>
+        console.log("newcategory.html");


slipped in?

Chocobo1 · 2018-08-01T04:42:28Z

src/webui/www/private/newcategory.html

-                    return false;
-                if (categoryName.match("^([^\\\\\\/]|[^\\\\\\/]([^\\\\\\/]|\\/(?=[^\\/]))*[^\\\\\\/])$") === null) {
-                    alert("QBT_TR(Invalid category name:\nPlease do not use any special characters in the category name.)QBT_TR[CONTEXT=HttpServer]");
+            var uriAction = safeTrim(new URI().getData('action'));


Maybe it is possible to write it as: String(new URI().getData('action')).trim(); instead of adding a new function?

String(null) returns "null" and String(undefined) returns "undefined" in JavaScript.

Chocobo1 · 2018-08-01T04:49:28Z

src/webui/www/private/newcategory.html

+                    return true;
+                };
+
+                if (uriAction === "set") {


There are 3 if-cases here, consider using switch()?

Piccirello · 2018-08-09T22:57:15Z

All fixed up and squashed

Chocobo1 · 2018-08-10T05:28:13Z

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

@@ -120,3 +120,9 @@ function escapeHtml(str) {
    div.appendChild(document.createTextNode(str));
    return div.innerHTML;
 }
+
+function safeTrim(value) {


String(null) returns "null" and String(undefined) returns "undefined" in JavaScript.

OK, however I think a true "safe trim" should look like this:

function safeTrim(str) { try { return str.trim(); } catch (e) { if (e instanceof TypeError) return ""; throw e; } }

Sure, will update the implementation.

Chocobo1 · 2018-08-10T12:18:44Z

Do you intend to squash the commits again? At least the last 2 commits can be merged into previous one.

Piccirello · 2018-08-10T13:26:23Z

Squashed both into the previous commit.

glassez · 2018-08-10T16:22:55Z

src/base/bittorrent/torrenthandle.cpp

-            if (!m_session->categories().contains(category))
-                if (!m_session->addCategory(category))
-                    return false;
+            if (!Session::isValidCategoryName(category) || !m_session->categories().contains(category))


Checking for valid name is redundant now.

glassez · 2018-08-10T16:31:05Z

src/webui/api/torrentscontroller.cpp

    applyToTorrents(hashes, [category](BitTorrent::TorrentHandle *torrent)
    {
+        const QStringList categories = BitTorrent::Session::instance()->categories().keys();


Where is it used?

glassez · 2018-08-10T16:33:23Z

src/webui/api/torrentscontroller.cpp

+    const QString category {params()["category"].trimmed()};
+    const QString savePath {params()["savePath"].trimmed()};
+
+    if (category.isEmpty() || savePath.isEmpty())


Empty save path is valid.

glassez · 2018-08-11T04:29:47Z

src/webui/api/synccontroller.cpp

-    QVariantList categories;
-    for (auto i = session->categories().cbegin(); i != session->categories().cend(); ++i)
-        categories << i.key();
+    QVariantList categoriesList;


You have chosen the wrong type for this task. You need to sync it as QVariantHash (with category names as keys and category info encoded as QVariantMap as values) to properly handle category changed/modified events. See "torrents" for example.

In my testing the QVariantList seems to work great. Removing a category shows it in the categories_removed section of sync/maindata. Editing a category's save path results in the category being shown in both categories and categories_removed.

Editing a category's save path results in the category being shown in both categories and categories_removed.

It's wrong behavior. It's not so critical while category has only one property (save path). Besides "category edited" events are not so frequent. Although it contradicts the "sync" feature design. "Edited" event should send only changed data, not to be transformed into pair of "removed" + "added" events.

I've switched it to a QVariantHash and now I'm seeing the intended behavior. Will push the changes.

Piccirello · 2018-08-20T06:00:02Z

Do you still allow to set torrent(s) category simultaneously with changing given category? (I see you use the same dialog for this action but you don't remove/disable "Save path" in it.)

I don't believe this was ever a feature of the GUI or WebUI. You can create a new category and set it in one action, but not modify an existing category while also setting it.

Currently you transfer almost all the logic into corresponding dialog. Why not to use it only for obtaining the data and keep the logic in "caller" JS functions? Or is it impossible? I mean, show dialog (with some initial values when needed), wait for "done" event (when the user presses "Ok"), obtain current values from it and do the appropriate action.

I like this pattern a lot. I believe we can support it with Window.postMessage, though that feature only works in IE10+. I can never remember what minimum browser versions we target, so not sure if this would be an issue.

glassez · 2018-08-20T07:32:19Z

I don't believe this was ever a feature of the GUI or WebUI.

Sorry, I'm not sure I understood you correctly...

You can create a new category and set it in one action, but not modify an existing category while also setting it.

It seems like you allow "modify an existing category while also setting it" and this behavior is incorrect. Or am I wrong and you don't allow it? Then please point me to a code preventing it.

glassez · 2018-08-20T07:33:34Z

I can never remember what minimum browser versions we target

@Chocobo1, can you answer?

Chocobo1 · 2018-08-20T09:30:22Z

@Chocobo1, can you answer?

I don't recall we ever defined it, how about we do it now. And put it in some file e.g. src/webui/www/Readme.md, @Piccirello do you mind do it?
Here is my naive proposal:

Chrome: (what is the lower bound in the industry?) -> Latest release
Firefox: Oldest active ESR branch -> Latest release
IE: 11 -> Latest release

Piccirello · 2018-08-24T00:12:59Z

It seems like you allow "modify an existing category while also setting it" and this behavior is incorrect. Or am I wrong and you don't allow it? Then please point me to a code preventing it.

Please refer to the changes in newcategory.html. There are 3 possible actions:

"set": new category is created and then set for torrent(s)
"create": new category is created
"edit": existing category is modified

There's no ability to modify an existing category and then set it on a torrent. You would need to initiate two discreet actions.

Piccirello · 2018-08-24T00:16:50Z

I don't recall we ever defined it, how about we do it now. And put it in some file e.g. src/webui/www/Readme.md

I like this proposal. We can also add the webui minimum browser requirements to the site. I do see this as a separate PR where I can also refactor this code (and possibly other forms) to use the new features unsupported on old versions of IE. For now the pattern I follow here is how we've architected all of our web forms.

glassez · 2018-08-24T12:53:12Z

"set": new category is created and then set for torrent(s)

How can I set existing category to torrent?

Piccirello · 2018-08-24T13:46:21Z

From the WebUI, right click on a torrent and select the category from the context menu.

glassez · 2018-08-24T16:23:16Z

From the WebUI, right click on a torrent and select the category from the context menu.

Oh, that's right. There's also a torrentSetCategoryFN() function that's not affected by your changes, so I didn't notice it. I have tested this PR and it works as expected.

I like this proposal. We can also add the webui minimum browser requirements to the site. I do see this as a separate PR where I can also refactor this code (and possibly other forms) to use the new features unsupported on old versions of IE.

OK.

Piccirello · 2018-08-25T19:25:21Z

Looks like this will close #9223, #5716

thalieht · 2018-08-26T07:31:33Z

@Piccirello you can add "closes #9223, closes #5716" or "fixes #xxxx" in the body of first post. Note that closes/fixes must exist before every issue mention for github to close it, same goes for when doing it in commits.

Edit: I did it here in case this gets merged before you see this.

Piccirello · 2018-08-27T04:32:53Z

Thanks thalieht, I didn’t know that was an official feature.

zeule · 2018-08-31T11:54:54Z

src/webui/api/torrentscontroller.cpp

@@ -821,10 +822,30 @@ void TorrentsController::createCategoryAction()
    checkParams({"category"});

    const QString category {params()["category"].trimmed()};
-    if (!BitTorrent::Session::isValidCategoryName(category) && !category.isEmpty())
+    const QString savePath {params()["savePath"].trimmed()};


Path string may end with whitespace.

zeule · 2018-08-31T11:59:04Z

src/webui/api/synccontroller.cpp

-    QVariantList categories;
-    for (auto i = session->categories().cbegin(); i != session->categories().cend(); ++i)
-        categories << i.key();
+    QVariantHash categoriesHash;


I can imagine why did you rename the variable while making the changes. But I don't think it has to get a new name in the final patch. Also, "hash" is a ambiguous suffix, which is used elsewhere in the project to refer to the hash value but not a hash table.

This is pretty pedantic, but I'll make the change.

I appreciate that!

Piccirello · 2018-09-10T20:40:58Z

If no more comments, please approve

glassez · 2018-09-13T16:43:51Z

Thank you!

glassez requested changes Jul 23, 2018

View reviewed changes

glassez added the WebUI WebUI-related issues/changes label Jul 23, 2018

Piccirello force-pushed the addCategory branch from 992e03f to 55510a8 Compare July 23, 2018 05:52

Piccirello changed the title ~~Add save path to WebUI new category dialog~~ Add save path and category editing to WebUI Jul 23, 2018

glassez requested changes Jul 23, 2018

View reviewed changes

glassez added the WebAPI WebAPI-related issues/changes label Jul 23, 2018

Piccirello force-pushed the addCategory branch from 02e722f to a2cf008 Compare July 24, 2018 03:10

thalieht mentioned this pull request Jul 24, 2018

[Feature] Categories allow definition of save location #9223

Closed

glassez requested changes Jul 30, 2018

View reviewed changes

Chocobo1 reviewed Aug 1, 2018

View reviewed changes

Piccirello force-pushed the addCategory branch 2 times, most recently from e8b62eb to a25452a Compare August 9, 2018 22:56

Chocobo1 reviewed Aug 10, 2018

View reviewed changes

Piccirello force-pushed the addCategory branch from 11a8331 to f649996 Compare August 10, 2018 13:26

Chocobo1 previously approved these changes Aug 10, 2018

View reviewed changes

glassez requested changes Aug 10, 2018

View reviewed changes

glassez requested changes Aug 11, 2018

View reviewed changes

Piccirello dismissed Chocobo1’s stale review via 694df5a August 14, 2018 03:57

Piccirello force-pushed the addCategory branch from 17c3964 to 1d9cfed Compare August 14, 2018 04:19

Piccirello added 2 commits August 18, 2018 22:04

Require torrent category creation to be explicit

d89ef22

Include category save path in web api sync data

a0e6007

glassez previously approved these changes Aug 24, 2018

View reviewed changes

Chocobo1 previously approved these changes Aug 25, 2018

View reviewed changes

glassez requested a review from zeule August 31, 2018 08:11

zeule suggested changes Aug 31, 2018

View reviewed changes

Add save path and editing to WebUI new category dialog

63c5376

Piccirello dismissed stale reviews from Chocobo1 and glassez via 63c5376 August 31, 2018 15:27

Piccirello force-pushed the addCategory branch from b404017 to 63c5376 Compare August 31, 2018 15:27

glassez approved these changes Sep 11, 2018

View reviewed changes

zeule approved these changes Sep 11, 2018

View reviewed changes

Chocobo1 approved these changes Sep 12, 2018

View reviewed changes

glassez merged commit 28a6ac3 into qbittorrent:master Sep 13, 2018

Piccirello mentioned this pull request Oct 17, 2018

API v2.1.0 - torrents/setCategory 409 Conflict error for new category #9719

Closed

Piccirello mentioned this pull request Mar 21, 2019

Control/Edit RSS feeds from the Web UI #453

Closed

Piccirello deleted the addCategory branch July 15, 2019 00:45

requinix mentioned this pull request Oct 11, 2024

Web API can create and assign categories in a single operation #21588

Open


		data["categories"] = categories;
		data["categories"] = categoriesList;

Add save path and category editing to WebUI #9228

Add save path and category editing to WebUI #9228

Conversation

Piccirello commented Jul 23, 2018 • edited by thalieht 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

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

glassez left a 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 Aug 9, 2018

Chocobo1 Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 commented Aug 10, 2018

Piccirello commented Aug 10, 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 Aug 20, 2018

glassez commented Aug 20, 2018

glassez commented Aug 20, 2018

Chocobo1 commented Aug 20, 2018

Piccirello commented Aug 24, 2018

Piccirello commented Aug 24, 2018

glassez commented Aug 24, 2018

Piccirello commented Aug 24, 2018

glassez commented Aug 24, 2018

Piccirello commented Aug 25, 2018

thalieht commented Aug 26, 2018 • edited Loading

Piccirello commented Aug 27, 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

Piccirello commented Sep 10, 2018

glassez commented Sep 13, 2018

Piccirello commented Jul 23, 2018 •

edited by thalieht

Loading

Chocobo1 Aug 10, 2018 •

edited

Loading

thalieht commented Aug 26, 2018 •

edited

Loading