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

Add regex name filter #619

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

MarelliF
Copy link

Hello,

I implemented a simple name regex check to allow filtering what images are shown.
It is integrated in the preferences dialog under the Filtering category.
This is useful mostly when using a local folder source.

I ran the tests prior to this pull request, all pass except TestNationalGeographicDownloader.TestNationalGeographicDownloader, which is not related to my modifications.
I tested a local build on my own machine (Manjaro Gnome), all works as expected.

Please let me know if I should modify anything for this to be merged in the master branch, I would be happy to help.

Best regards,
Francois

# name_regex_enabled = <True or False>
# name_regex = <regex>
name_regex_enabled = False
name_regex = *
Copy link
Member

Choose a reason for hiding this comment

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

nit: the default regex should probably be .* instead of *, as * alone isn't a valid regex

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, I updated the defaults to be a valid .*.
Thanks for spotting it.

@jlu5 jlu5 requested a review from peterlevi January 20, 2024 22:24
@peterlevi
Copy link
Member

peterlevi commented Jan 21, 2024

Regexes are mostly a programming concept and most users would not be familiar with them. IMHO, when we are talking about filtering by filename, globs are probably a more suitable, popular and easy-to-use concept - https://en.wikipedia.org/wiki/Glob_(programming).
Is there any strong reason to implement this filtering around regexes rather than globs? @jlu5 WDYT?

In both cases, but especially for regexes, it will be good if the preferences UI has a link to an external documentation on globs/regexes (for globs, a description with 1-2 examples for the use of ? and *, plus a link to read more may be better).

Additionally (but it would make the implementation more complex), since this mostly makes sense for local files, it may be better if this is not an option in Filtering, but rather an option that can be specified separately on all "local folder" sources. E.g. I may be interested in all **/*elephant* from my Pictures/Animals folder, and all **/*savannah* in the Pictures/Landscapes folder.

@jlu5
Copy link
Member

jlu5 commented Feb 7, 2024

Regexes are mostly a programming concept and most users would not be familiar with them. IMHO, when we are talking about filtering by filename, globs are probably a more suitable, popular and easy-to-use concept - https://en.wikipedia.org/wiki/Glob_(programming). Is there any strong reason to implement this filtering around regexes rather than globs? @jlu5 WDYT?

I don't have a strong opinion here. If we only focus on matching filenames, regexps are more expressive with little extra cost (Python's glob matching uses regex under the hood anyways), and the simplest usecase with wildcards aren't that much harder to create compared to globs IMO.

In both cases, but especially for regexes, it will be good if the preferences UI has a link to an external documentation on globs/regexes (for globs, a description with 1-2 examples for the use of ? and *, plus a link to read more may be better).

I agree on this.

Additionally (but it would make the implementation more complex), since this mostly makes sense for local files, it may be better if this is not an option in Filtering, but rather an option that can be specified separately on all "local folder" sources. E.g. I may be interested in all **/*elephant* from my Pictures/Animals folder, and all **/*savannah* in the Pictures/Landscapes folder.

If we go with full path search it'd make more sense to use globs, yeah. Matching directories of arbitrary depth isn't nearly as straightforward with regex.

But maybe glob and regex filters don't need to be mutually exclusive more generally 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants