WebUI: Add "isPrivate" filter for 'info' API #20833

ManiMatter · 2024-05-13T11:53:06Z

It is already possible to filter the "info" response by hashes, status, tag, category.
Since the "isPrivate" flag has been added in #20686, it would be nice if it was possible to also pre-filter the response by this attribute.

I have - to the best of my abilities - implemented the code, but it is at the edge of my skills and I would appreciate your review.

The failed checks seem macOS related and affecting any PRs right now, thus not triggered by the changes I propose here

src/base/torrentfilter.h

src/webui/api/serialize/serialize_torrent.h

src/webui/api/torrentscontroller.cpp

src/base/torrentfilter.cpp

ManiMatter · 2024-05-16T18:46:59Z

@glassez Thanks having included this on milestone 5.0, and for having reviewed the code. I was not able to figure out how to test whether the feature works properly. Just wanted to check if you had a chance to test if it actually works?

Many thanks

ManiMatter · 2024-06-06T11:47:15Z

@glassez - quick question, do I need to do anything here to get this merged?

glassez · 2024-06-06T12:24:00Z

@glassez - quick question, do I need to do anything here to get this merged?

At least it should be confirmed by someone from the team (or maybe by couple of other people) that it works as intended.

ManiMatter · 2024-06-06T13:14:03Z

Thank you for the fast response. I unfortunately did not figure out how to build a docker image from the code so that I could test it myself.

Did you test it on your end by any chance? If so, did it work?

Looking forward for the other people to test it (guess that is done via qbittorrent/web-developers ?)

src/webui/api/serialize/serialize_torrent.h

src/webui/api/serialize/serialize_torrent.cpp

src/base/torrentfilter.h

Chocobo1 · 2024-06-06T16:45:58Z

src/base/torrentfilter.h

@@ -70,6 +70,7 @@ class TorrentFilter
    static const std::optional<QString> AnyCategory;
    static const std::optional<TorrentIDSet> AnyID;
    static const std::optional<Tag> AnyTag;
+    static const std::optional<bool> AnyIsPrivate;


I would name it AnyPrivate.

src/base/torrentfilter.h

Chocobo1 · 2024-06-06T16:49:17Z

src/base/torrentfilter.cpp

@@ -34,6 +34,7 @@
 const std::optional<QString> TorrentFilter::AnyCategory;
 const std::optional<TorrentIDSet> TorrentFilter::AnyID;
 const std::optional<Tag> TorrentFilter::AnyTag;
+const std::optional<bool> TorrentFilter::AnyIsPrivate;


I would name it AnyPrivate.

AnyPrivate looks confusing.

Now since we use std::optional these constants don't do much good. We can use std::nullopt directly and not bother to come up with names for these constants.

We can use std::nullopt directly and not bother to come up with names for these constants.

👍

src/webui/api/torrentscontroller.cpp

Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>

ManiMatter · 2024-06-06T21:03:10Z

@Chocobo1 - many thanks for your review. I incorporated your changes.

Regarding renaming: I am not sure I understand your suggestion to rename the "isPrivate" to "private" in the various places. Since the flag that is returned currently in the properties is called "isPrivate", I feel it makes sense to keep calling it like this, rather than changing to "private". What was your thinking when suggesting the rename?

In terms of functionality, did the feature work for you as expected?

Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>

Chocobo1 · 2024-06-07T13:49:08Z

Regarding renaming: I am not sure I understand your suggestion to rename the "isPrivate" to "private" in the various places. Since the flag that is returned currently in the properties is called "isPrivate", I feel it makes sense to keep calling it like this, rather than changing to "private". What was your thinking when suggesting the rename?

It looks weird to me.
For example: bool TorrentFilter::matchIsPrivate(const BitTorrent::Torrent *const torrent) and without going into the internals, what is it supposed to mean? A match/matching is private... what? No, it is matching for a private flag/property on a torrent object. Using another name like matchPrivate (or matchPrivateTorrent, matchPrivateFlag, etc.) could avoid the confusion IMO. Look at other similar functions, they aren't that hard to read and understand: matchState, matchHash, matchCategory.
Another one: TorrentFilter::TorrentFilter(..., const std::optional<bool> isPrivate). What does it mean for the parameter by "is private"? Is it running a test (with some outcome) on the parameter/variable? No, I suppose the function want some state for a "private" flag/property. And therefore I would suggest just call it private (or privateFlag, privateState or something like that).

In terms of functionality, did the feature work for you as expected?

I haven't got time to test. BTW our CI do provide build artifacts, you can use it to verify.

glassez · 2024-06-07T14:25:07Z

And therefore I would suggest just call it private

👍
The torrent property is called "private". Using the Qt naming convention getter/setter methods are called isPrivate/setPrivate accordingly. But in most cases we have to refer to the original property name (i.e. without is prefix).

ManiMatter · 2024-06-07T14:28:03Z

BTW our CI do provide build artifacts, you can use it to verify.

Thank you for the pointer, which helped me find the CI artefacts.
However, I am not sure I know how to run them, would you mind pointing me in the right direction once more?

In my regular setup, I run a docker container which I pull from lscr.io/linuxserver/qbittorrent:latest.
My take is that the artefacts from the CI do not provide a docker image, correct?

If that's the case, I'm left off with my Mac to test (which runs on Sonoma).
Under the Mac OS checks, I see four options:
build 2.0.10 vs build 1.2.19 and GUI on vs GUI off.
What is the difference between those?

Lastly, after downloading them and unpacking the .zip, I see there is a .dmg inside and a cmake folder. The app inside the .dmg is crossed out in my case and when trying to open it I get a message "Not supported on this Mac".

Appreciate your patience and help - happy to support testing.. Just need to figure out how to get the artefacts to run.

ManiMatter · 2024-06-07T15:00:35Z

mh. I changed the naming convention across the board, but now CI complains, since the word "private" is a reserved word in C++. Any suggestions what to do? Change to "privateFlag"?

glassez · 2024-06-07T15:06:49Z

mh. I changed the naming convention across the board, but now CI complains, since the word "private" is a reserved word in C++. Any suggestions what to do? Change to "privateFlag"?

I couldn't imagine that you would misinterpret my comment above so much. I definitely didn't want you to change naming convention we use. I was just stating a fact about such a convention so that you could understand where the name of the method isPrivate came from.

ManiMatter · 2024-06-07T15:12:35Z

sorry. new to all of this.. let me revert the commit.
My understanding was that the suggestion was to consistently rename the "is private" to "private"; I must have misunderstood.

Could you again please point me in the right direction in what sense I shall change the namings? Are we only talking about the matchIsPrivate and setIsPrivate functions to matchPrivate and setPrivate?

Chocobo1 · 2024-06-08T06:39:23Z

In my regular setup, I run a docker container which I pull from lscr.io/linuxserver/qbittorrent:latest.
My take is that the artefacts from the CI do not provide a docker image, correct?

Yes, no docker image.

If that's the case, I'm left off with my Mac to test (which runs on Sonoma).
Under the Mac OS checks, I see four options:
build 2.0.10 vs build 1.2.19 and GUI on vs GUI off.
What is the difference between those?

Different libtorrent versions and with GUI or not. Probably doesn't matter for you.

Lastly, after downloading them and unpacking the .zip, I see there is a .dmg inside and a cmake folder. The app inside the .dmg is crossed out in my case and when trying to open it I get a message "Not supported on this Mac".

It seems our mac artifacts is for ARM. You're out of luck then...

ManiMatter · 2024-06-08T08:07:07Z

Many thanks for the guidance, @Chocobo1 . Besides not being 100% on the proposed renamings of the variables/functions (last post), I unfortunately do not know how to finish this PR without being able to test myself. Do you see any way that I can test (or would you know about someone who could support)?

ManiMatter mentioned this pull request May 13, 2024

WebUI: Add "isPrivate" filter for 'info' API #20808

Closed

thalieht added the WebAPI WebAPI-related issues/changes label May 13, 2024

ManiMatter force-pushed the info-API-filter-by-isPrivate-flag branch 3 times, most recently from 31955e6 to 37ee324 Compare May 13, 2024 13:37

ManiMatter and others added 2 commits May 13, 2024 15:41

Add "isPrivate" filter for 'info'-API

a2377c8

renamed KEY_TORRENT_ISPRIVATE to KEY_TORRENT_IS_PRIVATE for consistency

44e71b0

ManiMatter force-pushed the info-API-filter-by-isPrivate-flag branch from 37ee324 to 44e71b0 Compare May 13, 2024 13:41

Merge branch 'master' into info-API-filter-by-isPrivate-flag

f341038

glassez requested changes May 16, 2024

View reviewed changes

glassez reviewed May 16, 2024

View reviewed changes

src/base/torrentfilter.cpp Outdated Show resolved Hide resolved

glassez reviewed May 16, 2024

View reviewed changes

src/base/torrentfilter.cpp Outdated Show resolved Hide resolved

glassez reviewed May 16, 2024

View reviewed changes

src/base/torrentfilter.cpp Outdated Show resolved Hide resolved

Fix coding style

dafb7ca

glassez requested review from a team May 16, 2024 13:19

glassez added this to the 5.0 milestone May 16, 2024

ManiMatter requested a review from glassez May 16, 2024 16:50

Chocobo1 reviewed Jun 6, 2024

View reviewed changes

ManiMatter and others added 6 commits June 6, 2024 22:38

Update src/webui/api/serialize/serialize_torrent.h

27c4bff

Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>

Update src/webui/api/serialize/serialize_torrent.cpp

0da67bf

Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>

Update src/base/torrentfilter.h

f09e3a7

Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>

Update src/base/torrentfilter.h

6451bd6

Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>

Update src/base/torrentfilter.h

1b34b54

Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>

Merge branch 'qbittorrent:master' into info-API-filter-by-isPrivate-flag

82ed3b4

ManiMatter and others added 4 commits June 7, 2024 06:36

Update src/base/torrentfilter.cpp

0b25498

Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>

Update src/base/torrentfilter.cpp

d5a3248

Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>

Update src/base/torrentfilter.cpp

598b602

Co-authored-by: Chocobo1 <Chocobo1@users.noreply.github.com>

Update serialize_torrent.cpp

47c05fd

ManiMatter force-pushed the info-API-filter-by-isPrivate-flag branch from 7f51598 to 47c05fd Compare June 7, 2024 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WebUI: Add "isPrivate" filter for 'info' API #20833

WebUI: Add "isPrivate" filter for 'info' API #20833

ManiMatter commented May 13, 2024 •

edited

ManiMatter commented May 16, 2024

ManiMatter commented Jun 6, 2024

glassez commented Jun 6, 2024

ManiMatter commented Jun 6, 2024

Chocobo1 Jun 6, 2024

Chocobo1 Jun 6, 2024

glassez Jun 7, 2024

Chocobo1 Jun 8, 2024

ManiMatter commented Jun 6, 2024 •

edited

Chocobo1 commented Jun 7, 2024 •

edited

glassez commented Jun 7, 2024

ManiMatter commented Jun 7, 2024

ManiMatter commented Jun 7, 2024

glassez commented Jun 7, 2024

ManiMatter commented Jun 7, 2024

Chocobo1 commented Jun 8, 2024

ManiMatter commented Jun 8, 2024

WebUI: Add "isPrivate" filter for 'info' API #20833

Are you sure you want to change the base?

WebUI: Add "isPrivate" filter for 'info' API #20833

Conversation

ManiMatter commented May 13, 2024 • edited

ManiMatter commented May 16, 2024

ManiMatter commented Jun 6, 2024

glassez commented Jun 6, 2024

ManiMatter commented Jun 6, 2024

Chocobo1 Jun 6, 2024

Choose a reason for hiding this comment

Chocobo1 Jun 6, 2024

Choose a reason for hiding this comment

glassez Jun 7, 2024

Choose a reason for hiding this comment

Chocobo1 Jun 8, 2024

Choose a reason for hiding this comment

ManiMatter commented Jun 6, 2024 • edited

Chocobo1 commented Jun 7, 2024 • edited

glassez commented Jun 7, 2024

ManiMatter commented Jun 7, 2024

ManiMatter commented Jun 7, 2024

glassez commented Jun 7, 2024

ManiMatter commented Jun 7, 2024

Chocobo1 commented Jun 8, 2024

ManiMatter commented Jun 8, 2024

ManiMatter commented May 13, 2024 •

edited

ManiMatter commented Jun 6, 2024 •

edited

Chocobo1 commented Jun 7, 2024 •

edited