Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply filename filter to subfolder names as well #20902

Merged
merged 2 commits into from
Jun 12, 2024

Conversation

glassez
Copy link
Member

@glassez glassez commented May 30, 2024

Closes #14480.

@glassez glassez added the Core label May 30, 2024
@glassez glassez added this to the 5.0 milestone May 30, 2024
@glassez glassez requested a review from a team May 30, 2024 08:55
@thalieht
Copy link
Contributor

Apply filename filter to file paths

How do i test/use this?

@glassez
Copy link
Member Author

glassez commented May 30, 2024

Apply filename filter to file paths

How do i test/use this?

You can follow #14480 (comment)

(I haven't tested it myself yet.)

@thalieht
Copy link
Contributor

Doesn't work.

@glassez
Copy link
Member Author

glassez commented May 30, 2024

Doesn't work.

Could you test it now?

There are pitfalls in how Qt interprets wildcards.
I had to use some workaround. Although it has some limitations, I think it should suit the interested parties. Otherwise, we will need to raise minimal supported Qt to v6.6.
In short, now it applies a given pattern alternately to all components of the relative path of the torrent files. I.e. you cannot match the entire relative path, but only its individual components. For example, "foo" will match all the files and subfolders called "foo" (namely all the files that have name "foo" or have "foo" as one of their parent subfolder).

@thalieht
Copy link
Contributor

Any chance the root folder will be excluded? It seems like a bug if any pattern matches it and ends up deselecting everything.

@glassez
Copy link
Member Author

glassez commented May 30, 2024

Any chance the root folder will be excluded? It seems like a bug if any pattern matches it and ends up deselecting everything.

I think I could take care of that. But does it make sense? Note that there are several other similar cases:

  1. Single file matches pattern
  2. Each file of multi file torrent matches one or another of the specified patterns (so all of them are excluded)

What do you think we should do there?

@glassez

This comment was marked as outdated.

@thalieht
Copy link
Contributor

  1. Single file matches pattern

Should also be excluded IMO.

  1. Each file of multi file torrent matches one or another of the specified patterns (so all of them are excluded)

User should re-evaluate his patterns / works as advertised.

@glassez glassez marked this pull request as draft May 30, 2024 16:51
@glassez
Copy link
Member Author

glassez commented May 31, 2024

  1. Single file matches pattern

Should also be excluded IMO.

  1. Each file of multi file torrent matches one or another of the specified patterns (so all of them are excluded)

User should re-evaluate his patterns / works as advertised.

It turned out that it was easier and more reliable to cover all such cases at once.
The algorithm is as follows:
If all files are excluded after applying filters, then do not apply the filter at all.
We may have to make this behavior optional if we get negative feedback from some users, but right now I don't want to add any new options...

@glassez glassez marked this pull request as ready for review May 31, 2024 07:55
@thalieht
Copy link
Contributor

If all files are excluded after applying filters, then do not apply the filter at all.

That doesn't seem right. I was about to report that it doesn't work at all because a pattern matched the root folder until i read this. I'm sure i'm not the only one who will consider this a bug.

@glassez
Copy link
Member Author

glassez commented May 31, 2024

That doesn't seem right. I was about to report that it doesn't work at all because a pattern matched the root folder until i read this. I'm sure i'm not the only one who will consider this a bug.

Unfortunately, I have to admit that I don't understand exactly what you mean. Could you describe how you think it should behave in the above cases, when all files are excluded by the filter? Don't we have only two ways, i.e. either accept it (add a torrent with all the files excluded), or reject it (don't apply a filter for this torrent)? We can only report something in addition to one of these reactions.

@glassez
Copy link
Member Author

glassez commented May 31, 2024

Reverted the last commit.
The problem with the possibility of excluding all files by filename filter was not created by this PR, currently it is also possible to set such a set of patterns. So I suggest we postpone it for the future, if it is indeed requested by someone to be changed/fixed in some way.

@thalieht
Copy link
Contributor

Could you describe how you think it should behave in the above cases, when all files are excluded by the filter?

I don't know how things work on the code level. i was thinking in terms of "don't apply any pattern on the root folder (is it an item?) and if there is only 1 file in the torrent but apply the patterns to everything else" e.g. patterns:
s01
*.jpg
on a torrent with the root folder named "s01 something" with contents

  • s01e01 something
  • s01e02 something
  • image.jpg
  • text.txt

It would filter everything except the "text.txt" file. I mean it would make no sense to ignore all patterns because one of them matched the root folder i.e. ignore *.jpg in the example.

We can only report something in addition to one of these reactions.

I don't understand what that means.

@glassez
Copy link
Member Author

glassez commented Jun 1, 2024

I don't know how things work on the code level.

I don't ask about the code.

don't apply any pattern on the root folder (is it an item?) and if there is only 1 file in the torrent but apply the patterns to everything else

It makes sense, at least to me. But I can't imagine the possible expectations of other users from this feature. For example, if torrents are added automatically, how likely is it that the user does not want to download certain types of files anyway, even if some torrents do not contain anything else?

@thalieht
Copy link
Contributor

thalieht commented Jun 1, 2024

For example, if torrents are added automatically, how likely is it that the user does not want to download certain types of files anyway, even if some torrents do not contain anything else?

Good point, i forgot about that.

@glassez glassez changed the title Apply filename filter to file paths Apply filename filter to subfolder names as well Jun 2, 2024
@xavier2k6
Copy link
Member

Otherwise, we will need to raise minimal supported Qt to v6.6

there are fixes for gtk3 theme glitching in 6.5 series however they are behind LTS but are also available in 6.6/6.7 series, we also know that some other issues as noted via CI Qt changing that are fixed with later revisions of Qt 6.6/6.7 Series

The next Qt LTS will be 6.8 due around Sept. '24.

@glassez
Copy link
Member Author

glassez commented Jun 10, 2024

Otherwise, we will need to raise minimal supported Qt to v6.6

In any case, it should be borne in mind that these are different ways, and I do not know which one would be preferable.
Now (with this PR) you can specify foo to exclude any subfolder or file with that name (without caring about its depth).
Otherwise (if the pattern is applied to the entire subpath), it may not be so easy. For example, foo will only match if the torrent contains a single file named "foo" without subfolders, and *foo* will match any file or subfolder not only named "foo", but also with a name containing the substring "foo" in it.

Copy link

@8tm 8tm left a comment

Choose a reason for hiding this comment

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

Looks good to me

@glassez glassez merged commit d89f289 into qbittorrent:master Jun 12, 2024
14 checks passed
@glassez glassez deleted the filename-filter branch June 12, 2024 06:02
sledgehammer999 pushed a commit to sledgehammer999/qBittorrent that referenced this pull request Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List of paths or files to ignore in every new torrent
4 participants